Bug 82009

Summary:

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

Product:

WebKit

Reporter:

Hayato Ito <hayato>

Component:

DOM

Assignee:

Hayato Ito <hayato>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, dominicc, gustavo, morrita, pnormand, rakuco, rolandsteiner, shinyak, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

78585, 82694, 82696, 82702

Attachments:

Description	Flags
WIP. concept of walker. Not tested yet.	none
let me knwo required symbols on windows	none
give me necesarry symbols	none
add symbols	none
ready for review	none
try builds	none
Patch for landing	none

Hayato Ito

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

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Hayato Ito

Comment 1 2012-03-23 04:38:24 PDT

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

Hayato Ito

Comment 2 2012-03-28 04:51:09 PDT

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

Early Warning System Bot

Comment 3 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

Build Bot

Comment 4 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

Early Warning System Bot

Comment 5 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

WebKit Review Bot

Comment 6 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

Gyuyoung Kim

Comment 7 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

Philippe Normand

Comment 8 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

Build Bot

Comment 9 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

Dimitri Glazkov (Google)

Comment 10 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.

Hayato Ito

Comment 11 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.

Hayato Ito

Comment 12 2012-03-29 22:21:27 PDT

Created attachment 134725 [details] give me necesarry symbols

Philippe Normand

Comment 13 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

Build Bot

Comment 14 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

Hayato Ito

Comment 15 2012-03-29 23:59:49 PDT

Created attachment 134735 [details] add symbols

Hayato Ito

Comment 16 2012-03-30 00:40:47 PDT

Created attachment 134739 [details] ready for review

Dimitri Glazkov (Google)

Comment 17 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.

Hayato Ito

Comment 18 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.

Hayato Ito

Comment 19 2012-04-01 22:43:13 PDT

Created attachment 135026 [details] try builds

Hayato Ito

Comment 20 2012-04-02 01:35:53 PDT

Created attachment 135042 [details] Patch for landing

WebKit Review Bot

Comment 21 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>

WebKit Review Bot

Comment 22 2012-04-02 02:55:22 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.