Resource Load Statistics: Report user interaction immediately, but only when needed
authorwilander@apple.com <wilander@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Aug 2017 22:34:57 +0000 (22:34 +0000)
committerwilander@apple.com <wilander@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Aug 2017 22:34:57 +0000 (22:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=175090
<rdar://problem/33685546>

Reviewed by Chris Dumez.

Source/WebCore:

Test: http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html

* loader/ResourceLoadObserver.cpp:
(WebCore::ResourceLoadObserver::ResourceLoadObserver):
(WebCore::ResourceLoadObserver::logUserInteractionWithReducedTimeResolution):
    Now tells the UI process immediately but also records that it has
    done so to avoid doing it when not needed.
(WebCore::ResourceLoadObserver::scheduleNotificationIfNeeded):
    Conditional throttling gone, now always throttles.
(WebCore::ResourceLoadObserver::notifyObserver):
    Renamed from ResourceLoadObserver::notificationTimerFired().
(WebCore::ResourceLoadObserver::clearState):
    New function to allow the test runner to reset the web process'
    statistics state now that we keep track of whether or not we've
    reported user interaction to the UI process.
(WebCore::ResourceLoadObserver::setShouldThrottleObserverNotifications): Deleted.
(WebCore::ResourceLoadObserver::notificationTimerFired): Deleted.
* loader/ResourceLoadObserver.h:
(): Deleted.
* testing/Internals.cpp:
(WebCore::Internals::resetToConsistentState):
(WebCore::Internals::setResourceLoadStatisticsShouldThrottleObserverNotifications): Deleted.
    No longer needed since user interaction is always communicated
    immediately.
* testing/Internals.h:
* testing/Internals.idl:

Source/WebKit:

* WebProcess/InjectedBundle/API/c/WKBundle.cpp:
(WKBundleClearResourceLoadStatistics):
    Test infrastructure. Ends up calling
    WebCore::ResourceLoadObserver::clearState().
* WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:

Tools:

* WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
(WTR::InjectedBundle::beginTesting):
    Now calls WebCore::ResourceLoadObserver::clearState().

LayoutTests:

* http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:
    Now no longer needs to disable throttling since reports of
    user interaction happen immediately (when needed).
* http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time-expected.txt: Added.
* http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html: Added.
* platform/mac-wk2/TestExpectations:
    user-interaction-only-reported-once-within-short-period-of-time.html marked as [ Pass ].

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@220302 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html
LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html [new file with mode: 0644]
LayoutTests/platform/mac-wk2/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/loader/ResourceLoadObserver.cpp
Source/WebCore/loader/ResourceLoadObserver.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundle.cpp
Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h
Tools/ChangeLog
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp

index 64cf852..141d57a 100644 (file)
@@ -1,3 +1,19 @@
+2017-08-04  John Wilander  <wilander@apple.com>
+
+        Resource Load Statistics: Report user interaction immediately, but only when needed
+        https://bugs.webkit.org/show_bug.cgi?id=175090
+        <rdar://problem/33685546>
+
+        Reviewed by Chris Dumez.
+
+        * http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:
+            Now no longer needs to disable throttling since reports of
+            user interaction happen immediately (when needed).
+        * http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time-expected.txt: Added.
+        * http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html: Added.
+        * platform/mac-wk2/TestExpectations:
+            user-interaction-only-reported-once-within-short-period-of-time.html marked as [ Pass ].
+
 2017-08-04  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: capture async stack trace when workers/main context posts a message
index abdd07e..c3ad86c 100644 (file)
@@ -25,10 +25,8 @@ function finishTest() {
 }
 
 onload = function() {
-    if (internals) {
+    if (internals)
         internals.setResourceLoadStatisticsEnabled(true);
-        internals.setResourceLoadStatisticsShouldThrottleObserverNotifications(false);
-    }
 
     if (testRunner) {
         testRunner.setStatisticsNotifyPagesWhenDataRecordsWereScanned(true);
diff --git a/LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time-expected.txt b/LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time-expected.txt
new file mode 100644 (file)
index 0000000..abab6b1
--- /dev/null
@@ -0,0 +1,22 @@
+main frame - didStartProvisionalLoadForFrame
+main frame - didCommitLoadForFrame
+main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
+main frame - didFinishLoadForFrame
+Tests that user interaction is not reported repeatedly within a short period of time.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS document.location.origin is topFrameOrigin
+PASS testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin) is false
+PASS testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin) is false
+PASS testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin) is true
+PASS testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin) is false
+PASS testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin) is false
+PASS testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin) is false
+PASS testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin) is false
+PASS successfullyParsed is true
+
+TEST COMPLETE
+This is the test element
diff --git a/LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html b/LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html
new file mode 100644 (file)
index 0000000..414a8b3
--- /dev/null
@@ -0,0 +1,64 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="/js-test-resources/js-test.js"></script>
+<script src="/js-test-resources/ui-helper.js"></script>
+<script>
+    description("Tests that user interaction is not reported repeatedly within a short period of time.");
+    jsTestIsAsync = true;
+
+    const topFrameOrigin = "http://127.0.0.1:8000";
+    const subFrameOrigin = "http://localhost:8000";
+
+    function activateElement(elementId, callback) {
+        var element = document.getElementById(elementId);
+        var centerX = element.offsetLeft + element.offsetWidth / 2;
+        var centerY = element.offsetTop + element.offsetHeight / 2;
+        UIHelper.activateAt(centerX, centerY).then(
+            function() {
+                callback();
+            },
+            function() {
+                testFailed("Promise rejected.");
+                finishJSTest();
+            }
+        );
+    }
+
+    function firstInteraction() {
+        shouldBeFalse("testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin)");
+        shouldBeFalse("testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin)");
+        activateElement("testElement", secondInteraction);
+    }
+
+    function secondInteraction() {
+        shouldBeTrue("testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin)");
+        shouldBeFalse("testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin)");
+
+        if (testRunner)
+            testRunner.setStatisticsHasHadUserInteraction(topFrameOrigin, false);
+
+        shouldBeFalse("testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin)");
+
+        activateElement("testElement", finishTest);
+    }
+
+    function finishTest() {
+        shouldBeFalse("testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin)");
+        shouldBeFalse("testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin)");
+
+        finishJSTest();
+    }
+
+    onload = function() {
+        if (internals)
+            internals.setResourceLoadStatisticsEnabled(true);
+
+        shouldBe("document.location.origin", "topFrameOrigin");
+
+        firstInteraction();
+    };
+</script>
+<div id="testElement">This is the test element</div>
+</body>
+</html>
\ No newline at end of file
index a51c183..59f86b4 100644 (file)
@@ -728,8 +728,9 @@ webkit.org/b/167757 workers/bomb.html [ Pass Timeout ]
 
 webkit.org/b/172397 [ Sierra Debug ] animations/needs-layout.html [ Pass ImageOnlyFailure ]
 
