Bug 110146 – Rename AncestorChainWalker

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

AncestorChainWalker walker(node);
walker.parent();
return walker.get();

It's not obvious that the above code should behave differently from:

return node ? node->parentNode() : 0;

The class name should communicate the semantic difference.

[Ryosuke Niwa]

AncestorChainAcrossInsertionPointWalker?

[Ryosuke Niwa]

Also, m_isCrossingInsertionPoint should be named m_hasCrossedInsertionPoint and parent() should be named moveToParent().

[Mike West]

+Hayato-san, who will likely have opinions.

[Dimitri Glazkov (Google)]

EventPathWalker should be a good name.

[Ryosuke Niwa]

(In reply to comment #4)
> EventPathWalker should be a good name.

I like that!

[Hayato Ito]

(In reply to comment #4)
> EventPathWalker should be a good name.

+1. I've recently introduced 'EventPath' into EventContext in http://trac.webkit.org/changeset/142575.

We should spread this term, which is used in DOM Core spec, to Walker class too.
Hopefully, we can avoid a kind of confusion. This kind of confusion is apparently bad for the project.

AncestorChainWalker was formally called ComposedShadowTreeParentWalker, which was separated from ComposedShadowTreeWalker. I think there are some historical debts. We should clean up now.

[Darin Adler]

I think that for simple cases we need a function rather than a class so we don’t have use three lines for the idiom Ryosuke quoted.

[Hayato Ito]

Let me add one more history.
Before ComposedShadowTreeWalker was introduced, there was a 'Reified DOM Tree traversal'.
http://trac.webkit.org/changeset/112055

This is function style APIs. All public APIs are just static functions.

ComposedShadowTreeWalker was introduced, deprecating ReifiedTreeTraversal.
The reasons:
  - We need to parameterize Walker class. It might be good to have a parameter in its constructor.
  - I'd like to optimize the performance of walker without affecting the usage in caller side.

I think NodeTraversal, which was introduced recently, might be wrapper APIs for common use cases for walker.
If we can apply NodeTraversal, we should use it rather than using Walker directly.

[Ryosuke Niwa]

(In reply to comment #8)
> I think NodeTraversal, which was introduced recently, might be wrapper APIs for common use cases for walker.
> If we can apply NodeTraversal, we should use it rather than using Walker directly.

NodeTraversal isn't a class though...

[Hayato Ito]

I have to use yet another unfamiliar term, *reprojection*, to explain that. :)

Node can be distributed to more than one insertion point during reprojection.
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#reprojection

Therefore, the walker has to visit each insertion point consecutively since an event should be fired on these insertion points.

I am feeling that isCrossingInsertinPoint() is not good name, isVisitingInsertionPointInReprojection() might be a better name.


(In reply to comment #2)
> Also, m_isCrossingInsertionPoint should be named m_hasCrossedInsertionPoint and parent() should be named moveToParent().

[Hayato Ito]

Created attachment 189026 [details]
Renamed

[Hayato Ito]

Though it seems mac bot and win bot stopped working, let me set cq+ and see what will happen.

[WebKit Review Bot]

Comment on attachment 189026 [details]
Renamed

Clearing flags on attachment: 189026

Committed r143422: <http://trac.webkit.org/changeset/143422>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.