WebKit Bugzilla
Attachment 346088 Details for
Bug 188144
: REGRESSION (r230817): Terrible performance when selecting text on Stash code review
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Address review feedback.
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(); > }
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 188144
:
346018
|
346024
|
346045
|
346047
|
346088
|
346091