CSS 2.1 failure: dynamic-top-change-001 to 004 fail

• Component: New Bugs
• Status: RESOLVED FIXED

[WebKit Review Bot]

help
Requested by mwenge2 on #webkit.

[Robert Hogan]

Created attachment 107546 [details]
Patch

[Simon Fraser (smfr)]

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|

[WebKit Review Bot]

Comment on attachment 107546 [details]
Patch

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

[Robert Hogan]

Created attachment 107668 [details]
Patch

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[Robert Hogan]

Created attachment 108684 [details]
Patch

[Darin Adler]

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.

[WebKit Review Bot]

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

[Robert Hogan]

(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.

[Robert Hogan]

Created attachment 109056 [details]
Patch

[WebKit Review Bot]

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

[Robert Hogan]

(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.

[Robert Hogan]

Created attachment 109185 [details]
Pixel result from border test

[Konstantin Shcheglov]

> > 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.

[Dave Hyatt]

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.

[Robert Hogan]

Created attachment 112752 [details]
Patch

[WebKit Review Bot]

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

[Robert Hogan]

Created attachment 112882 [details]
Patch

[Dave Hyatt]

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.

[Robert Hogan]

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