Bug 133798 – Enable frame traversal through range-based for-loops

• Component: New Bugs
• Status: NEW

[Zan Dobersek]

Enable frame traversal through range-based for-loops

[Zan Dobersek]

Created attachment 232958 [details]
Patch

[WebKit Commit Bot]

Attachment 232958 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:432:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/FrameIterator.h:108:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/FrameIterator.h:114:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/dom/Document.cpp:5111:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 232958 [details]
Patch

Great idea. Not compiling in EWS, though.

[Zan Dobersek]

Created attachment 234448 [details]
Patch

[WebKit Commit Bot]

Attachment 234448 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:432:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/FrameIterator.h:108:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/FrameIterator.h:114:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/dom/Document.cpp:5137:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Created attachment 234453 [details]
Patch

[WebKit Commit Bot]

Attachment 234453 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:432:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/FrameIterator.h:108:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/FrameIterator.h:114:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/dom/Document.cpp:5137:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Created attachment 234459 [details]
Patch

Uploading for EWS processing, won't be requesting review until the patch builds everywhere.

[WebKit Commit Bot]

Attachment 234459 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.cpp:432:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/FrameIterator.h:108:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/page/FrameIterator.h:114:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/dom/Document.cpp:5137:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 234459 [details]
Patch

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

In such refactorings we have to be sure that both readability and performance as the same or better than before.

I am slightly concerned about performance, for example making sure that the empty objects being copied as m_incrementOperation don’t have a performance cost.

The code rewritten to use standard library functions like find_of and any_of looks harder to read than the original, although clearer in some ways, it’s noisy with begin/end and lambda syntax and as I am accustomed to reading loops sometimes I find the non-loop version harder to understand. I wonder if we could fix that somehow.

> Source/WebCore/ChangeLog:22
> +        - frameChildren(Type& frame)
> +        - frameReverseChildren(Type& frame)
> +        - frameLastChildren(Type& frame)
> +        - frameLineage(Type& frame)
> +        - frameAncestors(Type& frame)
> +        - frameTraversal(Type& frame)
> +        - frameTraversalWithin(Type& frame)
> +        - frameTraversalWithin(Type& frame, const Frame& stayWithin)

I don’t think these functions need the word “frame” in their names.

I’m also not sure that “reverse children” is the right name for iterating children backwards. I think “children backwards” might work better.

I don’t think that “last children” really makes sense as a name of a set of objects. This is a peculiar enough operation that I think we might want to omit it and leave it written out the old way at the single call site.

I don’t think we should have both frameTraversal and frameTraversalWithin. There is no case where starting to traverse at a frame other than the main frame, and iterating out to the parent, is intentional rather than a bug. So the two functions should be merged into a single one. To do that we might want to fix the code *before* this refactoring so there are no call sites that try to make a distinction.

The “descendants” operation should be named that, not “traversal”. But I don’t know what to call “descendants including self”; Antti or Andreas might have an idea.\

