Bug 87004 – [Meta] Refactor ComposedShadowTreeWalker

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

[Hayato Ito]

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.

[Hayato Ito]

Created attachment 144305 [details]
refactor a walker

[Dimitri Glazkov (Google)]

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"?

[Hayato Ito]

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.

[Hayato Ito]

Created attachment 144684 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 144684 [details]
Patch for landing

Clearing flags on attachment: 144684

Committed r118890: <http://trac.webkit.org/changeset/118890>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.