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.
<rdar://problem/20375516>
Created attachment 249873 [details] Patch
Created attachment 249875 [details] Patch
Created attachment 249876 [details] Patch
Created attachment 249878 [details] Patch
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.
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
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.
Created attachment 250035 [details] Patch
Created attachment 250036 [details] Patch (No longer based on global state)
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.
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.
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
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.
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.
Created attachment 250078 [details] Patch
Created attachment 250079 [details] Patch
Created attachment 250083 [details] Patch (Revised to correct test failure)
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.
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
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
Created attachment 250090 [details] Patch (Revised for non-Mac build failures, and documenting a test problem found by this work).
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 250112 [details] Patch
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
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
Created attachment 250207 [details] Patch
Created attachment 250208 [details] Patch
Created attachment 250209 [details] Patch (Correct iOS build)
Created attachment 250214 [details] Patch Part 1: (Version has Another iOS fix)
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?
Created attachment 250529 [details] Patch
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?
Committed r182768: <http://trac.webkit.org/changeset/182768>
Reopening to attach new patch.
Created attachment 251515 [details] Wrong Bug!
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.
Created attachment 251552 [details] Wrong Bug!
Created attachment 251556 [details] Wrong Bug
This was accidentally reopened when I used the wrong bug number in my patch.