[ContentChangeObserver] Introduce ContentChangeObserver::adjustObservedState
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 3 Mar 2019 00:15:11 +0000 (00:15 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 3 Mar 2019 00:15:11 +0000 (00:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195244
<rdar://problem/48536737>

Reviewed by Simon Fraser.

Source/WebCore:

Move state change handling code to adjustObservedState() and introduce signalContentChangeIfNeeded() to
let the client know about the state change (or lack of state change).

Test: fast/events/touch/ios/visibility-change-happens-at-the-second-timer.html

* page/ios/ContentChangeObserver.cpp:
(WebCore::ContentChangeObserver::didInstallDOMTimer):
(WebCore::ContentChangeObserver::didRemoveDOMTimer):
(WebCore::ContentChangeObserver::stopObservingDOMTimerExecute):
(WebCore::ContentChangeObserver::stopObservingStyleRecalc):
(WebCore::ContentChangeObserver::clearTimersAndReportContentChange):
(WebCore::ContentChangeObserver::didContentVisibilityChange):
(WebCore::ContentChangeObserver::addObservedDOMTimer):
(WebCore::ContentChangeObserver::removeObservedDOMTimer):
(WebCore::ContentChangeObserver::setShouldObserveStyleRecalc):
(WebCore::ContentChangeObserver::adjustObservedState):
(WebCore::ContentChangeObserver::signalContentChangeIfNeeded):
* page/ios/ContentChangeObserver.h:
(WebCore::ContentChangeObserver::isObservingDOMTimerScheduling const):
(WebCore::ContentChangeObserver::addObservedDOMTimer): Deleted.
(WebCore::ContentChangeObserver::setShouldObserveStyleRecalc): Deleted.

LayoutTests:

* fast/events/touch/ios/visibility-change-happens-at-the-second-timer-expected.txt: Added.
* fast/events/touch/ios/visibility-change-happens-at-the-second-timer.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/touch/ios/visibility-change-happens-at-the-second-timer-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/touch/ios/visibility-change-happens-at-the-second-timer.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/ios/ContentChangeObserver.cpp
Source/WebCore/page/ios/ContentChangeObserver.h

index cd61ff5..0a431ee 100644 (file)
@@ -1,3 +1,14 @@
+2019-03-02  Zalan Bujtas  <zalan@apple.com>
+
+        [ContentChangeObserver] Introduce ContentChangeObserver::adjustObservedState
+        https://bugs.webkit.org/show_bug.cgi?id=195244
+        <rdar://problem/48536737>
+
+        Reviewed by Simon Fraser.
+
+        * fast/events/touch/ios/visibility-change-happens-at-the-second-timer-expected.txt: Added.
+        * fast/events/touch/ios/visibility-change-happens-at-the-second-timer.html: Added.
+
 2019-03-02  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [iOS] Programmatic paste access should be granted when copying and pasting within the same origin
diff --git a/LayoutTests/fast/events/touch/ios/visibility-change-happens-at-the-second-timer-expected.txt b/LayoutTests/fast/events/touch/ios/visibility-change-happens-at-the-second-timer-expected.txt
new file mode 100644 (file)
index 0000000..66db4c9
--- /dev/null
@@ -0,0 +1,2 @@
+PASS if 'clicked' text is not shown below.
+
diff --git a/LayoutTests/fast/events/touch/ios/visibility-change-happens-at-the-second-timer.html b/LayoutTests/fast/events/touch/ios/visibility-change-happens-at-the-second-timer.html
new file mode 100644 (file)
index 0000000..3c15b78
--- /dev/null
@@ -0,0 +1,65 @@
+<html>
+<head>
+<title>This tests the case when the second timer triggers visible content change</title>
+<script src="../../../../resources/basic-gestures.js"></script>
+<style>
+#tapthis {
+    width: 400px;
+    height: 400px;
+    border: 1px solid green;
+}
+
+#becomesVisible {
+    visibility: hidden;
+    width: 100px;
+    height: 100px;
+    background-color: green;
+}
+</style>
+<script>
+async function test() {
+    if (!window.testRunner || !testRunner.runUIScript)
+        return;
+
+    testRunner.waitUntilDone();
+    testRunner.dumpAsText();
+
+    let rect = tapthis.getBoundingClientRect();
+    let x = rect.left + rect.width / 2;
+    let y = rect.top + rect.height / 2;
+
+    await tapAtPoint(x, y);
+}
+</script>
+</head>
+<body onload="test()">
+<div id=tapthis>PASS if 'clicked' text is not shown below.</div>
+<div id=becomesVisible></div>
+<pre id=result></pre>
+<script>
+tapthis.addEventListener("mouseover", function( event ) {
+    // 1. Install 2 timers on hover
+    setTimeout(function() {
+        // 2. Trigger some non-visibility style change with forcing offsetHeight.
+        becomesVisible.style.marginLeft = "5px";
+        document.body.offsetHeight;
+    }, 0);
+    // 3. Install a longer timer with visibility change.
+    setTimeout(function() {
+        becomesVisible.style.visibility = "visible";
+        document.body.offsetHeight;
+        if (window.testRunner)
+            setTimeout(testRunner.notifyDone(), 0);
+    }, 20);
+}, false);
+
+becomesVisible.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked hidden";
+}, false);
+
+tapthis.addEventListener("click", function( event ) {   
+    result.innerHTML = "clicked";
+}, false);
+</script>
+</body>
+</html>
index aafd88d..69ec099 100644 (file)
@@ -1,5 +1,35 @@
 2019-03-02  Zalan Bujtas  <zalan@apple.com>
 
