Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2386103004:
Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. (Closed)
|
Created:
4 years, 2 months ago by ananta Modified:
4 years, 2 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure that we don't report huge mouse movement deltas for mouse enter and leave events.
The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function.
However this function does not do the right thing because it checks the WebMouseEvent structure for a
mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they
are converted to a mouse move event with the cursor position as the location.
Fixes as below:-
1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this
to check if the mouse event is a mouse enter or leave.
BUG=650787
TBR=jam
Committed: https://crrev.com/116116ab28e3d6f1c9320f921e77e084426588ca
Cr-Commit-Position: refs/heads/master@{#423977}
Patch Set 1 #Patch Set 2 : Update comment #
Total comments: 2
Patch Set 3 : Don't update the location for mouse move events originating from the mouse enter and leave events. #
Total comments: 6
Patch Set 4 : Make args const reference. #Patch Set 5 : Make args const reference. #
Total comments: 2
Patch Set 6 : Address sky review comments #
Messages
Total messages: 49 (25 generated)
ananta@chromium.org changed reviewers: + scheib@chromium.org
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ananta@chromium.org changed reviewers: + jam@chromium.org
jam@chromium.org changed reviewers: + sky@chromium.org
+sky would be a better reviewer than me. I can rs after
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Logic fix seems to look good to me, though I have a question about the ordering assertion (inline). I wish we had unit tests that could recreate the ordering and contents from Windows we're getting -- no such infrastructure currently? https://codereview.chromium.org/2386103004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2386103004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1822: if (event->type() == ui::ET_MOUSE_MOVED) { It seems correct only if a move event will always arrive before an ENTERED/EXITED. Anything we can say in comments here to explain why we think this assertion is true?
https://codereview.chromium.org/2386103004/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2386103004/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1822: if (event->type() == ui::ET_MOUSE_MOVED) { On 2016/10/04 02:01:34, scheib OOO wrote: > It seems correct only if a move event will always arrive before an > ENTERED/EXITED. Anything we can say in comments here to explain why we think > this assertion is true? On Windows, a visible window can safely assume that a mouse move will be given to it for setting the mouse cursor. Given that here we are looking for a mouse enter or leave, it can be safely assumed that these should be preceded with mouse moves.
On 2016/10/04 02:21:20, ananta wrote: > https://codereview.chromium.org/2386103004/diff/20001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/2386103004/diff/20001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_aura.cc:1822: if > (event->type() == ui::ET_MOUSE_MOVED) { > On 2016/10/04 02:01:34, scheib OOO wrote: > > It seems correct only if a move event will always arrive before an > > ENTERED/EXITED. Anything we can say in comments here to explain why we think > > this assertion is true? > > On Windows, a visible window can safely assume that a mouse move will be given > to it for setting the mouse cursor. Given that here we are looking for a mouse > enter or leave, it can be safely assumed that these should be preceded with > mouse moves. I am looking at the interactive_ui_tests redness. Please continue reviewing.
Description was changed from ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. On Windows, the WM_MOUSEENTER and WM_MOUSELEAVE messages don't have mouse coordinates associated with them. We set the event locations to the current cursor position. When these events eventually come in RWHVA we convert these events to a mouse move which is sent to the renderer along with the bad location values. Additionally the movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing becaus it checks WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure always changes the event type for mouse enter and leave to a mouse move. Fixes as below:- 1. Remember the last mouse move location in RWHVA. When we receive a mouse enter or leave, set the location to this mouse move. 3. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 ========== to ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 ==========
Description was changed from ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 ========== to ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 ==========
Description was changed from ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 ========== to ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 ==========
On 2016/10/04 02:23:59, ananta wrote: > On 2016/10/04 02:21:20, ananta wrote: > > > https://codereview.chromium.org/2386103004/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > > > > https://codereview.chromium.org/2386103004/diff/20001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_view_aura.cc:1822: if > > (event->type() == ui::ET_MOUSE_MOVED) { > > On 2016/10/04 02:01:34, scheib OOO wrote: > > > It seems correct only if a move event will always arrive before an > > > ENTERED/EXITED. Anything we can say in comments here to explain why we think > > > this assertion is true? > > > > On Windows, a visible window can safely assume that a mouse move will be given > > to it for setting the mouse cursor. Given that here we are looking for a mouse > > enter or leave, it can be safely assumed that these should be preceded with > > mouse moves. > > I am looking at the interactive_ui_tests redness. Please continue reviewing. The change to update the location of mouse move events originating from a mouse leave or enter is unnecessary and is incorrect as it interferes the code in blink which uses the location to invoke the js onmouseout functions. Turns out that the fix to RWHVA::ModifyEventMovementAndCoords is sufficient for this problem.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2509: global_mouse_position_.SetPoint(event->globalX, event->globalY); This is the first I've seen of ModifyEventMovementAndCoords. Can you elaborate on why we need global_mouse_position_ per RWHVA? aura::Env has a last_mouse_location, can that be used? Screen also has GetCursorScreenPoint(), which uses GetCursorPos() where as last_mouse_location() is the last mouse location seen in chrome.
https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2509: global_mouse_position_.SetPoint(event->globalX, event->globalY); On 2016/10/04 18:17:55, sky wrote: > This is the first I've seen of ModifyEventMovementAndCoords. Can you elaborate > on why we need global_mouse_position_ per RWHVA? aura::Env has a > last_mouse_location, can that be used? Screen also has GetCursorScreenPoint(), > which uses GetCursorPos() where as last_mouse_location() is the last mouse > location seen in chrome. The global_mouse_position technically can be possibly replaced with the aura global. I would prefer to do that in a separate patch.
https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1320: // ModifyEventMovementAndCoords function. If you update aura::Env and nuke global_mouse_position_ does that means this goes away? I'm a bit confused by some of this code and would like to understand why it is the way it is rather than working around it.
https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1320: // ModifyEventMovementAndCoords function. On 2016/10/05 15:49:55, sky wrote: > If you update aura::Env and nuke global_mouse_position_ does that means this > goes away? I'm a bit confused by some of this code and would like to understand > why it is the way it is rather than working around it. It looks like the mouse events sent to blink require the movement X and Y deltas which are computed via the difference between the global mouse position in the current mouse event and its last position, which is saved in global_mouse_position_. When the mouse lock state is reset we force the cursor back to where it was originally placed which is represented by the unlock_global_mouse_position_ member. Removing global_mouse_position_ while good, is sadly not going to get rid of this code here and the modify mouse event function.
Ok, I think I get this, thanks for clarification. Is it possible to write test coverage? https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2504: ui::MouseEvent* ui_mouse_event) { Make this take a const&, in fact can both args be const& ?
https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2386103004/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2504: ui::MouseEvent* ui_mouse_event) { On 2016/10/06 16:33:36, sky wrote: > Make this take a const&, in fact can both args be const& ? Changed ui_mouse_event to be a const ref. The other one can't be as it is updated below. Regarding tests, it may be a touch tricky to write a non flaky one, because the only way I could get this to reliably repro is to hover the mouse right around the edge of the window border. Will see if an interactive test is doable in a future patchset.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Logic LGTM, tests ... it's unfortunate we don't have a way to unit_test this layer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2386103004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2386103004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.h:430: const ui::MouseEvent& ui_mouse_event); |event| should be last (style guide says out params last).
https://codereview.chromium.org/2386103004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/2386103004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.h:430: const ui::MouseEvent& ui_mouse_event); On 2016/10/06 22:38:48, sky wrote: > |event| should be last (style guide says out params last). Done.
LGTM
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/2386103004/#ps100001 (title: "Address sky review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 ========== to ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 TBR=jam ==========
TBR'ing jam
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 TBR=jam ========== to ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 TBR=jam ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 TBR=jam ========== to ========== Ensure that we don't report huge mouse movement deltas for mouse enter and leave events. The movement deltas are supposed to be fixed by the RWHVA::ModifyEventMovementAndCoords function. However this function does not do the right thing because it checks the WebMouseEvent structure for a mouse enter or leave event. The WebMouseEvent structure never sees a mouse enter or leave event as they are converted to a mouse move event with the cursor position as the location. Fixes as below:- 1. Pass the ui::MouseEvent structure to the RWHVA::ModifyEventMovementAndCoords function and use this to check if the mouse event is a mouse enter or leave. BUG=650787 TBR=jam Committed: https://crrev.com/116116ab28e3d6f1c9320f921e77e084426588ca Cr-Commit-Position: refs/heads/master@{#423977} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/116116ab28e3d6f1c9320f921e77e084426588ca Cr-Commit-Position: refs/heads/master@{#423977}
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
