Bug 143684 – Expand test infrastructure to support scrolling tests (Part 2)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

Part 2: Connect new testing infrastructure to WebCore.

Our scrolling tests currently rely on large timeouts to ensure that tests work properly. This is because we need to wait to check our final DOM state until any scroll-generated animations complete. For example, we need to wait until any rubber banding or scroll-snap animations complete before we can determine if the scroll gesture moved us to the proper location.

This change adds a new test EventSender object method called "callAfterScrollingCompletes". This new method will be called when the scroll animations are finished, greatly reducing the need for arbitrary timeouts.

[Brent Fulgham]

Created attachment 251561 [details]
Patch

[Simon Fraser (smfr)]

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.

[Brent Fulgham]

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!

[Brent Fulgham]

Created attachment 251891 [details]
Patch

[Brent Fulgham]

Created attachment 251892 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 251995 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

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)"

[Simon Fraser (smfr)]

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!

[Brent Fulgham]

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

[Gyuyoung Kim]

(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.

[Joseph Pecoraro]

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