Bug 154003 – Implement ComposedTreeIterator in terms of ElementAndTextDescendantIterator

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

[Antti Koivisto]

Make it more hackable and correct.

[Antti Koivisto]

Created attachment 270877 [details]
patch

[Antti Koivisto]

Created attachment 270878 [details]
patch

[Build Bot]

Comment on attachment 270878 [details]
patch

Attachment 270878 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/801721

New failing tests:
fast/shadow-dom/event-inside-slotted-node.html
fast/shadow-dom/shadow-layout-after-attach-shadow.html

[Build Bot]

Created attachment 270883 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 270878 [details]
patch

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

r=me assuming you find and fix the assertion failure (or maybe just a crash) that the mac-debug bot is showing.

> Source/WebCore/dom/ComposedTreeIterator.h:161
> +        Iterator()
> +            : ComposedTreeIterator()
> +        { }

I think you can write:

    Iterator() = default;

Which I think is better.

> Source/WebCore/dom/ComposedTreeIterator.h:162
>          Iterator(ContainerNode& root)

explicit?

> Source/WebCore/dom/ComposedTreeIterator.h:177
> +    Iterator begin() { return Iterator(m_parent); }

Since the constructor is not explicit, could just write:

    return m_parent;

> Source/WebCore/dom/ComposedTreeIterator.h:178
> +    Iterator end() { return Iterator(); }

I slightly prefer:

    return { };

> Source/WebCore/dom/ElementAndTextDescendantIterator.h:36
> +class ElementAndTextDescendantIterator {

This class has a lot of code. Any way for it to share more with other classes?

> Source/WebCore/dom/ElementAndTextDescendantIterator.h:38
> +    ElementAndTextDescendantIterator();

I prefer = default.

> Source/WebCore/dom/ElementAndTextDescendantIterator.h:39
> +    ElementAndTextDescendantIterator(ContainerNode& root);

explicit?

> Source/WebCore/dom/ElementAndTextDescendantIterator.h:52
> +    bool operator!() const { return !m_current; }

I normally expect an explicit operator bool any place I see an operator!

> Source/WebCore/dom/ElementAndTextDescendantIterator.h:86
> +    ElementAndTextDescendantIteratorAdapter(ContainerNode& root);

explicit?

> Source/WebCore/dom/ElementAndTextDescendantIterator.h:99
> +    : m_current(nullptr)

Why not initialize this in the class definition?

[Antti Koivisto]

https://trac.webkit.org/r196281 (with a bug fix)

[Antti Koivisto]

> This class has a lot of code. Any way for it to share more with other
> classes?

Yes, this is very similar to ElementDescendantIterator. We could probably share code by making a template class.