Expand test infrastructure to support scrolling tests (Part 1)

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/20375516>

[Brent Fulgham]

Created attachment 249873 [details]
Patch

[Brent Fulgham]

Created attachment 249875 [details]
Patch

[Brent Fulgham]

Created attachment 249876 [details]
Patch

[Brent Fulgham]

Created attachment 249878 [details]
Patch

[Brent Fulgham]

The first question for reviewers is whether implementing this as a Singleton is the right approach.

I initially tried to have this be a member of the MainFrame. However, making the various ScrollController instances (which do not generally have visibility of their containing objects) was going to involve threading a lot of MainFrame arguments through various create functions.

[Simon Fraser (smfr)]

Comment on attachment 249878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249878&action=review

> Source/WebCore/page/WheelEventTestTrigger.cpp:62
> +    setTestNotificationCallback([=](void) {
> +        JSObjectCallAsFunction(context, functionCallback, nullptr, 0, nullptr, nullptr);
> +        JSValueUnprotect(context, functionCallback);
> +    });
> +}

I don't think we should have test code like this compiled into webcore all the time.

It feels like this WheelEventTestTrigger thing should be some kind of delegate that's only hooked up during testing.

[Build Bot]

Comment on attachment 249878 [details]
Patch

Attachment 249878 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5761338848575488

New failing tests:
scrollbars/scrollevent-iframe-no-scrolling-wheel.html

[Build Bot]

Created attachment 249880 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Brent Fulgham]

Note: This test failure indicates that the test doesn't work as expected -- the event handler is actually not locating a valid target NSView to process the event! That is not new with this change -- we can now just see that the test is not working properly.

[Brent Fulgham]

Created attachment 250035 [details]
Patch

[Brent Fulgham]

Created attachment 250036 [details]
Patch (No longer based on global state)

[Brent Fulgham]

Comment on attachment 250036 [details]
Patch (No longer based on global state)

Revised version of the patch that tracks test state on the MainFrame, but only when the test system registers to monitor wheel events.

[Build Bot]

Comment on attachment 250036 [details]
Patch (No longer based on global state)

Attachment 250036 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4823346982158336

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 250038 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 250036 [details]
Patch (No longer based on global state)

Attachment 250036 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6009453941882880

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 250039 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Brent Fulgham]

Comment on attachment 250036 [details]
Patch (No longer based on global state)

Looks like it's hitting some assertions I added to check for missing MainFram attachment. There must be another code path for constructing scroll animators I need to handle.

[Brent Fulgham]

Created attachment 250078 [details]
Patch

[Brent Fulgham]

Created attachment 250079 [details]
Patch

[Brent Fulgham]

Created attachment 250083 [details]
Patch (Revised to correct test failure)

[Brent Fulgham]

Comment on attachment 250083 [details]
Patch (Revised to correct test failure)

Corrected cause of 'fast/forms/search/search-scroll-hidden-decoration-container-crash.html' failure.

[Build Bot]

Comment on attachment 250083 [details]
Patch (Revised to correct test failure)

Attachment 250083 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5320753788485632

New failing tests:
scrollbars/scrollevent-iframe-no-scrolling-wheel.html

[Build Bot]

Created attachment 250089 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Brent Fulgham]

Created attachment 250090 [details]
Patch (Revised for non-Mac build failures, and documenting a test problem found by this work).

[Brent Fulgham]

Comment on attachment 250090 [details]
Patch (Revised for non-Mac build failures, and documenting a test problem found by this work).

Correct the build errors encountered on iOS/EFL/GTK. Also mark the WK1 version of "scrollbars/scrollevent-iframe-no-scrolling-wheel.html" as failing. This is not a bug caused by this patch; the test is already wrong, the patch just added logging that identified the problem.

[Brent Fulgham]

Created attachment 250112 [details]
Patch

[Build Bot]

Comment on attachment 250112 [details]
Patch

Attachment 250112 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6490975409012736

