Pass context to Node::attach and detach

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Elliott Sprehn]

Pass context to Node::attach and detach

[Elliott Sprehn]

Created attachment 194671 [details]
Patch

[Elliott Sprehn]

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.

[Eric Seidel (no email)]

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?

[Build Bot]

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

[Elliott Sprehn]

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

[Build Bot]

Comment on attachment 194671 [details]
Patch

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

[Build Bot]

Comment on attachment 194671 [details]
Patch

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

[Elliott Sprehn]

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.