Bug 113123

Summary: Pass context to Node::attach and detach
Product: WebKit Reporter: Elliott Sprehn <esprehn>
Component: New BugsAssignee: Elliott Sprehn <esprehn>
Status: RESOLVED WONTFIX    
Severity: Normal CC: buildbot, cmarcelo, d-r, eric.carlson, eric, esprehn+autocc, feature-media-reviews, fmalita, japhet, jer.noble, koivisto, leviw, mifenton, ojan.autocc, ojan, pdr, philn, rniwa, schenney, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 111627    
Attachments:
Description Flags
Patch koivisto: review-, buildbot: commit-queue-

Elliott Sprehn
Reported 2013-03-22 17:53:08 PDT
Pass context to Node::attach and detach
Attachments
Patch (69.65 KB, patch)
2013-03-22 17:57 PDT, Elliott Sprehn 		koivisto: review-
buildbot: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Elliott Sprehn
Comment 1 2013-03-22 17:57:15 PDT
Created attachment 194671 [details] Patch
Elliott Sprehn
Comment 2 2013-03-22 17:59:02 PDT
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 3 2013-03-22 18:12:25 PDT
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 4 2013-03-22 19:00:40 PDT
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
Comment 5 2013-03-22 22:52:44 PDT
(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 6 2013-03-23 03:38:46 PDT
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 7 2013-03-23 09:47:21 PDT
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
Comment 8 2013-03-24 00:28:44 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.