New failing tests:
scrollbars/scrollevent-iframe-no-scrolling-wheel.html

[Build Bot]

Created attachment 250114 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Brent Fulgham]

Created attachment 250207 [details]
Patch

[Brent Fulgham]

Created attachment 250208 [details]
Patch

[Brent Fulgham]

Created attachment 250209 [details]
Patch (Correct iOS build)

[Brent Fulgham]

Created attachment 250214 [details]
Patch Part 1: (Version has Another iOS fix)

[Simon Fraser (smfr)]

Comment on attachment 250214 [details]
Patch Part 1: (Version has Another iOS fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=250214&action=review

> Source/WebCore/page/MainFrame.cpp:53
> +    , m_testTrigger(nullptr)

No need to initialize a unique_ptr.

> Source/WebCore/page/MainFrame.cpp:129
> +WheelEventTestTrigger* MainFrame::testTrigger()

const ?

> Source/WebCore/page/MainFrame.cpp:138
> +WheelEventTestTrigger* MainFrame::initializeTestTrigger()
> +{
> +    m_testTrigger = std::make_unique<WheelEventTestTrigger>();
> +
> +    return m_testTrigger.get();

This is susceptible to doing the wrong thing if called more than once. Maybe ensureTestTrigger()?

> Source/WebCore/page/WheelEventTestTrigger.cpp:33
> +#include <JavaScriptCore/JSObjectRef.h>
> +#include <JavaScriptCore/JSRetainPtr.h>

Do you still need these?

> Source/WebCore/page/WheelEventTestTrigger.cpp:63
> +    std::lock_guard<std::mutex> lock(m_testTriggerMutex);
> +    m_testNotificationCallback = WTF::move(functionCallback);
> +    
> +    if (!m_testTriggerTimer.isActive())
> +        m_testTriggerTimer.startRepeating(1.0 / 60.0);
> +}

Seems odd to hold the lock over starting a timer. I think you need to be more explicit about what the lock is protecting (presumably just the m_deferTestTriggerReasons and callback? Or just m_deferTestTriggerReasons?

> Source/WebCore/page/WheelEventTestTrigger.cpp:92
> +    std::lock_guard<std::mutex> lock(m_testTriggerMutex);

Don't we use WTF::mutex etc?

> Source/WebCore/page/WheelEventTestTrigger.cpp:97
> +    if (m_testNotificationCallback)
> +        m_testNotificationCallback();

Do you really want to hold the lock over calling the callback?

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:577
> +void WKBundlePageMonitoringWheelEvents(WKBundlePageRef pageRef)

The name makes it sound like a getter "is the page monitoring wheel events?". Would be better as WKBundlePageStartMonitoringWheelEvents(), or WKBundlePageMonitoringScrollingSomething

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:588
> +void WKBundlePageRegisterTestNotificationCallback(WKBundlePageRef pageRef, WKBundlePageTestNotificationCallback callback, void* context)

What kind of test notification? Doesn't sound related to the previous thing; would be better as WKBundlePageRegisterScrollingSomethingCompletion

[Brent Fulgham]

Comment on attachment 250214 [details]
Patch Part 1: (Version has Another iOS fix)

View in context: https://bugs.webkit.org/attachment.cgi?id=250214&action=review

>> Source/WebCore/page/MainFrame.cpp:53
>> +    , m_testTrigger(nullptr)
> 
> No need to initialize a unique_ptr.

OK!

>> Source/WebCore/page/MainFrame.cpp:129
>> +WheelEventTestTrigger* MainFrame::testTrigger()
> 
> const ?

OK!

>> Source/WebCore/page/MainFrame.cpp:138
>> +    return m_testTrigger.get();
> 
> This is susceptible to doing the wrong thing if called more than once. Maybe ensureTestTrigger()?

OK!

>> Source/WebCore/page/WheelEventTestTrigger.cpp:33
>> +#include <JavaScriptCore/JSRetainPtr.h>
> 
> Do you still need these?

Good point. I'll remove them.

>> Source/WebCore/page/WheelEventTestTrigger.cpp:63
>> +}
> 
> Seems odd to hold the lock over starting a timer. I think you need to be more explicit about what the lock is protecting (presumably just the m_deferTestTriggerReasons and callback? Or just m_deferTestTriggerReasons?

Yes. I think assignment of the callback should be protected as well, though maybe that's not as important.

I mainly want to avoid two threads trying to insert deferral reasons at the same time. It's probably not likely (or even possible) for two threads to update the notification callback, so maybe this lock isn't really warranted.

>> Source/WebCore/page/WheelEventTestTrigger.cpp:92
>> +    std::lock_guard<std::mutex> lock(m_testTriggerMutex);
> 
> Don't we use WTF::mutex etc?

Anders says use STL now.

>> Source/WebCore/page/WheelEventTestTrigger.cpp:97
>> +        m_testNotificationCallback();
> 
> Do you really want to hold the lock over calling the callback?

No.

>> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:577
>> +void WKBundlePageMonitoringWheelEvents(WKBundlePageRef pageRef)
> 
> The name makes it sound like a getter "is the page monitoring wheel events?". Would be better as WKBundlePageStartMonitoringWheelEvents(), or WKBundlePageMonitoringScrollingSomething

OK. I like WKBundlePageStartMonitoringScrollOperations

>> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:588
>> +void WKBundlePageRegisterTestNotificationCallback(WKBundlePageRef pageRef, WKBundlePageTestNotificationCallback callback, void* context)
> 
> What kind of test notification? Doesn't sound related to the previous thing; would be better as WKBundlePageRegisterScrollingSomethingCompletion

How about WKBundlePageRegisterScrollOperationCompletionCallback?

[Brent Fulgham]

Created attachment 250529 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 250529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250529&action=review

> Source/WebCore/page/WheelEventTestTrigger.cpp:53
> +void WheelEventTestTrigger::setTestNotificationCallback(std::function<void()> functionCallback)

This name doesn't imply that it's going to start a timer, so perhaps a better name?

> Source/WebCore/page/WheelEventTestTrigger.cpp:55
> +    { // Limit scope of mutex

I don't think the comment is necessary.

> Source/WebCore/page/WheelEventTestTrigger.cpp:70
> +    auto it = m_deferTestTriggerReasons.find(key);
> +    if (it == m_deferTestTriggerReasons.end()) {
> +        m_deferTestTriggerReasons.add(key, std::set<DeferTestTriggerReason>());
> +        it = m_deferTestTriggerReasons.find(key);

You should just call add() and look at the AddResult.

> Source/WebCore/page/WheelEventTestTrigger.cpp:86
> +        m_deferTestTriggerReasons.remove(key);

Can't you remove with the it, to avoid another hash lookup?

> Source/WebCore/page/WheelEventTestTrigger.cpp:93
> +    { // Limit scope of mutex

No need for comment.

> Source/WebCore/page/WheelEventTestTrigger.h:54
> +    void deferTestsForReason(void* key, DeferTestTriggerReason);

Very unclear what the void* key should be here. Maybe a typedef with a better name? Can it be any token that just has to match the one passed to removeTestDeferralForReason?

[Brent Fulgham]

Committed r182768: <http://trac.webkit.org/changeset/182768>

[Brent Fulgham]

Reopening to attach new patch.

[Brent Fulgham]

Created attachment 251515 [details]
Wrong Bug!

[WebKit Commit Bot]

Attachment 251515 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/ThreadedScrollingTree.h:61:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/page/scrolling/ScrollingTree.h:140:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/platform/ScrollAnimator.h:121:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 251552 [details]
Wrong Bug!

[Brent Fulgham]

Created attachment 251556 [details]
Wrong Bug

[Brent Fulgham]

This was accidentally reopened when I used the wrong bug number in my patch.