Bug 100397 – Remove setRenderStyle in favor of callbacks on HTMLOptionElement and HTMLOptGroupElement

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Eliminate setRenderStyle, you really want setNonRendererRenderStyle

[Elliott Sprehn]

Created attachment 170692 [details]
Patch

[Early Warning System Bot]

Comment on attachment 170692 [details]
Patch

Attachment 170692 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14563677

[Early Warning System Bot]

Comment on attachment 170692 [details]
Patch

Attachment 170692 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14543745

[Elliott Sprehn]

Created attachment 170697 [details]
Patch

Try again

[Ojan Vafai]

Comment on attachment 170697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170697&action=review

Nice cleanup!

> Source/WebCore/ChangeLog:8
> +        Introduce a new method setNonRendererRenderStyle that more explicitly does

Bikeshed nit: setNonRendererStyle. It's less verbose and consistent with setStyleInternal and setAnimatableStyle.

> Source/WebCore/ChangeLog:14
> +        also hides the if statement protecting against null rendererers meaning we end up

typo: rendererers

> Source/WebCore/ChangeLog:21
> +

You need to update the list below. Specifically, it's missing the removal of setRenderStyle. I think that deserves a comment explaining why it's being removed.

> Source/WebCore/ChangeLog:26
> +        (WebCore::Element::pseudoStyleCacheIsInvalid):

Might be worth nothing that the only place this is called is in Element::recalcStyle.

> Source/WebCore/dom/Element.cpp:1168
> +                renderer->setAnimatableStyle(newStyle.get());

Technically, this is a one-line control clause and should not have curlies. May as well fix it while you're in this code.

[Build Bot]

Comment on attachment 170697 [details]
Patch

Attachment 170697 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14563685

[Eric Seidel (no email)]

Comment on attachment 170697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170697&action=review

> Source/WebCore/dom/Node.h:533
> +    virtual void setNonRendererRenderStyle(PassRefPtr<RenderStyle>) { };

I wouldn't bother even putting this in the header.  I don't see the advantage of it being inlined on gcc.

[WebKit Review Bot]

Comment on attachment 170697 [details]
Patch

Attachment 170697 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14544751

New failing tests:
fast/text/international/pop-up-button-text-alignment-and-direction.html
fast/text/international/bidi-menulist.html
fast/repaint/control-clip.html
fast/events/tabindex-focus-blur-all.html
fast/forms/select/optgroup-rendering.html

[Ojan Vafai]

These look like real failures.

[Elliott Sprehn]

Created attachment 170760 [details]
Patch

[Elliott Sprehn]

(In reply to comment #10)
> Created an attachment (id=170760) [details]
> Patch

Want to look again Ojan? Turns out we need a little more finesse to make this work, but in the end this eliminates more virtual calls and that if else tree in recalcStyle turns out to actually only be 2 cases. :)

[Eric Seidel (no email)]

Comment on attachment 170760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170760&action=review

> Source/WebCore/dom/Element.cpp:1157
> +        } else if (usesNonRendererStyle())

So this is now an extra virtual call in teh no-renderer case.  I'm not sure how often we recalc style and don't have renderers (if at all).

It's interesting that SVGGradientStops already do somethign like this but w/o hacking into the normal recalc style.  They force their own recalc when their style is needed instead of signing up for a non-renderer style like this.  I'm not sure which method is better, but we clearly don't need 2 in the long run.

[Darin Adler]

Comment on attachment 170760 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170760&action=review

> Source/WebCore/ChangeLog:31
> +        (Element):

Best to delete bogus lines like this even if the change log script spits them out.

> Source/WebCore/ChangeLog:40
> +        (Node):

Best to delete bogus lines like this even if the change log script spits them out.

> Source/WebCore/ChangeLog:47
> +        (WebCore):

Best to delete bogus lines like this even if the change log script spits them out.

> Source/WebCore/WebCore.order:1540
> -__ZN7WebCore4Node14setRenderStyleEN3WTF10PassRefPtrINS_11RenderStyleEEE
> +__ZN7WebCore4Node25setNonRendererRenderStyleEN3WTF10PassRefPtrINS_11RenderStyleEEE

No need to edit order files. They are regenerated periodically and need not be maintained by hand.

> Source/WebCore/html/HTMLOptGroupElement.h:58
> +    virtual RenderStyle* nonRendererStyle() const;

Need OVERRIDE here.

> Source/WebCore/html/HTMLOptionElement.h:86
> +    virtual RenderStyle* nonRendererStyle() const;

Need OVERRIDE here.

[Elliott Sprehn]

(In reply to comment #12)
> (From update of attachment 170760 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170760&action=review
> 
> > Source/WebCore/dom/Element.cpp:1157
> > +        } else if (usesNonRendererStyle())
> 
> So this is now an extra virtual call in teh no-renderer case.  I'm not sure how often we recalc style and don't have renderers (if at all).
> 
> It's interesting that SVGGradientStops already do somethign like this but w/o hacking into the normal recalc style.  They force their own recalc when their style is needed instead of signing up for a non-renderer style like this.  I'm not sure which method is better, but we clearly don't need 2 in the long run.

It's actually not an extra. We always made virtual calls before for setRenderStyle, now we only make them for the no-renderer case. So it's not any worse than before :)

[Elliott Sprehn]

Created attachment 170787 [details]
Patch

Fix comments from darin

[Elliott Sprehn]

Created attachment 170794 [details]
Patch

Clean up of NodeRenderingContext

[Build Bot]

Comment on attachment 170794 [details]
Patch

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

[Elliott Sprehn]

Created attachment 170813 [details]
Patch

If you stare at it long enough the simplest solution jumps out at you.

[Ojan Vafai]

Comment on attachment 170813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170813&action=review

This looks great. I'll leave it in the review queue for now so others have a chance to take a look in case I'm missing something. Otherwise, I'll r+ tomorrow.

> Source/WebCore/html/HTMLOptGroupElement.cpp:121
> +    updateNonRenderStyle();

This maybe deserve a comment explaining that we update the member variable here to avoid needing to have a virtual setRenderStyle called during recalcStyle. These two new instances are the only times we actually set m_style in customStyleForRenderer. I think this only works because these element types don't ever have renderers, right? Might be worth mentioning that in the comment as well.

> Source/WebCore/html/HTMLOptionElement.cpp:321
> +    updateNonRenderStyle();

Ditto

[Elliott Sprehn]

I ran the html5-full-render.html perf test and there's no change, so it's a good clean up but doesn't seem to buy us much in recalcStyle.

[Elliott Sprehn]

Created attachment 170948 [details]
Patch for landing

[Elliott Sprehn]

(In reply to comment #21)
> Created an attachment (id=170948) [details]
> Patch for landing

I added some comments explaining what's going on.

[Elliott Sprehn]

Created attachment 170976 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 170976 [details]
Patch for landing

Clearing flags on attachment: 170976

Committed r132684: <http://trac.webkit.org/changeset/132684>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

This broke the Apple Windows build. See <http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/56914/steps/compile-webkit/logs/stdio>.

[Elliott Sprehn]

(In reply to comment #26)
> This broke the Apple Windows build. See <http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/56914/steps/compile-webkit/logs/stdio>.

All those symbols are totally gone from the codebase so this is something weird on the bot. 

I created https://bugs.webkit.org/show_bug.cgi?id=100556 that touches all the files it references, hopefully that makes the bot fix itself.