Bug 87004 - [Meta] Refactor ComposedShadowTreeWalker
Summary: [Meta] Refactor ComposedShadowTreeWalker
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:
 
Reported: 2012-05-21 04:43 PDT by Hayato Ito
Modified: 2012-05-29 21:20 PDT (History)
6 users (show)

See Also:


Attachments
refactor a walker (9.29 KB, patch)
2012-05-28 01:48 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (9.29 KB, patch)
2012-05-29 19:53 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-05-21 04:43:35 PDT
There is a design issue of current ComposedShadowTreeWalker:

- parentIncludingInsertionPointAndShadowRoot() does not use Policy.
- Nothing prevents users from mixing parentIncludingInsertionPointAndShadowRoot() and other functions.

It might be better that we have:
 - A separated Walker class which provides only parentIncludingInsertionPointAndShadowRoot() and get rid of the function from current Walker class.
 - or yet another traversing Policy used for parentIncludingInsertionPointAndShadowRoot().
 - or making parentIncludingInsertionPointAndShadowRoot() a static function.
Comment 1 Hayato Ito 2012-05-28 01:48:13 PDT
Created attachment 144305 [details]
refactor a walker
Comment 2 Dimitri Glazkov (Google) 2012-05-29 09:42:45 PDT
Comment on attachment 144305 [details]
refactor a walker

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

> Source/WebCore/ChangeLog:3
> +        Introcues ComposedShadowTreeParentWalker, extracted from ComposedShadowTreeWalker.

Introcues -> Introduces

> Source/WebCore/dom/ComposedShadowTreeWalker.h:124
> +    void parentIncludingInsertionPointAndShadowRoot();

If this is a specialized walker, why not just call this "parent"?
Comment 3 Hayato Ito 2012-05-29 19:26:56 PDT
Thank you for the review.

(In reply to comment #2)
> (From update of attachment 144305 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144305&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Introcues ComposedShadowTreeParentWalker, extracted from ComposedShadowTreeWalker.
> 
> Introcues -> Introduces

My bad. Let me fix.

> 
> > Source/WebCore/dom/ComposedShadowTreeWalker.h:124
> > +    void parentIncludingInsertionPointAndShadowRoot();
> 
> If this is a specialized walker, why not just call this "parent"?

I agree that this is long and ugly function name, but I'd like to leave words of 'IncludingInsertionPointAndShadowRoot' somewhere to make its behavior clear and remind caller of the specialness of the function.

Another idea is to change class name, say ComposedShadowTreeParentXXXXInserionPointAndShadowRootXXXWalker' (I cannot have a good name :( ) and call the function 'parent()' since the class has only one traversal member function. But I'd like to avoid such a long class name.

So let me land this as is. I am okay to rename the function name if we have better one.
Comment 4 Hayato Ito 2012-05-29 19:53:06 PDT
Created attachment 144684 [details]
Patch for landing
Comment 5 WebKit Review Bot 2012-05-29 21:20:19 PDT
Comment on attachment 144684 [details]
Patch for landing

Clearing flags on attachment: 144684

Committed r118890: <http://trac.webkit.org/changeset/118890>
Comment 6 WebKit Review Bot 2012-05-29 21:20:30 PDT
All reviewed patches have been landed.  Closing bug.