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.
Created attachment 171770 [details] Patch
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.
Comment on attachment 171770 [details] Patch Attachment 171770 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14661501
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.
Created attachment 171901 [details] Patch
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()
(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.
Thanks, Simon! http://trac.webkit.org/changeset/133205