Bug 89732 – [Refactoring] NodeRenderingContext ctor could be built on top of the ComposedShadowTreeWalker

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 149264 [details]
Patch

[Dimitri Glazkov (Google)]

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?

[Hajime Morrita]

Created attachment 149432 [details]
Patch

[Hajime Morrita]

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.

[Build Bot]

Comment on attachment 149432 [details]
Patch

Attachment 149432 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13094326

[Hajime Morrita]

Created attachment 149439 [details]
Patch

[Dimitri Glazkov (Google)]

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.

[Hajime Morrita]

Created attachment 149442 [details]
Patch

[Hajime Morrita]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 149469 [details]
Patch

[Dimitri Glazkov (Google)]

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?

[Hajime Morrita]

OK, I'll give another try. Enjoy I/O :-)

[Hajime Morrita]

Created attachment 149673 [details]
Patch

[Hajime Morrita]

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

[Build Bot]

Comment on attachment 149673 [details]
Patch

Attachment 149673 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13102606

[Hajime Morrita]

Created attachment 149699 [details]
Patch

[Build Bot]

Comment on attachment 149699 [details]
Patch

Attachment 149699 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13087807

[Hajime Morrita]

Created attachment 149899 [details]
Patch

[Dimitri Glazkov (Google)]

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?

[Hayato Ito]

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.

[Hajime Morrita]

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

[Hajime Morrita]

Created attachment 150070 [details]
Patch for landing

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 150081 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 150081 [details]
Patch for landing

Clearing flags on attachment: 150081

Committed r121518: <http://trac.webkit.org/changeset/121518>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Mac build fix landed in http://trac.webkit.org/changeset/121522.