Bug 82009 - [Shadow DOM] Introduce ComposedShadowTreeWalker as a successor of ReifiedTreeTraversal APIs
Summary: [Shadow DOM] Introduce ComposedShadowTreeWalker as a successor of ReifiedTree...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks: 78585 82694 82696 82702
  Show dependency treegraph
 
Reported: 2012-03-22 20:55 PDT by Hayato Ito
Modified: 2012-04-02 02:55 PDT (History)
10 users (show)

See Also:


Attachments
WIP. concept of walker. Not tested yet. (31.37 KB, patch)
2012-03-23 04:38 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
let me knwo required symbols on windows (43.98 KB, patch)
2012-03-28 04:51 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
give me necesarry symbols (45.84 KB, patch)
2012-03-29 22:21 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
add symbols (45.93 KB, patch)
2012-03-29 23:59 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
ready for review (46.03 KB, patch)
2012-03-30 00:40 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
try builds (45.55 KB, patch)
2012-04-01 22:43 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (45.62 KB, patch)
2012-04-02 01:35 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2012-03-22 20:55:16 PDT
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.
Comment 1 Hayato Ito 2012-03-23 04:38:24 PDT
Created attachment 133460 [details]
WIP. concept of walker. Not tested yet.
Comment 2 Hayato Ito 2012-03-28 04:51:09 PDT
Created attachment 134267 [details]
let me knwo required symbols on windows
Comment 3 Early Warning System Bot 2012-03-28 05:08:49 PDT
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
Comment 4 Build Bot 2012-03-28 05:09:43 PDT
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
Comment 5 Early Warning System Bot 2012-03-28 05:11:04 PDT
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
Comment 6 WebKit Review Bot 2012-03-28 05:13:39 PDT
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
Comment 7 Gyuyoung Kim 2012-03-28 05:13:47 PDT
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
Comment 8 Philippe Normand 2012-03-28 05:16:27 PDT
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
Comment 9 Build Bot 2012-03-28 05:18:24 PDT
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
Comment 10 Dimitri Glazkov (Google) 2012-03-28 09:13:42 PDT
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.
Comment 11 Hayato Ito 2012-03-28 19:14:29 PDT
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.
Comment 12 Hayato Ito 2012-03-29 22:21:27 PDT
Created attachment 134725 [details]
give me necesarry symbols
Comment 13 Philippe Normand 2012-03-29 23:27:00 PDT
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
Comment 14 Build Bot 2012-03-29 23:32:55 PDT
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
Comment 15 Hayato Ito 2012-03-29 23:59:49 PDT
Created attachment 134735 [details]
add symbols
Comment 16 Hayato Ito 2012-03-30 00:40:47 PDT
Created attachment 134739 [details]
ready for review
Comment 17 Dimitri Glazkov (Google) 2012-03-30 09:11:18 PDT
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.
Comment 18 Hayato Ito 2012-04-01 19:17:04 PDT
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.
Comment 19 Hayato Ito 2012-04-01 22:43:13 PDT
Created attachment 135026 [details]
try builds
Comment 20 Hayato Ito 2012-04-02 01:35:53 PDT
Created attachment 135042 [details]
Patch for landing
Comment 21 WebKit Review Bot 2012-04-02 02:55:14 PDT
Comment on attachment 135042 [details]
Patch for landing

Clearing flags on attachment: 135042

Committed r112845: <http://trac.webkit.org/changeset/112845>
Comment 22 WebKit Review Bot 2012-04-02 02:55:22 PDT
All reviewed patches have been landed.  Closing bug.