| Summary: | The new tree builder needs to call attach() on elements it attaches to the DOM | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, eric, ggaren, hyatt, jamesr | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Other | ||||||
| OS: | OS X 10.5 | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 41123 | ||||||
| Attachments: |
|
||||||
|
Description
Adam Barth
2010-06-28 12:27:39 PDT
Created attachment 59923 [details]
Patch
Comment on attachment 59923 [details]
Patch
Seems like some of this basic stuff we could steal from the odl tree builder.
Comment on attachment 59923 [details]
Patch
We'd have to refactor the old tree builder. This stuff is in big monolithic functions with manual ref counting...
It would really be more efficient to build the whole tree and then attach() it from its root. That's a pretty big refactoring task, but if you're rewriting tree building anyway, maybe it's natural to make that change now. Oh, we can do that. You just call attach once at the root and it does all the children for you? (In reply to comment #3) > (From update of attachment 59923 [details]) > We'd have to refactor the old tree builder. This stuff is in big monolithic functions with manual ref counting... Totally doable. I wonder how many assumptions in other parts of WebCore that could break. Comment on attachment 59923 [details] Patch Clearing flags on attachment: 59923 Committed r62028: <http://trac.webkit.org/changeset/62028> All reviewed patches have been landed. Closing bug. (In reply to comment #5) > Oh, we can do that. You just call attach once at the root and it does all the children for you? You got it. Of course, things are never that easy :). You'll have to get clarity on how long you can validly delay attaching renderers for a subtree. For example, if you're about to insert a script into the document, you might have to flush any pending DOM nodes, so that rendering-dependent JavaScript properties report correct values. Maybe that's the only tricky case. (In reply to comment #4) > It would really be more efficient to build the whole tree and then attach() it from its root. I believe you. But could you clarify what aspects would make it more efficient? Would that cut out layouts/style resolves? Thank you for bringing this up Geoff. This is exactly the time to do so! It would be super helpful if the attach() call could be delayed as long as possible. One thing this would allow would be to delay calculating styles and updating the style selector. Currently we synchronously re resolve styles on the entire DOM whenever a <style> tag is parsed out, which kind of sucks (see http://code.google.com/p/chromium/issues/detail?id=35471 and https://bugs.webkit.org/show_bug.cgi?id=36303). I've been doing some work to attach() lazily in more cases, but it's hard to make it work perfectly. The tricky bit is that attach() has side effects for some elements. Plugins trigger their resource loading and initialization off the render tree currently so you need create the renderer (via attach()ing) shortly after parsing out the plugin. At a minimum you need to do this before executing any script to avoid observable ordering issues, and preferably do the attach() before yielding so that the plugin resource loads can kick off as soon as possible. Queries from javascript will force any elements in the DOM to get attach()d anyway because they force a style resolution + layout pass before returning and recalcStyle() attaches any unattached nodes. Also, the old tree builder's residual style fixup code makes some deep assumptions about how attach() works and how it's related to addChild()/etc, presumably as a performance optimization (to avoid doing unnecessary work). That bit of code will probably need careful attention. (In reply to comment #10) > (In reply to comment #4) > > It would really be more efficient to build the whole tree and then attach() it from its root. > > I believe you. But could you clarify what aspects would make it more efficient? Would that cut out layouts/style resolves? First, I should take back my statement that you might need to attach everything before running scripts. Our design is that any JavaScript accessors that depend on layout should update layout if needed before doing their thing. Any failures in this area are the fault of the accessor. So, in theory, you should be able to delay attaching until the layout timer fires and you actually display the page. Bearing that in mind, here are two cases that would definitely get faster if the parser attached elements lazily (or not at all, and just relied on the layout timer): 1. An element is added to the page but, before it displays, a script removes it from the page. (More common than you might think.) 2. An element is added to the page but, before it displays, another element is added, or a new stylesheet is loaded, changing the first element's layout or rendering. In both of these cases, the eager attach done by the parser is just thrown away. Can one of you expert-types file a new bug about this and marking it as blocking 41123? We have a good handle on when scripts execute in the new design. I'd rather not do this until we're able to actually run the LayoutTests so we can get some sort of handle on what we're breaking. HTML5 TreeBuilder should attach whole subtrees instead of individual nodes https://bugs.webkit.org/show_bug.cgi?id=41361 |