+        [ContentChangeObserver] Introduce ContentChangeObserver::adjustObservedState
+        https://bugs.webkit.org/show_bug.cgi?id=195244
+        <rdar://problem/48536737>
+
+        Reviewed by Simon Fraser.
+
+        Move state change handling code to adjustObservedState() and introduce signalContentChangeIfNeeded() to
+        let the client know about the state change (or lack of state change).
+
+        Test: fast/events/touch/ios/visibility-change-happens-at-the-second-timer.html
+
+        * page/ios/ContentChangeObserver.cpp:
+        (WebCore::ContentChangeObserver::didInstallDOMTimer):
+        (WebCore::ContentChangeObserver::didRemoveDOMTimer):
+        (WebCore::ContentChangeObserver::stopObservingDOMTimerExecute):
+        (WebCore::ContentChangeObserver::stopObservingStyleRecalc):
+        (WebCore::ContentChangeObserver::clearTimersAndReportContentChange):
+        (WebCore::ContentChangeObserver::didContentVisibilityChange):
+        (WebCore::ContentChangeObserver::addObservedDOMTimer):
+        (WebCore::ContentChangeObserver::removeObservedDOMTimer):
+        (WebCore::ContentChangeObserver::setShouldObserveStyleRecalc):
+        (WebCore::ContentChangeObserver::adjustObservedState):
+        (WebCore::ContentChangeObserver::signalContentChangeIfNeeded):
+        * page/ios/ContentChangeObserver.h:
+        (WebCore::ContentChangeObserver::isObservingDOMTimerScheduling const):
+        (WebCore::ContentChangeObserver::addObservedDOMTimer): Deleted.
+        (WebCore::ContentChangeObserver::setShouldObserveStyleRecalc): Deleted.
+
+2019-03-02  Zalan Bujtas  <zalan@apple.com>
+
         [ContentChangeObserver] Move away from WKContentChange values
         https://bugs.webkit.org/show_bug.cgi?id=195240
         <rdar://problem/48532358>
index 88f479c..af92dc2 100644 (file)
@@ -65,8 +65,7 @@ void ContentChangeObserver::didInstallDOMTimer(const DOMTimer& timer, Seconds ti
         return;
     LOG_WITH_STREAM(ContentObservation, stream << "didInstallDOMTimer: register this timer: (" << &timer << ") and observe when it fires.");
 
-    setHasIndeterminateState();
-    addObservedDOMTimer(timer);
+    registerDOMTimer(timer);
 }
 
 void ContentChangeObserver::didRemoveDOMTimer(const DOMTimer& timer)
@@ -75,10 +74,8 @@ void ContentChangeObserver::didRemoveDOMTimer(const DOMTimer& timer)
         return;
     LOG_WITH_STREAM(ContentObservation, stream << "removeDOMTimer: remove registered timer (" << &timer << ")");
 
-    removeObservedDOMTimer(timer);
-    // FIXME: Just because this is the last timer, it does not mean we are in a determined state.
-    if (!hasObservedDOMTimer())
-        m_page.chrome().client().observedContentChange(m_page.mainFrame());
+    unregisterDOMTimer(timer);
+    notifyContentChangeIfNeeded();
 }
 
 void ContentChangeObserver::startObservingDOMTimerExecute(const DOMTimer& timer)
