Created attachment 194671 [details] Patch

This needs a perf test (I have one, but it's not in run-perf-tests format), but I wanted to post the patch instead of having it just sitting on my machine.

Comment on attachment 194671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194671&action=review > Source/WebCore/dom/Element.cpp:-1259 > -void Element::createRendererIfNeeded() Nothing overrides this anymore? > Source/WebCore/dom/NodeRenderingContext.h:48 > + NodeRenderingContext(Node*, const AttachContext* = 0); isn't explicit still needed?

Comment on attachment 194671 [details] Patch Attachment 194671 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17239397

(In reply to comment #3) > (From update of attachment 194671 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194671&action=review > > > Source/WebCore/dom/Element.cpp:-1259 > > -void Element::createRendererIfNeeded() > > Nothing overrides this anymore? It's not virtual, so there were never any overrides. You're thinking of Element::createRenderer. Element::createRendererIfNeeded and Text::createTextRendererIfNeeded were both single line, non virtual methods that had only one caller (or two for Text) and delegated immediately to NodeRenderingContext. I inlined the call sites to just use NodeRenderingContext().createRendererForElementIfNeeded (or createRendererForTextIfNeeded) which makes the code more clear and lets you pass the AttachContext inside Element::attach. > > > Source/WebCore/dom/NodeRenderingContext.h:48 > > + NodeRenderingContext(Node*, const AttachContext* = 0); > > isn't explicit still needed? Yeah it is, I'll add it back.

Comment on attachment 194671 [details] Patch Attachment 194671 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17181917

Comment on attachment 194671 [details] Patch Attachment 194671 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17190964

I've been thinking about this more and with a bitfield for lazy attach, or making lazy attach not lie about being attached, we could get the same performance improvement by avoiding n^2 behavior. Making lazyAttach not setAttached would be hard in the short term, so I think we should go with the bitfield. We should still pass the style down through reattach though in recalcStyle. @eric: How do you feel about using a bitfield instead? If we do that do you want to keep the AttachContext? Passing just the RenderStyle would make this patch simpler.