[webkit-dev] AncestorChainWalker substance and style thoughts

[Antti Koivisto]

The purpose of the NodeTraversal refactoring was simply to move the
existing traversal functions off the (rather bloated) Node and add Element
specific functions. I think longer term iterators are the way to go as they
allow decoupling our internal document tree data structures from the DOM
API.

The problem with ComposedShadowTreeWalker is not that it is an iterator but
that it is ill-defined and complex. It calls back to non-trivial, mutating
functions on the structure being traversed (ContentDistributor::distribute!).
Simply traversing a tree with it can apparently end up marking the tree for
style recalc.

I don't think adding NodeRenderingTraversal on top of this helped much
though it did clarify the case where ComposedShadowTreeWalker is actually
needed.


  antti

On Tue, Feb 19, 2013 at 4:18 AM, Hajime Morrita <morrita at chromium.org>wrote:

> AncestorChainWalker follows the style of ComposedShadowTreeWalker,
> which also breaks conventions like mutations-should-be-verbs. We also fix
> its style for integrity.
>
> Non-trivial tree traversal API in general should, IMO, be modeled after
> recently introduced NodeTraversal.
> Having separate namespaces allows us to keep Node API minimal. Also it is
> stateless and simple.
>
> I made NodeRenderingTraversal in that way.
> AncestorChainWalker could be just merged to NodeRenderingTraversal.
>
> Speaking of ComposedShadowTreeWalker, I think it is worth having a class
> instead of namespace
> since it holds configuration parameters to decide, for example, whether it
> skips specific types of node.
>
>
>
> On Tue, Feb 19, 2013 at 10:37 AM, Darin Adler <darin at apple.com> wrote:
>
>> I am trying to learn about code related to shadow DOM that needs to be
>> understood by anyone working on an part of the DOM.
>>
>> I have some questions and concerns about AncestorChainWalker. If there is
>> a document I should be reading that covers these, please point me to it.
>>
>> The parentNode and parentElement functions are just plain old functions
>> that are used to walk the ancestor chain of the normal DOM. But the shadow
>> DOM AncestorChainWalker is a class. Why? It seems that it could just be a
>> function with two return values. Is it more efficient to have it be a
>> class? Can the operation be correctly done without storing
>> m_distributedNode in a data member?
>>
>> There is a function in AncestorChainWalker named parent. That name is a
>> noun, so the function should be a const function that returns a value.
>> Since it’s not, the function name should be a verb phrase, such as
>> advanceToParent, or event just “advance” since it’s in the context of an
>> ancestor chain walker.
>>
>> The function crossingInsertionPoint should be named
>> isCrossingInsertionPoint as the data member is. But also, since the walker
>> sits still on a single node, I don’t think it makes sense to talk about the
>> position as “is crossing”. It should be “just crossed” or something like
>> that instead. Unless an insertion point is like a bridge and is not itself
>> a “true node”.
>>
>> The function that returns the current node in the ancestor chain is named
>> “get”. That’s not a good name, and should be avoided if possible. It could
>> be named “node” or “currentNode” instead.
>>
>> -- Darin
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20130219/27b564b9/attachment.html>