Bug 89732

Summary: [Refactoring] NodeRenderingContext ctor could be built on top of the ComposedShadowTreeWalker
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, hayato, rakuco, rniwa, tasak, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 89177    
Bug Blocks: 72352    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-02
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Hajime Morrita 2012-06-21 20:48:27 PDT
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.
Comment 1 Hajime Morrita 2012-06-25 02:24:38 PDT
Created attachment 149264 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2012-06-25 08:59:16 PDT
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?
Comment 3 Hajime Morrita 2012-06-25 19:25:25 PDT
Created attachment 149432 [details]
Patch
Comment 4 Hajime Morrita 2012-06-25 19:28:04 PDT
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 5 Build Bot 2012-06-25 19:59:35 PDT
Comment on attachment 149432 [details]
Patch

Attachment 149432 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13094326
Comment 6 Hajime Morrita 2012-06-25 20:15:04 PDT
Created attachment 149439 [details]
Patch
Comment 7 Dimitri Glazkov (Google) 2012-06-25 20:16:35 PDT
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.
Comment 8 Hajime Morrita 2012-06-25 20:58:24 PDT
Created attachment 149442 [details]
Patch
Comment 9 Hajime Morrita 2012-06-25 20:58:56 PDT
(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 10 WebKit Review Bot 2012-06-25 21:26:19 PDT
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
Comment 11 WebKit Review Bot 2012-06-25 21:26:23 PDT
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
Comment 12 Hajime Morrita 2012-06-26 00:23:48 PDT
Created attachment 149469 [details]
Patch
Comment 13 Dimitri Glazkov (Google) 2012-06-26 13:58:54 PDT
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?
Comment 14 Hajime Morrita 2012-06-26 17:47:04 PDT
OK, I'll give another try. Enjoy I/O :-)
Comment 15 Hajime Morrita 2012-06-26 20:46:32 PDT
Created attachment 149673 [details]
Patch
Comment 16 Hajime Morrita 2012-06-26 20:52:18 PDT
(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 17 Build Bot 2012-06-26 21:07:01 PDT
Comment on attachment 149673 [details]
Patch

Attachment 149673 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13102606
Comment 18 Hajime Morrita 2012-06-27 00:53:00 PDT
Created attachment 149699 [details]
Patch
Comment 19 Build Bot 2012-06-27 01:42:49 PDT
Comment on attachment 149699 [details]
Patch

Attachment 149699 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13087807
Comment 20 Hajime Morrita 2012-06-28 01:31:25 PDT
Created attachment 149899 [details]
Patch
Comment 21 Dimitri Glazkov (Google) 2012-06-28 08:50:37 PDT
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?
Comment 22 Hayato Ito 2012-06-28 15:34:11 PDT
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.
Comment 23 Hajime Morrita 2012-06-28 19:21:24 PDT
(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!
Comment 24 Hajime Morrita 2012-06-28 20:07:09 PDT
Created attachment 150070 [details]
Patch for landing
Comment 25 WebKit Review Bot 2012-06-28 20:09:20 PDT
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
Comment 26 Hajime Morrita 2012-06-28 21:44:11 PDT
Created attachment 150081 [details]
Patch for landing
Comment 27 WebKit Review Bot 2012-06-28 23:01:13 PDT
Comment on attachment 150081 [details]
Patch for landing

Clearing flags on attachment: 150081

Committed r121518: <http://trac.webkit.org/changeset/121518>
Comment 28 WebKit Review Bot 2012-06-28 23:01:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Ryosuke Niwa 2012-06-28 23:55:00 PDT
Mac build fix landed in http://trac.webkit.org/changeset/121522.