|
|
Created:
6 years, 10 months ago by pkotwicz Modified:
6 years, 10 months ago CC:
chromium-reviews, sadrul, ben+aura_chromium.org, tfarina, kalyank Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix crash of OmniboxViewViewsTest.SelectAllOnClick on Linux Aura
XGrabPointer() generates EnterNotify/LeaveNotify events with the mouse cursor's current position which affects:
- aura::Env::last_mouse_location()
- The position of mouse events sent via ui_controls::SendMouseEventsNotifyWhenDone()
This CL moves the mouse cursor such that the EnterNotify/LeaveNotify events have the intended location (as opposed to the position of the mouse cursor before the test began).
BUG=163931
TEST=OmniboxViewViewsTest test suite passes
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252301
Patch Set 1 : #
Total comments: 2
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 37 (0 generated)
Sadrul, PTAL
https://codereview.chromium.org/149743006/diff/70001/ui/aura/root_window.cc File ui/aura/root_window.cc (right): https://codereview.chromium.org/149743006/diff/70001/ui/aura/root_window.cc#n... ui/aura/root_window.cc:719: SetLastMouseLocation(window(), point); This is a good fix. Mind having a test for this? https://codereview.chromium.org/149743006/diff/70001/ui/views/test/ui_control... File ui/views/test/ui_controls_factory_desktop_aurax11.cc (right): https://codereview.chromium.org/149743006/diff/70001/ui/views/test/ui_control... ui/views/test/ui_controls_factory_desktop_aurax11.cc:201: dispatcher->host()->PostNativeEvent(&xevent); We are potentially sending two mouse-move events here, right? From our offline discussion, it should work OK if we use dispatcher->MoveCursorTo() when button_down_mask == 0, and use XMotionEvent otherwise, right? Can we do that instead?
Sadrul, can you please take another look?
Ping?
Looks reasonable. But the test failures on chromeos look related?
Sadrul, can you please take another look? Yes those failures were related. They are now fixed - GetUIControlsAt() in ui_controls_factor_ash.cc does screen to root window coordinate conversion so UIControlsX11::SendMouseMoveNotifyWhenDone() takes in root window coordinates - RootWindow::MoveCursorTo() does not generate any events if the mouse is moved onto a hidden window. Hence the change in ViewEventTestBase
Ping?
A few nits. Otherwise, lgtm https://chromiumcodereview.appspot.com/149743006/diff/330001/chrome/test/base... File chrome/test/base/view_event_test_base.cc (right): https://chromiumcodereview.appspot.com/149743006/diff/330001/chrome/test/base... chrome/test/base/view_event_test_base.cc:131: context->GetDispatcher()->host()->Show(); This change doesn't look relevant (since this is for ash)? https://chromiumcodereview.appspot.com/149743006/diff/330001/ui/aura/root_win... File ui/aura/root_window_unittest.cc (right): https://chromiumcodereview.appspot.com/149743006/diff/330001/ui/aura/root_win... ui/aura/root_window_unittest.cc:1751: TEST_F(RootWindowTestRotated, BoundsChanged) { Could this be part of RootWindowTest, and you set the rotation on the screen at the start of the test? https://chromiumcodereview.appspot.com/149743006/diff/330001/ui/aura/test/ui_... File ui/aura/test/ui_controls_factory_aurax11.cc (right): https://chromiumcodereview.appspot.com/149743006/diff/330001/ui/aura/test/ui_... ui/aura/test/ui_controls_factory_aurax11.cc:142: long root_x, The desktop version calls this screen_x, screen_y (and the doc for UIControlsAura also claims x,y are in absolute screen coordinates). So let's call these screen_x|y and use the ScreenPositionClient (although I suppose in the case of chromeos it doesn't matter because the window is positioned at 0,0)?
https://chromiumcodereview.appspot.com/149743006/diff/330001/chrome/test/base... File chrome/test/base/view_event_test_base.cc (right): https://chromiumcodereview.appspot.com/149743006/diff/330001/chrome/test/base... chrome/test/base/view_event_test_base.cc:131: context->GetDispatcher()->host()->Show(); I am trying to keep ui_controls_factory_aurax11.cc and ui_controls_factory_desktop_aurax11.cc as similar to each other as possible so yes this is relevant (Notice the changes in ui_controls_factory_aurax11.cc) https://chromiumcodereview.appspot.com/149743006/diff/330001/ui/aura/root_win... File ui/aura/root_window_unittest.cc (right): https://chromiumcodereview.appspot.com/149743006/diff/330001/ui/aura/root_win... ui/aura/root_window_unittest.cc:1751: TEST_F(RootWindowTestRotated, BoundsChanged) { Ok https://chromiumcodereview.appspot.com/149743006/diff/330001/ui/aura/test/ui_... File ui/aura/test/ui_controls_factory_aurax11.cc (right): https://chromiumcodereview.appspot.com/149743006/diff/330001/ui/aura/test/ui_... ui/aura/test/ui_controls_factory_aurax11.cc:142: long root_x, Root window coordinates are passed in. The documentation is wrong. Notice GetUIControlsAt() in ui_controls_factory_ash.cc I agree that this is confusing. I will fix this as part of a separate CL. (Separate CL because I need to test this on Windows too)
Scott, can you please take a look at the change in chrome/test/base/view_event_test_base.cc
LGTM
The CQ bit was checked by pkotwicz@chromium.org
The CQ bit was unchecked by pkotwicz@chromium.org
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/149743006/610001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/149743006/900001
The CQ bit was unchecked by pkotwicz@chromium.org
The CQ bit was checked by pkotwicz@chromium.org
Failed to get patchset properties (patchset not found?)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/149743006/620007
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/149743006/620007
The CQ bit was unchecked by pkotwicz@chromium.org
The CQ bit was checked by pkotwicz@chromium.org
The CQ bit was unchecked by pkotwicz@chromium.org
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/149743006/1150001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/149743006/1150001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/149743006/1150001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/149743006/1150001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/149743006/1150001
Message was sent while issue was closed.
Change committed as 252301 |