Bug 82009 – [Shadow DOM] Introduce ComposedShadowTreeWalker as a successor of ReifiedTreeTraversal APIs

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

[Hayato Ito]

See the comment on https://bugs.webkit.org/show_bug.cgi?id=79197#c35.

It might be better to rewrite Reified tree traversal APIs using cursor pattern instead of stateless static function pattern.
Let me implement a cursor pattern version here and compare both versions.

[Hayato Ito]

Created attachment 133460 [details]
WIP. concept of walker. Not tested yet.

[Hayato Ito]

Created attachment 134267 [details]
let me knwo required symbols on windows

[Early Warning System Bot]

Comment on attachment 134267 [details]
let me knwo required symbols on windows

Attachment 134267 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12147000

[Build Bot]

Comment on attachment 134267 [details]
let me knwo required symbols on windows

Attachment 134267 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12146999

[Early Warning System Bot]

Comment on attachment 134267 [details]
let me knwo required symbols on windows

Attachment 134267 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12155095

[WebKit Review Bot]

Comment on attachment 134267 [details]
let me knwo required symbols on windows

Attachment 134267 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12154103

[Gyuyoung Kim]

Comment on attachment 134267 [details]
let me knwo required symbols on windows

Attachment 134267 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12155096

[Philippe Normand]

Comment on attachment 134267 [details]
let me knwo required symbols on windows

Attachment 134267 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12153144

[Build Bot]

Comment on attachment 134267 [details]
let me knwo required symbols on windows

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

[Dimitri Glazkov (Google)]

Comment on attachment 134267 [details]
let me knwo required symbols on windows

While you're still working on this, I have another bikeshedding suggestion for the name. We need to mention "Shadow DOM" here somewhere. Soo... I thought about this and have this new name... Please don't shoot me! ... It's "ComposedShadowTreeWalker".  What do you think? Please feel free to reject the suggestion.

[Hayato Ito]

lol. I don't have a strong opinion. Either is okay.
ComposedShadowTreeWalker is long, but more intuitive.
In general, I prefer a long intuitive name to a short non-intuitive name. So let's use ComposedShadowTreeWalker from now if no one objects to it. It's not too late.

(In reply to comment #10)
> (From update of attachment 134267 [details])
> While you're still working on this, I have another bikeshedding suggestion for the name. We need to mention "Shadow DOM" here somewhere. Soo... I thought about this and have this new name... Please don't shoot me! ... It's "ComposedShadowTreeWalker".  What do you think? Please feel free to reject the suggestion.

[Hayato Ito]

Created attachment 134725 [details]
give me necesarry symbols

[Philippe Normand]

Comment on attachment 134725 [details]
give me necesarry symbols

Attachment 134725 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12288056

[Build Bot]

Comment on attachment 134725 [details]
give me necesarry symbols

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

[Hayato Ito]

Created attachment 134735 [details]
add symbols

[Hayato Ito]

Created attachment 134739 [details]
ready for review

[Dimitri Glazkov (Google)]

Comment on attachment 134739 [details]
ready for review

View in context: https://bugs.webkit.org/attachment.cgi?id=134739&action=review

This is sooo much more elegant. Thank you for doing this! A few tweaks before you land:

> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:45
> +#ifndef NDEBUG
> +#define ASSERT_PRECONDITION() assertPrecondition()
> +#else
> +#define ASSERT_PRECONDITION()
> +#endif
> +
> +#ifndef NDEBUG
> +#define ASSERT_POSTCONDITION() assertPostcondition()
> +#else
> +#define ASSERT_POSTCONDITION()
> +#endif

The WebKit typical approach here is to declare these as empty inline-in-header functions for #ifdef NDEBUG.

> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:77
> +ComposedShadowTreeWalker ComposedShadowTreeWalker::startedWithFirstChild(const Node* node, Policy policy)

I think just fromFirstChild is nicer.

> Source/WebCore/dom/ComposedShadowTreeWalker.h:61
> +    void nextNode();
> +    void previousNode();

These can just be next and previous.

[Hayato Ito]

Thank you for the review.
Let me land this patch after I address your comments.

(In reply to comment #17)
> (From update of attachment 134739 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134739&action=review
> 
> This is sooo much more elegant. Thank you for doing this! A few tweaks before you land:
> 
> > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:45
> > +#ifndef NDEBUG
> > +#define ASSERT_PRECONDITION() assertPrecondition()
> > +#else
> > +#define ASSERT_PRECONDITION()
> > +#endif
> > +
> > +#ifndef NDEBUG
> > +#define ASSERT_POSTCONDITION() assertPostcondition()
> > +#else
> > +#define ASSERT_POSTCONDITION()
> > +#endif
> 
> The WebKit typical approach here is to declare these as empty inline-in-header functions for #ifdef NDEBUG.

Okay. I'll move these to headers.

> 
> > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:77
> > +ComposedShadowTreeWalker ComposedShadowTreeWalker::startedWithFirstChild(const Node* node, Policy policy)
> 
> I think just fromFirstChild is nicer.

Thats' nice.

> 
> > Source/WebCore/dom/ComposedShadowTreeWalker.h:61
> > +    void nextNode();
> > +    void previousNode();
> 
> These can just be next and previous.

Okay. Let me do it. I'll rename parentNode() to parent() also.

[Hayato Ito]

Created attachment 135026 [details]
try builds

[Hayato Ito]

Created attachment 135042 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 135042 [details]
Patch for landing

Clearing flags on attachment: 135042

Committed r112845: <http://trac.webkit.org/changeset/112845>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.