-# Currently only tests with click, not tap.
+# Move to general wk2 expectations once webkit.org/b/175170 is resolved.
 http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html [ Pass ]
+http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html [ Pass ]
 
 webkit.org/b/173861 [ Release ] http/tests/webrtc/filtering-ice-candidate-same-origin-frame.html [ Pass Timeout ]
 webkit.org/b/173861 [ Release ] http/tests/webrtc/filtering-ice-candidate-cross-origin-frame.html [ Pass Timeout ]
index 9044436..a198e4f 100644 (file)
@@ -1,3 +1,38 @@
+2017-08-04  John Wilander  <wilander@apple.com>
+
+        Resource Load Statistics: Report user interaction immediately, but only when needed
+        https://bugs.webkit.org/show_bug.cgi?id=175090
+        <rdar://problem/33685546>
+
+        Reviewed by Chris Dumez.
+
+        Test: http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html
+
+        * loader/ResourceLoadObserver.cpp:
+        (WebCore::ResourceLoadObserver::ResourceLoadObserver):
+        (WebCore::ResourceLoadObserver::logUserInteractionWithReducedTimeResolution):
+            Now tells the UI process immediately but also records that it has
+            done so to avoid doing it when not needed.
+        (WebCore::ResourceLoadObserver::scheduleNotificationIfNeeded):
+            Conditional throttling gone, now always throttles.
+        (WebCore::ResourceLoadObserver::notifyObserver):
+            Renamed from ResourceLoadObserver::notificationTimerFired().
+        (WebCore::ResourceLoadObserver::clearState):
+            New function to allow the test runner to reset the web process'
+            statistics state now that we keep track of whether or not we've
+            reported user interaction to the UI process.
+        (WebCore::ResourceLoadObserver::setShouldThrottleObserverNotifications): Deleted.
+        (WebCore::ResourceLoadObserver::notificationTimerFired): Deleted.
+        * loader/ResourceLoadObserver.h:
+        (): Deleted.
+        * testing/Internals.cpp:
+        (WebCore::Internals::resetToConsistentState):
+        (WebCore::Internals::setResourceLoadStatisticsShouldThrottleObserverNotifications): Deleted.
+            No longer needed since user interaction is always communicated
+            immediately.
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2017-08-04  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: capture async stack trace when workers/main context posts a message
index 37b710e..5c228eb 100644 (file)
@@ -99,19 +99,6 @@ static bool areDomainsAssociated(Page* page, const String& firstDomain, const St
     return firstMetaDomainIdentifier == metaDomainIdentifiers.get().get(secondDomain);
 }
 
