Eliminate setRenderStyle, you really want setNonRendererRenderStyle
Created attachment 170692 [details] Patch
Comment on attachment 170692 [details] Patch Attachment 170692 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14563677
Comment on attachment 170692 [details] Patch Attachment 170692 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14543745
Created attachment 170697 [details] Patch Try again
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.
Comment on attachment 170697 [details] Patch Attachment 170697 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14563685
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.
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
These look like real failures.
Created attachment 170760 [details] Patch
(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. :)
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.
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.
(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 :)
Created attachment 170787 [details] Patch Fix comments from darin
Created attachment 170794 [details] Patch Clean up of NodeRenderingContext
Comment on attachment 170794 [details] Patch Attachment 170794 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14544876
Created attachment 170813 [details] Patch If you stare at it long enough the simplest solution jumps out at you.
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
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.
Created attachment 170948 [details] Patch for landing
(In reply to comment #21) > Created an attachment (id=170948) [details] > Patch for landing I added some comments explaining what's going on.
Created attachment 170976 [details] Patch for landing
Comment on attachment 170976 [details] Patch for landing Clearing flags on attachment: 170976 Committed r132684: <http://trac.webkit.org/changeset/132684>
All reviewed patches have been landed. Closing bug.
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>.
(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.