Bug 100917 – There should be a way to dump the scrolling tree from the layout tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Beth Dakin]

Since we're about to add multiple items to the scrolling tree by supporting fixed nodes, there should be a way to dump the scrolling tree from the layout tests.

[Beth Dakin]

Created attachment 171770 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:223:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 20 files


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

[Build Bot]

Comment on attachment 171770 [details]
Patch

Attachment 171770 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14661501

[Simon Fraser (smfr)]

Comment on attachment 171770 [details]
Patch

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

This looks great, just some nits.

> Source/WebCore/page/Page.cpp:256
> +String Page::scrollingStateTreeAsText()
> +{
> +    if (Document* document = m_mainFrame->document())
> +        document->updateLayout();
> +
> +    if (ScrollingCoordinator* scrollingCoordinator = this->scrollingCoordinator())
> +        return scrollingCoordinator->scrollingStateTreeAsText();
> +
> +    return String();
> +}

I think we should put this on Frame, like layerTreeAsText(), especially since it's per-frame, and not deep across frame boundaries.

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:120
> +    virtual String scrollingStateTreeAsText();

Should be const I think.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:87
> +    static void writeIndent(TextStream&, int indent);

Should be const.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:94
> +    void dumpNode(TextStream&, int indent);
> +
> +    virtual void dumpProperties(TextStream&, int indent) = 0;

These too.

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:75
> +    virtual String scrollingStateTreeAsText() OVERRIDE;

const

> LayoutTests/platform/mac/tiled-drawing/scrolling-tree-slow-scrolling.html:9
> +            position: fixed; /* At this time, position:fixed forces slow mode. */

You could change the setting via internals to ensure this is always true.

[Beth Dakin]

Created attachment 171901 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 171901 [details]
Patch

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

> Source/WebCore/ChangeLog:41
> +        * WebCore.exp.in:
> +        * page/Page.cpp:
> +        (WebCore::Page::scrollingStateTreeAsText):
> +        (WebCore):
> +        * page/Page.h:
> +        (Page):
> +        * page/scrolling/ScrollingCoordinator.cpp:
> +        (WebCore::ScrollingCoordinator::scrollingStateTreeAsText):
> +        (WebCore):
> +        * page/scrolling/ScrollingCoordinator.h:
> +        (ScrollingCoordinator):
> +        * page/scrolling/ScrollingStateNode.cpp:
> +        (WebCore::ScrollingStateNode::writeIndent):
> +        (WebCore):
> +        (WebCore::ScrollingStateNode::dumpNode):
> +        (WebCore::ScrollingStateNode::scrollingStateTreeAsText):
> +        * page/scrolling/ScrollingStateNode.h:
> +        (WebCore):
> +        (ScrollingStateNode):
> +        * page/scrolling/ScrollingStateScrollingNode.cpp:
> +        (WebCore::ScrollingStateScrollingNode::dumpProperties):
> +        (WebCore):
> +        * page/scrolling/ScrollingStateScrollingNode.h:
> +        (ScrollingStateScrollingNode):
> +        * page/scrolling/mac/ScrollingCoordinatorMac.h:
> +        (ScrollingCoordinatorMac):
> +        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
> +        (WebCore::ScrollingCoordinatorMac::scrollingStateTreeAsText):
> +        (WebCore):
> +        * testing/Internals.cpp:
> +        (WebCore::Internals::scrollingStateTreeAsText):
> +        (WebCore):
> +        * testing/Internals.h:
> +        * testing/Internals.idl:

This list could be cleaned up.

> Source/WebCore/page/Page.h:198
> +        String scrollingStateTreeAsText();

const?

> Source/WebCore/page/scrolling/ScrollingStateNode.h:92
> +    void dumpNode(TextStream&, int indent) const;

I don't think you need Node in the name.

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:211
> +    ts << "(" << "ScrollingStateScrollingNode" << "\n";

I think it would be better to just dump "scrolling node", in case we rename the classes in future.

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:213
> +    if (m_viewportRect != IntRect()) {

Maybe !m_viewportRect.isEmpty()

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:218
> +    if (m_contentsSize != IntSize()) {

Maybe !m_contentsSize.isEmpty()

[Beth Dakin]

(In reply to comment #6)
> (From update of attachment 171901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171901&action=review
> 
> > Source/WebCore/ChangeLog:41
> > +        * WebCore.exp.in:
> > +        * page/Page.cpp:
> > +        (WebCore::Page::scrollingStateTreeAsText):
> > +        (WebCore):
> > +        * page/Page.h:
> > +        (Page):
> > +        * page/scrolling/ScrollingCoordinator.cpp:
> > +        (WebCore::ScrollingCoordinator::scrollingStateTreeAsText):
> > +        (WebCore):
> > +        * page/scrolling/ScrollingCoordinator.h:
> > +        (ScrollingCoordinator):
> > +        * page/scrolling/ScrollingStateNode.cpp:
> > +        (WebCore::ScrollingStateNode::writeIndent):
> > +        (WebCore):
> > +        (WebCore::ScrollingStateNode::dumpNode):
> > +        (WebCore::ScrollingStateNode::scrollingStateTreeAsText):
> > +        * page/scrolling/ScrollingStateNode.h:
> > +        (WebCore):
> > +        (ScrollingStateNode):
> > +        * page/scrolling/ScrollingStateScrollingNode.cpp:
> > +        (WebCore::ScrollingStateScrollingNode::dumpProperties):
> > +        (WebCore):
> > +        * page/scrolling/ScrollingStateScrollingNode.h:
> > +        (ScrollingStateScrollingNode):
> > +        * page/scrolling/mac/ScrollingCoordinatorMac.h:
> > +        (ScrollingCoordinatorMac):
> > +        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
> > +        (WebCore::ScrollingCoordinatorMac::scrollingStateTreeAsText):
> > +        (WebCore):
> > +        * testing/Internals.cpp:
> > +        (WebCore::Internals::scrollingStateTreeAsText):
> > +        (WebCore):
> > +        * testing/Internals.h:
> > +        * testing/Internals.idl:
> 
> This list could be cleaned up.
> 

Will do.

> > Source/WebCore/page/Page.h:198
> > +        String scrollingStateTreeAsText();
> 
> const?
> 

This one can't be const because scrollingCoordinator() is not const.

> > Source/WebCore/page/scrolling/ScrollingStateNode.h:92
> > +    void dumpNode(TextStream&, int indent) const;
> 
> I don't think you need Node in the name.
> 

Will remove.

> > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:211
> > +    ts << "(" << "ScrollingStateScrollingNode" << "\n";
> 
> I think it would be better to just dump "scrolling node", in case we rename the classes in future.
> 

Will change.

> > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:213
> > +    if (m_viewportRect != IntRect()) {
> 
> Maybe !m_viewportRect.isEmpty()
> 

Will fix.

> > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:218
> > +    if (m_contentsSize != IntSize()) {
> 
> Maybe !m_contentsSize.isEmpty()

Will fix.

[Beth Dakin]

Thanks, Simon!

http://trac.webkit.org/changeset/133205