> Source/WebCore/dom/Document.cpp:357
> +    for (auto& ancestorFrame : frameLineage(*targetFrame)) {
> +        Document* ancestorDocument = ancestorFrame.document();

Makes me wish for a document lineage function.

> Source/WebCore/dom/Document.cpp:1950
> +    for (const auto& frame : frameTraversal(page.mainFrame())) {

Seems to me that frameTraversal should take either a Page or a MainFrame, not a Frame.

> Source/WebCore/dom/Document.cpp:5135
> +            auto traversal = frameTraversal(*frame());

This does a complete traversal only because this is a “document in main frame only” function. Would be best to assert this, I think.

> Source/WebCore/dom/Document.cpp:5137
> +            descendentHasNonEmptyStack = std::any_of(traversal.begin(), traversal.end(),
> +                [](const Frame& descendant) { return !!descendant.document()->webkitFullscreenElement(); });

Maybe it will take some getting used to; I don’t find this any_of form all that readable. Especially when we have to pass both begin and end. Maybe it’s partly because the lambda isn’t formatted the way we usually do?

The use of !! here is ugly. There has to be a more elegant way of doing that. Maybe just declaring the return value for the lambda and then leaving it out? Maybe we could just leave out the !! and everything would work fine?

> Source/WebCore/dom/Document.cpp:5259
> +        for (auto& descendant : frameTraversal(*frame())) {

This does a complete traversal only because this is a “document in main frame only” function. Would be best to assert this, I think.

> Source/WebCore/dom/Document.cpp:5790
> +    for (const auto& frame : frameTraversal(page->mainFrame())) {

The fact that this does a complete traversal is a bug.

> Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:102
> +    for (auto& frame : frameTraversalWithin(*m_pageAgent->mainFrame())) {

There is no reason to use frameTraversalWithin here; for the main frame there is no distinction between frameTraversal and frameTraversalWithin.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:270
> +    for (auto& frame : frameTraversal(*m_document->frame())) {

This function only uses frameTraversal because it is actually a “main frame only” function. We should assert that instead.

> Source/WebCore/inspector/InspectorPageAgent.cpp:514
> +    for (auto& frame : frameTraversalWithin(*mainFrame())) {

There is no reason to use frameTraversalWithin here; for the main frame there is no distinction between frameTraversal and frameTraversalWithin.

> Source/WebCore/inspector/InspectorPageAgent.cpp:546
> +    for (auto& frame : frameTraversalWithin(*mainFrame()))

There is no reason to use frameTraversalWithin here; for the main frame there is no distinction between frameTraversal and frameTraversalWithin.

> Source/WebCore/inspector/InspectorPageAgent.cpp:628
> +    for (auto& frame : frameTraversalWithin(*mainFrame())) {

There is no reason to use frameTraversalWithin here; for the main frame there is no distinction between frameTraversal and frameTraversalWithin.

> Source/WebCore/loader/FrameLoader.cpp:272
> +    fprintf(stderr, "FrameLoader::init() %p frame %p ref count %d\n", this, &m_frame, m_frame.refCount());

Please don’t add this unconditional printf. Maybe you were using it to debug this patch?

> Source/WebCore/loader/FrameLoader.cpp:785
> +    auto children = frameChildren(m_frame);
> +    return std::all_of(children.begin(), children.end(), [](const Frame& frame) {
> +        return frame.loader().m_isComplete;
> +    });

Seems like we should make an all_of that takes a collection so we don’t need the local variable and the begin/end. Is there something coming in future C++ to address this? (Same thought for any_of, naturally.)

> Source/WebCore/loader/FrameLoader.cpp:1141
> +    fprintf(stderr, "FrameLoader::completed() %p frame %p (%d) m_isComplete %d\n", this, &m_frame, m_frame.refCount(), m_isComplete);

More unconditional printf?

> Source/WebCore/loader/FrameLoader.cpp:1146
> +    if (Frame* descendant = m_frame.tree().traverseNext(&m_frame)) {
> +        for (auto& frame : frameTraversalWithin(*descendant, m_frame))
> +            frame.navigationScheduler().startTimer();
> +    }

This is pretty awkward. I guess the point here to include everything within the frame, not counting the frame itself. That calls into question the behavior of frameTraversal. In the other cases we have used the name descendants to mean “descendants not counting this thing itself” and we should use that. I don’t think that “self plus descendants” ought to be named “traversal within”.

> Source/WebCore/loader/FrameLoader.cpp:1147
> +    fprintf(stderr, "\tframe %p (%d)\n", &m_frame, m_frame.refCount());

More unconditional printf?

> Source/WebCore/loader/FrameLoader.cpp:1150
> +        fprintf(stderr, "\tparent %p\n", parent);

More unconditional printf?

> Source/WebCore/loader/FrameLoader.cpp:1154
> +    fprintf(stderr, "\tframe %p (%d)\n", &m_frame, m_frame.refCount());

More unconditional printf?

> Source/WebCore/loader/FrameLoader.cpp:1158
> +    fprintf(stderr, "\tEND OF FrameLoader::completed() %p frame %p (%d)\n", this, &m_frame, m_frame.refCount());

More unconditional printf?

> Source/WebCore/loader/FrameLoader.cpp:2812
> +    if (Frame* firstChild = m_frame.tree().firstChild()) {
> +        for (auto& frame : frameTraversalWithin(*firstChild, m_frame))
> +            targetFrames.append(frame);
> +    }

Should be descendants.

> Source/WebCore/page/FrameIterator.h:1
> +#ifndef FrameIterator_h

New source files need a copyright header.

> Source/WebCore/page/FrameIterator.h:11
> +struct ChildIteratorIncrement {

Not happy with the names of these structures.

> Source/WebCore/page/FrameIterator.h:35
> +struct FrameIterator : std::iterator<std::input_iterator_tag, FrameType> {

Why struct instead of class?

> Source/WebCore/page/FrameIterator.h:36
> +    FrameIterator() = default;

Seems strange that we don’t initialize the members in the default constructor.

> Source/WebCore/page/FrameIterator.h:48
> +    operator FrameType*() const { return m_frame; }

Do we need this? What code uses it?

> Source/WebCore/page/FrameIterator.h:58
> +    bool operator==(const FrameIterator& other) const { return m_frame == other.m_frame; }
> +    bool operator!=(const FrameIterator& other) const { return !(*this == other); }

I’d normally suggest making these free functions rather than members.

> Source/WebCore/page/FrameTree.cpp:426
> +    for (const auto& child : WebCore::frameChildren(frame))

The WebCore:: prefix should not be needed here because the type being passed in is already in the WebCore namespace, so argument dependent lookup should work.

> Source/WebCore/page/FrameView.cpp:4039
> +    for (auto& frame : frameTraversal(m_frame->tree().top())) {
> +        if (RenderView* renderView = frame.contentRenderer())
>              renderView->compositor().setTracksRepaints(trackRepaints);
>      }

This code is peculiar. Why would a particular subframe’s FrameView be so presumptuous as to set this on its parent frame’s RenderView. My guess is that this is an “only top level FrameView” function and needs an assertion saying that.

> Source/WebCore/page/Page.cpp:272
> +    // FIXME: Revert to mainFrame() when const iteration is supported.

Doesn’t seem like a valuable FIXME. Even if we do plan to make this change, it doesn’t seem worth a FIXME.

> Source/WebCore/replay/SerializationMethods.cpp:87
>      unsigned long currentIndex = 0;

The use of unsigned long in this function is quite strange.