Bug 154003 - Implement ComposedTreeIterator in terms of ElementAndTextDescendantIterator
Summary: Implement ComposedTreeIterator in terms of ElementAndTextDescendantIterator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 154035
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-08 13:14 PST by Antti Koivisto
Modified: 2016-03-30 00:07 PDT (History)
6 users (show)

See Also:


Attachments
patch (40.39 KB, patch)
2016-02-08 13:36 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (35.12 KB, patch)
2016-02-08 13:39 PST, Antti Koivisto
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (927.96 KB, application/zip)
2016-02-08 14:46 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2016-02-08 13:14:07 PST
Make it more hackable and correct.
Comment 1 Antti Koivisto 2016-02-08 13:36:04 PST
Created attachment 270877 [details]
patch
Comment 2 Antti Koivisto 2016-02-08 13:39:51 PST
Created attachment 270878 [details]
patch
Comment 3 Build Bot 2016-02-08 14:46:46 PST
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
Comment 4 Build Bot 2016-02-08 14:46:51 PST
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
Comment 5 Darin Adler 2016-02-08 15:06:42 PST
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?
Comment 6 Antti Koivisto 2016-02-08 17:16:50 PST
https://trac.webkit.org/r196281 (with a bug fix)
Comment 7 Antti Koivisto 2016-02-08 17:18:19 PST
> 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.