Bug 68149

Created attachment 107546 [details] Patch

Comment on attachment 107546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107546&action=review I did not review the rest of the patch. > Source/WebCore/rendering/style/RenderStyle.h:1326 > + bool inheritsParentProperty() { return m_inheritsParentProperty; } This should be |const|

Comment on attachment 107546 [details] Patch Attachment 107546 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9685869

Created attachment 107668 [details] Patch

Comment on attachment 107668 [details] Patch Ah, nice to see someone tackling this old gem! It's trickier than it looks! I like the idea of trying to track this in the RenderStyle. However, you're patching the wrong place. I think you just want to make Node::diff return the right value (Inherited) when an inherited property change is detected. I would even go further and have RenderStyles maintain a hash of all properties they inherit. If you know the actual properties, then the diff could actually check to see only if those specific properties changed. It seems like we should be able to do better as far as places we have to patch. There should be a chokepoint we can find for inheritance. Just patching a few places in CSSStyleApplyProperty.cpp doesn't cut it. Probably a good chokepoint is when the declarations have been collected and are being walked. I think you should be able to maintain some sort of map as you're applying the declarations (that gets cleared as non-inherited values are encountered). Then what you're left with is what you could put on the RenderStyle.

To clarify, I think it's cleaner to just taint the parent when a child has a property they explicitly inherit. That way around the same place in Node::diff that checks s1->inheritedNonEqual(s2) you can also check s1->explicitlyInheritedNonEqual(s2) or something like that, and that check can then look at the list of properties set in the parent style that are inherited by some child explicitly and see if they are non-equal. I think it would be a fine first step to just taint the parent globally if you don't want to do property-specific stuff yet and just return Inherit period if any property changes. We can refine the performance later. It's important, though, that the taint only apply to non-inherited properties. A property that inherits by default that a user explicitly inherits shouldn't cause a taint.

Created attachment 108684 [details] Patch

Comment on attachment 108684 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108684&action=review > Source/WebCore/css/CSSStyleSelector.cpp:2304 > +static bool implicitlyInherited(CSSPropertyID property) Should probably be named propertyIsImplicitlyInherited.

Comment on attachment 108684 [details] Patch Attachment 108684 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9882095 New failing tests: fast/table/border-collapsing/cached-change-tbody-border-color.html

(In reply to comment #9) > (From update of attachment 108684 [details]) > Attachment 108684 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/9882095 > > New failing tests: > fast/table/border-collapsing/cached-change-tbody-border-color.html This is a false positive I think.

Created attachment 109056 [details] Patch

Comment on attachment 109056 [details] Patch Attachment 109056 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9884600 New failing tests: fast/table/border-collapsing/cached-change-tbody-border-color.html

(In reply to comment #12) > (From update of attachment 109056 [details]) > Attachment 109056 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/9884600 > > New failing tests: > fast/table/border-collapsing/cached-change-tbody-border-color.html Konstantin, you introduced this test - the failure is that the repainted area extends to the area of the blue border instead of the yellow border, i.e. the table instead of the tbody. Does this indicate a problem do you think? I've tested this patch against https://bug-64546-attachments.webkit.org/attachment.cgi?id=100837 and it causes no regressions in the improved performance introduced by bug 64546.

Created attachment 109185 [details] Pixel result from border test

> > New failing tests: > > fast/table/border-collapsing/cached-change-tbody-border-color.html > > Konstantin, you introduced this test - the failure is that the repainted area extends to the area of the blue border instead of the yellow border, i.e. the table instead of the tbody. > > Does this indicate a problem do you think? What this test checks is that border color changes from "pink" to "yellow". As I can see in your version this behavior is kept, so this should be OK. I'm not sure why there is such difference in repaint area with your test, but your version looks better for me.

Comment on attachment 109056 [details] Patch The patch looks good. I'm giving r- only because Antti happens to have implemented the same function (whether or not a property is inherited), so you guys need to coordinate. I think his patch has r+ or is close to landing, so I think you'll want to reformulate your patch after he lands to use his new function.

Created attachment 112752 [details] Patch

Comment on attachment 112752 [details] Patch Attachment 112752 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10223915 New failing tests: fast/table/border-collapsing/cached-change-tbody-border-color.html

Created attachment 112882 [details] Patch

Comment on attachment 112882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112882&action=review r=me. One minor fix I'd like you to make though. > Source/WebCore/css/CSSStyleSelector.cpp:2488 > + if (isInherit && m_parentStyle && !CSSProperty::isInheritedProperty(property)) > + m_parentStyle->setHasExplicitlyInheritedProperties(); Minor optimization you could make here is to add !m_parentStyle->hasExplicitlyInheritedProperties() to the if. That way once the parent style is tainted by one child, you don't waste time checking other children. I'm thinking of the case where you have a parent with a bunch of kids that all inherit. The first kid taints you. No need to keep checking properties when you look at the other kids.

Committed r98805: <http://trac.webkit.org/changeset/98805>