Attachment 346088 Details for Bug 188144 – Address review feedback.

[patch] Address review feedback.

bug-188144-20180730125015.patch (text/plain), 7.44 KB, created by Wenson Hsieh on 2018-07-30 12:50:16 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2018-07-30 12:50:16 PDT

Size: 7.44 KB

patch

obsolete

>Subversion Revision: 234364
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 1a2af9c7f5c4fe3b005c4f78af1ded1b84ddaae4..f31a14d53ab1c38e69f6cd53daa662158b5b8e2a 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,18 @@
>+2018-07-30  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        REGRESSION (r230817): Terrible performance when selecting text on Stash code review
>+        https://bugs.webkit.org/show_bug.cgi?id=188144
>+        <rdar://problem/42642489>
>+
>+        Reviewed by Darin Adler.
>+
>+        Make it possible to remove a `reverse_iterator` from a `Deque`. To do this, we make the existing `remove`
>+        functions take const lvalue references instead of non-const references; this allows us to pass
>+        `reverse_iterator::base()` into `Deque::remove()`.
>+
>+        * wtf/Deque.h:
>+        (WTF::inlineCapacity>::remove):
>+
> 2018-07-28  Mark Lam  <mark.lam@apple.com>
> 
>         Gardening: build fix for internal builds.
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index f051f4140704cc10e86e403fa08b88af89fda0a9..3c7b284008fec94695552511a8aa2cddc6a09234 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,37 @@
>+2018-07-30  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        REGRESSION (r230817): Terrible performance when selecting text on Stash code review
>+        https://bugs.webkit.org/show_bug.cgi?id=188144
>+        <rdar://problem/42642489>
>+
>+        Reviewed by Darin Adler.
>+
>+        After r230817, mouse events were serially dispatched to the web process and handled before the subsequent mouse
>+        event. However, this resulted in rapid-fire mouse move events filling up the mouse event queue in the case where
>+        mouse move events were being handled by the web process at a slower rate than the UI process was enqueueing
>+        them. To mitigate this, r231511 introduced a mechanism for replacing the most recently enqueued mouse move event
>+        with an incoming mouse move event.
>+
>+        However, when a user with a force-click-enabled trackpad performs a mouse drag, a rapid stream of
>+        "mouseforcechanged" events is interleaved alongside the stream of "mousemove" events. This renders r231511
>+        ineffective, since the most recently queued event is often a "mouseforcechanged" event instead of a "mousemove".
>+        On the stash code review page, this can result in hundreds of mouse events being backed up in the mouse event
>+        queue, causing perceived slowness when selecting text.
>+
>+        To fix this, we extend the mechanism introduced in r231511, such that it is capable of replacing both
>+        "mouseforcechanged" and "mousemove" events in the queue. Rather than consider only the most recently queued
>+        item, we instead find the most recently queued event that matches the type of the incoming event, remove it from
>+        the queue, and then append the incoming event to the end of the queue. To avoid the risk of removing the only
>+        "mousemove" or "mouseforcechanged" event in the middle of a mouse down and mouse up, we also bail when searching
>+        backwards for an event to replace if we come across any event that is neither of these types.
>+
>+        This effectively throttles the rate at which mouseforcechanged or mousemove events are dispatched when a user
>+        with force-click-enabled hardware clicks and drags the mouse across the page.
>+
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::removeOldRedundantEvent):
>+        (WebKit::WebPageProxy::handleMouseEvent):
>+
> 2018-07-29  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         Fix incorrect guards around a method declaration in PageClient.h
>diff --git a/Source/WTF/wtf/Deque.h b/Source/WTF/wtf/Deque.h
>index 25e9c8421108a78b559b87aa8acf503b4edac888..e11bb3ceb66515150ceee0eaa97e2714b336eb71 100644
>--- a/Source/WTF/wtf/Deque.h
>+++ b/Source/WTF/wtf/Deque.h
>@@ -91,8 +91,8 @@ public:
>     template<typename U> void prepend(U&&);
>     void removeFirst();
>     void removeLast();
>-    void remove(iterator&);
>-    void remove(const_iterator&);
>+    void remove(const iterator&);
>+    void remove(const const_iterator&);
>     
>     template<typename Func> void removeAllMatching(const Func&);
>     
>@@ -521,14 +521,14 @@ inline void Deque<T, inlineCapacity>::removeLast()
> }
> 
> template<typename T, size_t inlineCapacity>
>-inline void Deque<T, inlineCapacity>::remove(iterator& it)
>+inline void Deque<T, inlineCapacity>::remove(const iterator& it)
> {
>     it.checkValidity();
>     remove(it.m_index);
> }
> 
> template<typename T, size_t inlineCapacity>
>-inline void Deque<T, inlineCapacity>::remove(const_iterator& it)
>+inline void Deque<T, inlineCapacity>::remove(const const_iterator& it)
> {
>     it.checkValidity();
>     remove(it.m_index);
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 70d58330259c65b9e006893891f81e96f1bf33a5..97f8a0729d399721455e48e0b25af5006f82994c 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -1945,21 +1945,47 @@ void WebPageProxy::setDragCaretRect(const IntRect& dragCaretRect)
> 
> #endif // ENABLE(DRAG_SUPPORT)
> 
>+static bool removeOldRedundantEvent(Deque<NativeWebMouseEvent>& queue, WebEvent::Type incomingEventType)
>+{
>+    if (incomingEventType != WebEvent::MouseMove && incomingEventType != WebEvent::MouseForceChanged)
>+        return false;
>+
>+    auto it = queue.rbegin();
>+    auto end = queue.rend();
>+
>+    // Must not remove the first event in the deque, since it is already being dispatched.
>+    if (it != end)
>+        --end;
>+
>+    for (; it != end; ++it) {
>+        auto type = it->type();
>+        if (type == incomingEventType) {
>+            queue.remove(--it.base());
>+            return true;
>+        }
>+        if (type != WebEvent::MouseMove && type != WebEvent::MouseForceChanged)
>+            break;
>+    }
>+    return false;
>+}
>+
> void WebPageProxy::handleMouseEvent(const NativeWebMouseEvent& event)
> {
>     if (!isValid())
>         return;
> 
>-    // If we receive multiple mousemove events and the most recent mousemove event has not been
>-    // sent to WebProcess for processing, replace the pending mousemove event with a new one.
>-    if (event.type() == WebEvent::MouseMove && m_mouseEventQueue.size() > 1 && m_mouseEventQueue.last().type() == WebEvent::MouseMove) {
>-        LOG(MouseHandling, "UIProcess: updated pending mousemove event (queue size %zu)", m_mouseEventQueue.size());
>-        m_mouseEventQueue.removeLast();
>-        m_mouseEventQueue.append(event);
>-    } else {
>-        LOG(MouseHandling, "UIProcess: enqueued mouse event %s (queue size %zu)", webMouseEventTypeString(event.type()), m_mouseEventQueue.size());
>-        m_mouseEventQueue.append(event);
>-    }
>+    // If we receive multiple mousemove or mouseforcechanged events and the most recent mousemove or mouseforcechanged event
>+    // (respectively) has not yet been sent to WebProcess for processing, remove the pending mouse event and insert the new
>+    // event in the queue.
>+    bool didRemoveEvent = removeOldRedundantEvent(m_mouseEventQueue, event.type());
>+    m_mouseEventQueue.append(event);
>+
>+#if LOG_DISABLED
>+    UNUSED_PARAM(didRemoveEvent);
>+#else
>+    LOG(MouseHandling, "UIProcess: %s mouse event %s (queue size %zu)", didRemoveEvent ? "replaced" : "enqueued", webMouseEventTypeString(event.type()), m_mouseEventQueue.size());
>+#endif
>+
>     if (m_mouseEventQueue.size() == 1) // Otherwise, called from DidReceiveEvent message handler.
>         processNextQueuedMouseEvent();
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 188144: 346018 | 346024 | 346045 | 346047 | 346088 | 346091