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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted 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 171901: Patch
https://bugs.webkit.org/attachment.cgi?id=171901&action=review

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