NodeRenderingContext is custom-made traverser for composed tree. In theory, it could be built on top of ComposedShadowTreeWalker. We might need to touch ComposedShadowTreeWalker to unveil its internal for performance reason.
Created attachment 149264 [details] Patch
Comment on attachment 149264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149264&action=review I really like the approach. A few questions/nits: > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:73 > +ComposedShadowTreeWalker::ComposedShadowTreeWalker(const Node* node, SearchParentTag) Why do you need this? Isn't ComposedShadowTreeWalker(node) already instantiated with Policy = CrossUpperBoundary? > Source/WebCore/dom/ComposedShadowTreeWalker.h:95 > + ComposedShadowTreeParent findParent() const; Could we possibly just return NodeRenderingContext here?
Created attachment 149432 [details] Patch
Hi Dimitri, thanks for taking look in busy days! > > I really like the approach. A few questions/nits: > > > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:73 > > +ComposedShadowTreeWalker::ComposedShadowTreeWalker(const Node* node, SearchParentTag) > > Why do you need this? Isn't ComposedShadowTreeWalker(node) already instantiated with Policy = CrossUpperBoundary? Originally this was to avoid assretPrecodntion(). But updated patch simplified around this. > > > Source/WebCore/dom/ComposedShadowTreeWalker.h:95 > > + ComposedShadowTreeParent findParent() const; > > Could we possibly just return NodeRenderingContext here? I don't want to introduce such dependency. But I agree that manually copying member variables looks ugly. Updated patch extracted common part to a new class.
Comment on attachment 149432 [details] Patch Attachment 149432 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13094326
Created attachment 149439 [details] Patch
Comment on attachment 149439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149439&action=review > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:51 > +ComposedShadowTreeWalker::ComposedShadowTreeWalker(const Node* node, Policy policy, ComposedShadowTreeParent* searchingParent) I think this is worse. The magic parameter for a treewalker seems like a bad design. It's not intuitive at first glance on what it's for.
Created attachment 149442 [details] Patch
(In reply to comment #7) > (From update of attachment 149439 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149439&action=review > > > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:51 > > +ComposedShadowTreeWalker::ComposedShadowTreeWalker(const Node* node, Policy policy, ComposedShadowTreeParent* searchingParent) > > I think this is worse. The magic parameter for a treewalker seems like a bad design. It's not intuitive at first glance on what it's for. Makes sense. I added a private constructor separately.
Comment on attachment 149442 [details] Patch Attachment 149442 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13093349 New failing tests: fullscreen/full-screen-render-inline.html
Created attachment 149445 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 149469 [details] Patch
Comment on attachment 149469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149469&action=review I have a few thoughts -- but no definitive idea on how to make this better. Sorry :) > Source/WebCore/dom/ComposedShadowTreeParent.h:27 > +#ifndef ComposedShadowTreeParent_h Why does it need to be a separate class? It really has no purpose outside of ComposedShadowTreeWalker. Can it just be a helper nested class? Also, it is not clear why it's called "parent", other than because it's only used by the findParent. It smells more like a composition context of some sort. > Source/WebCore/dom/ComposedShadowTreeParent.h:48 > + bool outOfComposition() const { return m_outOfComposition; } fallbackContent? > Source/WebCore/dom/ComposedShadowTreeParent.h:53 > + void childWasOutOfComposition() { m_outOfComposition = true; } can we just say setIsFallbackContent? > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:65 > + , m_searchingParent(searchingParent) searchingParent as a name needs more clarity. The constructor argument seems simply an client-like parameter. You are adjusting the state of the client when traversing, right? It has nothing to do with "parent" per se. > Source/WebCore/dom/NodeRenderingContext.cpp:57 > + , m_parent(ComposedShadowTreeWalker::findParent(m_node)) It seems that you're doing a bit more work here in this line for a non-shadow DOM tree cases, compared to the early return before. Are there any perf implications?
OK, I'll give another try. Enjoy I/O :-)
Created attachment 149673 [details] Patch
(In reply to comment #13) > (From update of attachment 149469 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149469&action=review > > I have a few thoughts -- but no definitive idea on how to make this better. Sorry :) Me either. But updated the patch anyway :-) > > > Source/WebCore/dom/ComposedShadowTreeParent.h:27 > > +#ifndef ComposedShadowTreeParent_h > > Why does it need to be a separate class? It really has no purpose outside of ComposedShadowTreeWalker. > > Can it just be a helper nested class? Original intention was not to include ComposedShadowTreeWalker.h from NodeRenderingContext.h but seems over-reaction. Moved it to a nested class renaming to ParentTraversalDetails. > > Also, it is not clear why it's called "parent", other than because it's only used by the findParent. > > It smells more like a composition context of some sort. > I felt so at first. But it's not such a generic class and cares only for traversal from child to parent. Putting it to a member function might've gave that impression. I moved it to a parameter to traversal methods. > > > Source/WebCore/dom/ComposedShadowTreeParent.h:48 > > + bool outOfComposition() const { return m_outOfComposition; } > > fallbackContent? It's not only for fallback content. It also can be a light children of host node, shadow-root children which isn't composed for rendering. > > > Source/WebCore/dom/ComposedShadowTreeParent.h:53 > > + void childWasOutOfComposition() { m_outOfComposition = true; } > > can we just say setIsFallbackContent? > > > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:65 > > + , m_searchingParent(searchingParent) > > searchingParent as a name needs more clarity. Agreed. Got rid of this. > > The constructor argument seems simply an client-like parameter. You are adjusting the state of the client when traversing, right? > > It has nothing to do with "parent" per se. > > > Source/WebCore/dom/NodeRenderingContext.cpp:57 > > + , m_parent(ComposedShadowTreeWalker::findParent(m_node)) > > It seems that you're doing a bit more work here in this line for a non-shadow DOM tree cases, compared to the early return before. Are there any perf implications? Right. And the original patch actually regressed a bit. I eliminated the return value and inlined some method. Now there is no perf regression. (I used Dromaeo/dom-modify.html for this benchmark.)
Comment on attachment 149673 [details] Patch Attachment 149673 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13102606
Created attachment 149699 [details] Patch
Comment on attachment 149699 [details] Patch Attachment 149699 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13087807
Created attachment 149899 [details] Patch
Comment on attachment 149899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149899&action=review > Source/WebCore/dom/NodeRenderingContext.h:73 > + ComposedShadowTreeWalker::ParentTranversalDetails m_parent; In this neck of the woods, m_parent usually means a ContainerNode*. Can we name this more precisely? m_parentTraversalDetails is a bit too verbose, but maybe m_parentDetails?
Can someone review bug 89177 and merge it to this patch before landing? https://bugs.webkit.org/show_bug.cgi?id=89177 Without 89177, the redering might break.
(In reply to comment #21) > In this neck of the woods, m_parent usually means a ContainerNode*. Can we name this more precisely? m_parentTraversalDetails is a bit too verbose, but maybe m_parentDetails? Agreed. I'll rename before landing. Thanks for the review, Dimitri!
Created attachment 150070 [details] Patch for landing
Comment on attachment 150070 [details] Patch for landing Rejecting attachment 150070 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/13111244
Created attachment 150081 [details] Patch for landing
Comment on attachment 150081 [details] Patch for landing Clearing flags on attachment: 150081 Committed r121518: <http://trac.webkit.org/changeset/121518>
All reviewed patches have been landed. Closing bug.
Mac build fix landed in http://trac.webkit.org/changeset/121522.