-void ResourceLoadObserver::setShouldThrottleObserverNotifications(bool shouldThrottle)
-{
-    m_shouldThrottleNotifications = shouldThrottle;
-
-    if (!m_notificationTimer.isActive())
-        return;
-
-    // If we change the notification state, we need to restart any notifications
-    // so they will be on the right schedule.
-    m_notificationTimer.stop();
-    scheduleNotificationIfNeeded();
-}
-
 void ResourceLoadObserver::setNotificationCallback(WTF::Function<void (Vector<ResourceLoadStatistics>&&)>&& notificationCallback)
 {
     ASSERT(!m_notificationCallback);
@@ -119,7 +106,7 @@ void ResourceLoadObserver::setNotificationCallback(WTF::Function<void (Vector<Re
 }
 
 ResourceLoadObserver::ResourceLoadObserver()
-    : m_notificationTimer(*this, &ResourceLoadObserver::notificationTimerFired)
+    : m_notificationTimer(*this, &ResourceLoadObserver::notifyObserver)
 {
 }
 
@@ -280,16 +267,21 @@ void ResourceLoadObserver::logUserInteractionWithReducedTimeResolution(const Doc
     if (url.isBlankURL() || url.isEmpty())
         return;
 
-    auto& statistics = ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));
+    auto domain = primaryDomain(url);
     auto newTime = reduceToHourlyTimeResolution(WallTime::now());
-    if (newTime == statistics.mostRecentUserInteractionTime)
+    auto lastReportedUserInteraction = m_lastReportedUserInteractionMap.get(domain);
+    if (newTime == lastReportedUserInteraction)
         return;
 
+    m_lastReportedUserInteractionMap.set(domain, newTime);
+
+    auto& statistics = ensureResourceStatisticsForPrimaryDomain(domain);
     statistics.hadUserInteraction = true;
     statistics.lastSeen = newTime;
     statistics.mostRecentUserInteractionTime = newTime;
 
-    scheduleNotificationIfNeeded();
+    m_notificationTimer.stop();
+    notifyObserver();
 }
 
 ResourceLoadStatistics& ResourceLoadObserver::ensureResourceStatisticsForPrimaryDomain(const String& primaryDomain)
@@ -309,12 +301,13 @@ void ResourceLoadObserver::scheduleNotificationIfNeeded()
     }
 
     if (!m_notificationTimer.isActive())
-        m_notificationTimer.startOneShot(m_shouldThrottleNotifications ? minimumNotificationInterval : 0_s);
+        m_notificationTimer.startOneShot(minimumNotificationInterval);
 }
 
-void ResourceLoadObserver::notificationTimerFired()
+void ResourceLoadObserver::notifyObserver()
 {
     ASSERT(m_notificationCallback);
+    m_notificationTimer.stop();
     m_notificationCallback(takeStatistics());
 }
 
@@ -339,4 +332,11 @@ Vector<ResourceLoadStatistics> ResourceLoadObserver::takeStatistics()
     return statistics;
 }
 
+void ResourceLoadObserver::clearState()
+{
+    m_notificationTimer.stop();
+    m_resourceStatisticsMap.clear();
+    m_lastReportedUserInteractionMap.clear();
+}
+
 } // namespace WebCore
index 32684bd..dcc2946 100644 (file)
@@ -33,6 +33,7 @@
 namespace WTF {
 class Lock;
 class WorkQueue;
+class WallTime;
 }
 
 namespace WebCore {
@@ -62,6 +63,7 @@ public:
 
     WEBCORE_EXPORT void setNotificationCallback(WTF::Function<void (Vector<ResourceLoadStatistics>&&)>&&);
 
+    WEBCORE_EXPORT void clearState();
 private:
     ResourceLoadObserver();
 
@@ -69,13 +71,13 @@ private:
     ResourceLoadStatistics& ensureResourceStatisticsForPrimaryDomain(const String&);
 
     void scheduleNotificationIfNeeded();
-    void notificationTimerFired();
+    void notifyObserver();
     Vector<ResourceLoadStatistics> takeStatistics();
 
     HashMap<String, ResourceLoadStatistics> m_resourceStatisticsMap;
+    HashMap<String, WTF::WallTime> m_lastReportedUserInteractionMap;
     WTF::Function<void (Vector<ResourceLoadStatistics>&&)> m_notificationCallback;
     Timer m_notificationTimer;
-    bool m_shouldThrottleNotifications { true };
 };
     
 } // namespace WebCore
index ee2f91f..4b25a91 100644 (file)
@@ -468,8 +468,6 @@ void Internals::resetToConsistentState(Page& page)
 #if USE(LIBWEBRTC)
     WebCore::useRealRTCPeerConnectionFactory();
 #endif
