[webkit-reviews] review denied: [Bug 100917] There should be a way to dump the scrolling tree from the layout tests : [Attachment 171770] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 100917: There should be a way to dump the scrolling tree from the layout
tests
https://bugs.webkit.org/show_bug.cgi?id=100917

Attachment 171770: Patch
https://bugs.webkit.org/attachment.cgi?id=171770&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.