@@ -96,19 +93,10 @@ void ContentChangeObserver::stopObservingDOMTimerExecute(const DOMTimer& timer)
         return;
     LOG_WITH_STREAM(ContentObservation, stream << "stopObservingDOMTimerExecute: stop observing (" << &timer << ") timer callback.");
 
-    removeObservedDOMTimer(timer);
-    stopObservingContentChanges();
-    // Check if the timer callback triggered either a sync or async style update.
-    if (hasDeterminateState()) {
-        LOG_WITH_STREAM(ContentObservation, stream << "stopObservingDOMTimerExecute: (" << &timer << ") in determined state.");
-        m_page.chrome().client().observedContentChange(m_page.mainFrame());
-        return;
-    }
-    if (WebCore::hasPendingStyleRecalc(m_page)) {
-        // An async style recalc has been scheduled. Let's observe it.
-        LOG_WITH_STREAM(ContentObservation, stream << "stopObservingDOMTimerExecute: (" << &timer << ") wait until next style recalc fires.");
-        setShouldObserveStyleRecalc(true);
-    }
+    m_isObservingContentChanges = false;
+    unregisterDOMTimer(timer);
+    setShouldObserveStyleRecalc(WebCore::hasPendingStyleRecalc(m_page));
+    notifyContentChangeIfNeeded();
 }
 
 void ContentChangeObserver::startObservingStyleRecalc()
@@ -129,19 +117,15 @@ void ContentChangeObserver::stopObservingStyleRecalc()
     LOG(ContentObservation, "stopObservingStyleRecalc: stop observing style recalc");
 
     setShouldObserveStyleRecalc(false);
-    if (hasDeterminateState()) {
-        LOG(ContentObservation, "stopObservingStyleRecalc: has determinate state, report content change.");
-        m_page.chrome().client().observedContentChange(m_page.mainFrame());
-        return;
-    }
-    LOG(ContentObservation, "stopObservingStyleRecalc: can't decide it yet.");
+    adjustObservedState(Event::StyleRecalcFinished);
+    notifyContentChangeIfNeeded();
 }
 
 void ContentChangeObserver::clearTimersAndReportContentChange()
 {
     if (!hasObservedDOMTimer())
         return;
-    LOG(ContentObservation, "clearTimersAndReportContentChange: remove registered timers and report content change.");
+    LOG_WITH_STREAM(ContentObservation, stream << "clearTimersAndReportContentChange: remove registered timers and report content change." << observedContentChange());
 
     clearObservedDOMTimers();
     m_page.chrome().client().observedContentChange(m_page.mainFrame());
@@ -157,18 +141,19 @@ void ContentChangeObserver::willDetachPage()
     clearTimersAndReportContentChange();
 }
 
-void ContentChangeObserver::didContentVisibilityChange()
+void ContentChangeObserver::contentVisibilityDidChange()
 {
-    setHasVisibleChangeState();
+    LOG(ContentObservation, "contentVisibilityDidChange: visible content change did happen.");
+    adjustObservedState(Event::ContentVisibilityChanged);
 }
 
 void ContentChangeObserver::startObservingContentChanges()
 {
     ASSERT(!hasPendingStyleRecalc(m_page));
-    startObservingDOMTimerScheduling();
-    setHasNoChangeState();
     clearObservedDOMTimers();
+    startObservingDOMTimerScheduling();
     m_isObservingContentChanges = true;
+    adjustObservedState(Event::ContentObservationStarted);
 }
 
 void ContentChangeObserver::stopObservingContentChanges()
@@ -182,12 +167,23 @@ WKContentChange ContentChangeObserver::observedContentChange() const
     return WKObservedContentChange();
 }
 
-void ContentChangeObserver::removeObservedDOMTimer(const DOMTimer& timer)
+void ContentChangeObserver::registerDOMTimer(const DOMTimer& timer)
+{
+    m_DOMTimerList.add(&timer);
+    adjustObservedState(Event::InstalledDOMTimer);
+}
+
+void ContentChangeObserver::unregisterDOMTimer(const DOMTimer& timer)
 {
     m_DOMTimerList.remove(&timer);
-    // Force reset the content change flag when the last observed content modifier is removed. We should not be in an indeterminate state anymore.
-    if (!hasObservedDOMTimer() && observedContentChange() == WKContentIndeterminateChange)
-        setHasNoChangeState();
+    adjustObservedState(Event::RemovedDOMTimer);
+}
+
+void ContentChangeObserver::setShouldObserveStyleRecalc(bool shouldObserve)
+{
+    if (shouldObserve)
+        LOG(ContentObservation, "Wait until next style recalc fires.");
+    m_shouldObserveStyleRecalc = shouldObserve;
 }
 
 bool ContentChangeObserver::hasDeterminateState() const
