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.
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.
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.
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
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.
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.
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
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
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.
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.
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
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.
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
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
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?
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?
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.
2015-03-31 17:59 PDT, Brent Fulgham
2015-03-31 18:04 PDT, Brent Fulgham
2015-03-31 18:08 PDT, Brent Fulgham
2015-03-31 18:15 PDT, Brent Fulgham
2015-03-31 18:54 PDT, Build Bot
2015-04-02 20:46 PDT, Brent Fulgham
2015-04-02 21:02 PDT, Brent Fulgham
2015-04-02 21:49 PDT, Build Bot
2015-04-02 21:53 PDT, Build Bot
2015-04-03 09:29 PDT, Brent Fulgham
2015-04-03 09:31 PDT, Brent Fulgham
2015-04-03 09:56 PDT, Brent Fulgham
2015-04-03 10:48 PDT, Build Bot
2015-04-03 11:16 PDT, Brent Fulgham
2015-04-03 18:00 PDT, Brent Fulgham
2015-04-03 18:52 PDT, Build Bot
2015-04-06 08:59 PDT, Brent Fulgham
2015-04-06 09:40 PDT, Brent Fulgham
2015-04-06 10:21 PDT, Brent Fulgham
2015-04-06 11:33 PDT, Brent Fulgham
2015-04-10 13:23 PDT, Brent Fulgham
2015-04-23 17:23 PDT, Brent Fulgham
2015-04-24 09:00 PDT, Brent Fulgham
2015-04-24 10:27 PDT, Brent Fulgham