[Webkit-unassigned] [Bug 149997] Implement iterator for traversing composed DOM

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=149997

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #262914|review?                     |review+
              Flags|                            |

--- Comment #12 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 262914
  --> https://bugs.webkit.org/attachment.cgi?id=262914
patch

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

> Source/WebCore/dom/ComposedTreeIterator.cpp:35
> +    auto* node = m_current;

Please add a change log note saying this code doesn't run in most cases.

> Source/WebCore/dom/ComposedTreeIterator.cpp:79
> +    if (is<HTMLSlotElement>(*m_current) && !m_shadowStack.last().currentSlot) {

shadowStack.last() appears many times in this function.
Can we store that in a local variable?

> Source/WebCore/dom/ComposedTreeIterator.cpp:83
> +            m_shadowStack.last().currentSlot = &slot;
> +            m_shadowStack.last().currentSlotNodeIndex = 0;

Should we add a helper function to set slot + offset?

> Source/WebCore/dom/ComposedTreeIterator.cpp:95
> +            if (++m_shadowStack.last().currentSlotNodeIndex < slot->assignedNodes()->size()) {

Can we add a descriptive local variable like:
bool nextNodeIsInSameSlot = ++m_shadowStack.last().currentSlotNodeIndex < slot->assignedNodes()->size()
?

> Source/WebCore/dom/ComposedTreeIterator.cpp:121
> +        ASSERT(m_shadowStack.last().host == downcast<ShadowRoot>(*parent).host());

Ditto about storing m_shadowStack.last() in a local variable.

> Source/WebCore/dom/ComposedTreeIterator.cpp:151
> +    ASSERT(m_shadowStack.last().host == m_current->parentNode());
> +    if (!m_shadowStack.last().currentSlot) {

Can we put a blank line after all these assertions?
It's hard to read it.
Also, can we store m_shadowStack.last() in a local variable?

> Source/WebCore/dom/ComposedTreeIterator.cpp:157
> +    if (++m_shadowStack.last().currentSlotNodeIndex < slotNodes->size())

Ditto about defining a descriptive local variable.

> Source/WebCore/dom/ComposedTreeIterator.cpp:168
> +    ASSERT(m_shadowStack.last().host == m_current->parentNode());
> +    if (!m_shadowStack.last().currentSlot) {

Ditto about adding a blank line here.

> Source/WebCore/dom/ComposedTreeIterator.cpp:175
> +    auto* slotNodes = m_shadowStack.last().currentSlot->assignedNodes();
> +    ASSERT(slotNodes->at(m_shadowStack.last().currentSlotNodeIndex) == m_current);
> +    if (m_shadowStack.last().currentSlotNodeIndex > 0)
> +        m_current = slotNodes->at(--m_shadowStack.last().currentSlotNodeIndex);

Same comments about defining a local variable for m_shadowStack.last()
and defining a descriptive local variable like previousNodeIsInSameSlot.

> Source/WebCore/dom/ComposedTreeIterator.h:75
> +        HTMLSlotElement* currentSlot { nullptr };
> +        unsigned currentSlotNodeIndex { 0 };

It seems useful to have a helper method that sets both of these variables.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151013/93703f19/attachment.html>