-    
-    ResourceLoadObserver::shared().setShouldThrottleObserverNotifications(true);
 }
 
 Internals::Internals(Document& document)
@@ -3860,11 +3858,6 @@ void Internals::setResourceLoadStatisticsEnabled(bool enable)
     Settings::setResourceLoadStatisticsEnabled(enable);
 }
 
-void Internals::setResourceLoadStatisticsShouldThrottleObserverNotifications(bool enable)
-{
-    ResourceLoadObserver::shared().setShouldThrottleObserverNotifications(enable);
-}
-    
 String Internals::composedTreeAsText(Node& node)
 {
     if (!is<ContainerNode>(node))
index ec2f22b..0964184 100644 (file)
@@ -539,7 +539,6 @@ public:
 
     String resourceLoadStatisticsForOrigin(const String& origin);
     void setResourceLoadStatisticsEnabled(bool);
-    void setResourceLoadStatisticsShouldThrottleObserverNotifications(bool);
 
 #if ENABLE(STREAMS_API)
     bool isReadableStreamDisturbed(JSC::ExecState&, JSC::JSValue);
index 89d14d7..c46e1c6 100644 (file)
@@ -498,7 +498,6 @@ enum EventThrottlingBehavior {
 
     DOMString resourceLoadStatisticsForOrigin(DOMString domain);
     void setResourceLoadStatisticsEnabled(boolean enable);
-    void setResourceLoadStatisticsShouldThrottleObserverNotifications(boolean enable);
 
     [MayThrowException] void setCanShowModalDialogOverride(boolean allow);
 
index cf3b538..18f0715 100644 (file)
@@ -1,3 +1,17 @@
+2017-08-04  John Wilander  <wilander@apple.com>
+
+        Resource Load Statistics: Report user interaction immediately, but only when needed
+        https://bugs.webkit.org/show_bug.cgi?id=175090
+        <rdar://problem/33685546>
+
+        Reviewed by Chris Dumez.
+
+        * WebProcess/InjectedBundle/API/c/WKBundle.cpp:
+        (WKBundleClearResourceLoadStatistics):
+            Test infrastructure. Ends up calling
+            WebCore::ResourceLoadObserver::clearState().
+        * WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:
+
 2017-08-04  Matt Lewis  <jlewis3@apple.com>
 
         Unreviewed, rolling out r220288.
index d0826dd..34bce00 100644 (file)
@@ -39,6 +39,7 @@
 #include "WebPage.h"
 #include "WebPageGroupProxy.h"
 #include <WebCore/DatabaseTracker.h>
+#include <WebCore/ResourceLoadObserver.h>
 
 using namespace WebCore;
 using namespace WebKit;
@@ -276,3 +277,8 @@ void WKBundleSetTabKeyCyclesThroughElements(WKBundleRef bundleRef, WKBundlePageR
 {
     toImpl(bundleRef)->setTabKeyCyclesThroughElements(toImpl(pageRef), enabled);
 }
+
+void WKBundleClearResourceLoadStatistics(WKBundleRef)
+{
+    ResourceLoadObserver::shared().clearState();
+}
index ce83243..9b58a9e 100644 (file)
@@ -91,6 +91,8 @@ WK_EXPORT bool WKBundleIsProcessingUserGesture(WKBundleRef bundle);
 
 WK_EXPORT void WKBundleSetTabKeyCyclesThroughElements(WKBundleRef bundle, WKBundlePageRef page, bool enabled);
 
+WK_EXPORT void WKBundleClearResourceLoadStatistics(WKBundleRef);
+
 #ifdef __cplusplus
 }
 #endif
index 6575c46..73803de 100644 (file)
@@ -1,3 +1,15 @@
+2017-08-04  John Wilander  <wilander@apple.com>
+
+        Resource Load Statistics: Report user interaction immediately, but only when needed
+        https://bugs.webkit.org/show_bug.cgi?id=175090
+        <rdar://problem/33685546>
+
+        Reviewed by Chris Dumez.
+
+        * WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
+        (WTR::InjectedBundle::beginTesting):
+            Now calls WebCore::ResourceLoadObserver::clearState().
+
 2017-08-04  Tim Horton  <timothy_horton@apple.com>
 
         Add an API test for r220286
index 7a58e23..96b0113 100644 (file)
@@ -367,6 +367,7 @@ void InjectedBundle::beginTesting(WKDictionaryRef settings)
     WKBundleClearAllDatabases(m_bundle);
     WKBundlePageClearApplicationCache(page()->page());
     WKBundleResetOriginAccessWhitelists(m_bundle);
+    WKBundleClearResourceLoadStatistics(m_bundle);
 
     // [WK2] REGRESSION(r128623): It made layout tests extremely slow
     // https://bugs.webkit.org/show_bug.cgi?id=96862