Bug 143684

Created attachment 251561 [details] Patch

Comment on attachment 251561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251561&action=review > Source/WebCore/dom/Element.cpp:883 > +static void connectScrollAnimatorToTestTrigger(RenderBox& renderer, Document& document) > +{ > + if (!document.frame()) > + return; > + > + ScrollableArea* scrollableArea = nullptr; > + if (is<RenderListBox>(renderer)) > + scrollableArea = downcast<RenderListBox>(&renderer); > + else > + scrollableArea = renderer.layer(); > + > + if (!scrollableArea) > + return; > + > + scrollableArea->scrollAnimator().setWheelEventTestTrigger(document.frame()->mainFrame().testTrigger()); I don't think we should be running any of this code if not in a testing environment. There needs to be some "fast return" path that early returns outside of DRT/WTR. > Source/WebCore/dom/Element.cpp:891 > + connectScrollAnimatorToTestTrigger(*renderer, document()); Don't this and scrollTop() both bottleneck through some lower-level programmatic scroll code where you could set up the trigger? > Source/WebCore/page/MainFrame.cpp:134 > +RefPtr<WheelEventTestTrigger> MainFrame::ensureTestTrigger() ensure() should return a Ref<>. > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:303 > + frameView.scrollAnimator().setWheelEventTestTrigger(m_page->mainFrame().testTrigger()); > + scrollingTree()->setWheelEventTestTrigger(m_page->mainFrame().testTrigger()); > + if (const auto& trigger = m_page->mainFrame().testTrigger()) > + trigger->removeTestDeferralForReason(reinterpret_cast<WheelEventTestTrigger::ScrollableAreaIdentifier>(scrollingNodeID), WheelEventTestTrigger::ScrollingThreadSyncNeeded); > +#endif Again we should avoid running all this code when not testing.

Comment on attachment 251561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251561&action=review >> Source/WebCore/dom/Element.cpp:883 >> + scrollableArea->scrollAnimator().setWheelEventTestTrigger(document.frame()->mainFrame().testTrigger()); > > I don't think we should be running any of this code if not in a testing environment. There needs to be some "fast return" path that early returns outside of DRT/WTR. Maybe I could add a WebKit preference flag that could be set by clients who want to activate this code? Maybe "Internals" would be the right place to switch this on? >> Source/WebCore/dom/Element.cpp:891 >> + connectScrollAnimatorToTestTrigger(*renderer, document()); > > Don't this and scrollTop() both bottleneck through some lower-level programmatic scroll code where you could set up the trigger? I'll see if I can confirm this. >> Source/WebCore/page/MainFrame.cpp:134 >> +RefPtr<WheelEventTestTrigger> MainFrame::ensureTestTrigger() > > ensure() should return a Ref<>. OK! >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:303 >> +#endif > > Again we should avoid running all this code when not testing. OK!

Created attachment 251891 [details] Patch

Created attachment 251892 [details] Patch

Comment on attachment 251561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251561&action=review >>> Source/WebCore/dom/Element.cpp:891 >>> + connectScrollAnimatorToTestTrigger(*renderer, document()); >> >> Don't this and scrollTop() both bottleneck through some lower-level programmatic scroll code where you could set up the trigger? > > I'll see if I can confirm this. Yes -- they all pass through RenderLayer, but they lose access to the document. I can do it one step above (in RenderBox, and RenderListBox). >>> Source/WebCore/page/MainFrame.cpp:134 >>> +RefPtr<WheelEventTestTrigger> MainFrame::ensureTestTrigger() >> >> ensure() should return a Ref<>. > > OK! Actually, the nullability of this trigger value is important as a means of identifying when testing is happening. When the WheelEventTestTrigger is a nullptr, we don't want to do any processing. >>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:303 >>> +#endif >> >> Again we should avoid running all this code when not testing. > > OK! You won't have a test trigger when not testing, so the only code we run here is to set 'nullptr' in the scroll animator and the scrolling tree. We want to do this each pass in case the user activates testing via JS during a test run. Later, when they turn testing off we want to make sure that state gets propagated to the scrolling tree and scroll animators.

Created attachment 251995 [details] Patch

Comment on attachment 251995 [details] Patch Revised based on smfr's comments: 1. Add a new 'quick' flag for checking if we are in 'testing' mode. 2. Do not manipulate the scrollTree directly. Instead, update our testing state when the scrollTree syncs (and when we begin a scroll operation). Since all of the access to the test code was of the form "page->frame->mainFrame->testTrigger", I moved the trigger code to Page.

Comment on attachment 251995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251995&action=review > Tools/DumpRenderTree/mac/DumpRenderTree.mm:1834 > + This should be wrapped in "#if !PLATFORM(IOS)"

Comment on attachment 251995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251995&action=review > Source/WebCore/page/Page.h:447 > + WEBCORE_EXPORT bool isUnderTest() const { return m_isUnderTest; } > + void setIsUnderTest(bool isUnderTest) { m_isUnderTest = isUnderTest; } This name is much too generic. What about all the other kinds of testing that happens? I'd call this expectsWheelEventTriggers or something. > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:907 > + if (auto* trigger = testTrigger()) It this... > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1363 > + if (m_wheelEventTestTrigger) different from this? > Source/WebCore/rendering/RenderBox.cpp:597 > +static void connectScrollAnimatorToTestTrigger(RenderLayer* layer, Frame* frame) Maybe call this setupWheelEventTestTrigger()? > Source/WebCore/rendering/RenderBox.cpp:599 > + if (!layer || !frame) You should pass RenderLayer&, since the caller dereferences it just below. > Source/WebCore/rendering/RenderBox.cpp:606 > + layer->scrollAnimator().setWheelEventTestTrigger(page->testTrigger()); Could you put this code in RenderLayer::scrollToOffset()? > Source/WebCore/rendering/RenderListBox.cpp:666 > +static void connectScrollAnimatorToTestTrigger(RenderListBox& renderer, Frame* frame) Don't really like the "connect" terminology again. > Source/WebCore/testing/js/WebCoreTestSupport.cpp:99 > + page->setIsUnderTest(false); > + page->clearTrigger(); These always go hand-in-hand. Seems like Page::isUnderTest could just return whether the trigger is non-null. > Source/WebKit2/WebProcess/WebPage/EventDispatcher.cpp:145 > + if (WebPage* webPage = WebProcess::singleton().webPage(pageID)) { > + if (Page* page = webPage->corePage()) { > + if (page->isUnderTest()) { > + if (const auto& trigger = page->testTrigger()) > + trigger->deferTestsForReason(reinterpret_cast<WheelEventTestTrigger::ScrollableAreaIdentifier>(webPage->corePage()), WheelEventTestTrigger::ScrollingThreadSyncNeeded); > + } > + } > + } So many ifs!

Committed r183595: <http://trac.webkit.org/changeset/183595>

(In reply to comment #11) > Committed r183595: <http://trac.webkit.org/changeset/183595> EFL port has broken since r183595. Build fix - http://trac.webkit.org/changeset/183605.

Mac Build Fix: https://trac.webkit.org/r183616