@@ -197,6 +193,40 @@ bool ContentChangeObserver::hasDeterminateState() const
     return observedContentChange() == WKContentNoChange && !hasObservedDOMTimer() && !hasPendingStyleRecalc(m_page);
 }
 
+void ContentChangeObserver::adjustObservedState(Event event)
+{
+    switch (event) {
+    case Event::ContentObservationStarted:
+        setHasNoChangeState();
+        break;
+    case Event::InstalledDOMTimer:
+        // Expecting a timer fire. Promote to an indeterminate state.
+        ASSERT(!hasVisibleChangeState());
+        setHasIndeterminateState();
+        break;
+    case Event::RemovedDOMTimer:
+    case Event::StyleRecalcFinished:
+        // Demote to "no change" when there's no pending activity anymore.
+        if (observedContentChange() == WKContentIndeterminateChange && !hasObservedDOMTimer() && !hasPendingStyleRecalc(m_page))
+            setHasNoChangeState();
+        break;
+    case Event::ContentVisibilityChanged:
+        setHasVisibleChangeState();
+        break;
+    }
+}
+
+void ContentChangeObserver::notifyContentChangeIfNeeded()
+{
+    if (!hasDeterminateState()) {
+        LOG(ContentObservation, "notifyContentChangeIfNeeded: not in a determined state yet.");
+        return;
+    }
+    LOG_WITH_STREAM(ContentObservation, stream << "notifyContentChangeIfNeeded: sending observedContentChange ->" << observedContentChange());
+    m_page.chrome().client().observedContentChange(m_page.mainFrame());
+}
+
+
 static Visibility elementImplicitVisibility(const Element& element)
 {
     auto* renderer = element.renderer();
@@ -254,7 +284,7 @@ ContentChangeObserver::StyleChangeScope::~StyleChangeScope()
     if ((m_previousDisplay == DisplayType::None && style->display() != DisplayType::None)
         || (m_previousVisibility == Visibility::Hidden && style->visibility() != Visibility::Hidden)
         || (m_previousImplicitVisibility == Visibility::Hidden && elementImplicitVisibility(m_element) == Visibility::Visible))
-        m_contentChangeObserver->didContentVisibilityChange();
+        m_contentChangeObserver->contentVisibilityDidChange();
 }
 
 ContentChangeObserver::StyleRecalcScope::StyleRecalcScope(Page* page)
index 4bb6268..5172ee0 100644 (file)
@@ -44,7 +44,7 @@ public:
 
     void didInstallDOMTimer(const DOMTimer&, Seconds timeout, bool singleShot);
     void didRemoveDOMTimer(const DOMTimer&);
-    void didContentVisibilityChange();
+    void contentVisibilityDidChange();
     void didSuspendActiveDOMObjects();
     void willDetachPage();
 
@@ -89,12 +89,12 @@ private:
     void startObservingStyleRecalc();
     void stopObservingStyleRecalc();
 
-    void addObservedDOMTimer(const DOMTimer& timer) { m_DOMTimerList.add(&timer); }
+    void registerDOMTimer(const DOMTimer&);
+    void unregisterDOMTimer(const DOMTimer&);
     bool isObservingDOMTimerScheduling() const { return m_isObservingDOMTimerScheduling; }
-    void removeObservedDOMTimer(const DOMTimer&);
     bool containsObservedDOMTimer(const DOMTimer& timer) const { return m_DOMTimerList.contains(&timer); }
 
-    void setShouldObserveStyleRecalc(bool shouldObserve) { m_shouldObserveStyleRecalc = shouldObserve; }
+    void setShouldObserveStyleRecalc(bool);
     bool shouldObserveStyleRecalc() const { return m_shouldObserveStyleRecalc; }
 
     bool isObservingContentChanges() const { return m_isObservingContentChanges; }
@@ -110,6 +110,17 @@ private:
     bool hasObservedDOMTimer() const { return !m_DOMTimerList.isEmpty(); }
     bool hasDeterminateState() const;
 
+    void notifyContentChangeIfNeeded();
+
+    enum class Event {
+        ContentObservationStarted,
+        InstalledDOMTimer,
+        RemovedDOMTimer,
+        StyleRecalcFinished,
+        ContentVisibilityChanged
+    };
+    void adjustObservedState(Event);
+
     Page& m_page;
     HashSet<const DOMTimer*> m_DOMTimerList;
     bool m_shouldObserveStyleRecalc { false };