Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1670473002:
MacViews: Fix views_unittests MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActiv… (Closed)
DescriptionMacViews: Fix views_unittests MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because a
non-simulated cursor position is detected during button creation, where button
states are initialized, thus not detecting a hover state.
the ui::test::EventGenerator is used to simulate cursor position, but isn't
created until after the button widget, which means an unexpected cursor position
is detected between the time of widget creation and EventGenerator creation.
Additionally, using the set_current_location() method does not trigger mouse
events, which means the button's hover state is never detected.
Fix is to create the EventGenerator object before button creation so that
when mouse events are triggered the hit tests will detect mouse hover states.
BUG=579380
Committed: https://crrev.com/1ddeaf4f19e4c98b7f489746bfdec7cf90218883
Cr-Commit-Position: refs/heads/master@{#376101}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review comments. #
Total comments: 9
Patch Set 3 : Treat scoped_ptrs properly. #Patch Set 4 : Rebase, set cursor location consistently. #Patch Set 5 : Make EventGenerator private. #Messages
Total messages: 28 (11 generated)
Description was changed from
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, fail on Mac because the cursor position is detected
incorrectly. Though the ui::test::EventGenerator is used to simulate cursor
position, it isn't created until after the button widget is created,
which means an unexpected cursor position (whereever the cursor happens to
be while the test is running) is detected between the time of widget creation
and the time the EventGenerator is created. Using the set_current_location()
method also does not trigger any mouse events, which means the button's hover
state is never detected.
Fix is to create the EventGenerator object before the test itself so that
when mouse events are triggered the hit tests will detect mouse hover.
BUG=579380
==========
to
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, fail on Mac because the cursor position is detected
incorrectly. Though the ui::test::EventGenerator is used to simulate cursor
position, it isn't created until after the button widget is created,
which means an unexpected cursor position (whereever the cursor happens to
be while the test is running) is detected between the time of widget creation
and the time the EventGenerator is created. Using the set_current_location()
method also does not trigger any mouse events, which means the button's hover
state is never detected.
Fix is to create the EventGenerator object before the test itself so that
when mouse events are triggered the hit tests will detect mouse hover.
BUG=579380
==========
patricialor@chromium.org changed reviewers: + tapted@chromium.org
PTAL, I just moved the EventGenerators used for most (all?) tests into the MenuButtonTest object.
nice work! just nits really. https://codereview.chromium.org/1670473002/diff/1/ui/views/controls/button/me... File ui/views/controls/button/menu_button_unittest.cc (right): https://codereview.chromium.org/1670473002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button_unittest.cc:89: ui::test::EventGenerator* generator_; this should be scoped_ptr. https://codereview.chromium.org/1670473002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button_unittest.cc:90: }; nit: there should be a DISALLOW_COPY_AND_ASSIGN(MenuButtonTest) here
oh and have the tests always failed since they were added in r301015? CL description should say that, e.g. .. added in r301015 and have always failed on Mac.
Description was changed from
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, fail on Mac because the cursor position is detected
incorrectly. Though the ui::test::EventGenerator is used to simulate cursor
position, it isn't created until after the button widget is created,
which means an unexpected cursor position (whereever the cursor happens to
be while the test is running) is detected between the time of widget creation
and the time the EventGenerator is created. Using the set_current_location()
method also does not trigger any mouse events, which means the button's hover
state is never detected.
Fix is to create the EventGenerator object before the test itself so that
when mouse events are triggered the hit tests will detect mouse hover.
BUG=579380
==========
to
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because the
cursor position is detected incorrectly. Though the ui::test::EventGenerator is
used to simulate cursor position, it isn't created until after the button widget
is created, which means an unexpected cursor position (whereever the cursor
happens to be while the test is running) is detected between the time of widget
creation and the time the EventGenerator is created. Using the
set_current_location() method also does not trigger any mouse events, which
means the button's hover state is never detected.
Fix is to create the EventGenerator object before the test itself so that
when mouse events are triggered the hit tests will detect mouse hover.
BUG=579380
==========
Did a bisect and it appears to have regressed about 2 months ago in r363547, which I have added to the description. https://codereview.chromium.org/1670473002/diff/1/ui/views/controls/button/me... File ui/views/controls/button/menu_button_unittest.cc (right): https://codereview.chromium.org/1670473002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button_unittest.cc:89: ui::test::EventGenerator* generator_; On 2016/02/04 23:27:09, tapted wrote: > this should be scoped_ptr. Done. https://codereview.chromium.org/1670473002/diff/1/ui/views/controls/button/me... ui/views/controls/button/menu_button_unittest.cc:90: }; On 2016/02/04 23:27:08, tapted wrote: > nit: there should be a DISALLOW_COPY_AND_ASSIGN(MenuButtonTest) here Done.
It's still hard to draw the link between the failures and the fix. I think we know: (1) set_mouse_location(..) not generating mouse events (2) On Mac, mouse location reported according to present cursor position on the test machine until the EventGenerator has been created. (3) r363547 -> https://codereview.chromium.org/1495973002 not altering logic to have more dependence on HOVER (1) and (2) aren't enough to justify this as a good fix, since set_mouse_location still doesn't generate events, even with the generator constructed sooner. Then, adding (3), it still needs a leap of logic. I *think* what now happens is that by constructing the EventGenerator _before_ the menu button, the menu button initialization paths are now able to retrieve a mouse location to `bootstrap` how the menu button should look in a consistent manner. i.e. On other platforms it was "accidentally" consistent because the cursor location is always simulated, and just happens to be in the right/wrong location to configure the expected initial state of the button. On mac it's not simulated from the start, so it can be flaky. If that's true, then I think we should also move all the initial set_mouse_location calls to just after the generator is created. Then put a comment above that line which says // Set the initial mouse location in a consistent way so that the menu button we are about to create initializes its hover state in a consistent manner. https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/menu_button_unittest.cc (right): https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:29: MenuButtonTest() : generator(nullptr), widget_(nullptr), button_(nullptr) {} nit: doesn't need an initializer now that it's a scoped_ptr https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:33: if (widget_ && !widget_->IsClosed()) nit: although it's not crashing.... since the generator is constructed with the Widget's window, and the window is about to be destroyed, we should reset the generator before this to properly preserve nested object lifetimes. https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:60: scoped_ptr<ui::test::EventGenerator> generator; generator -> generator_ also, this should go back to private: so that it's consistent with widget() and button() (test harnesses are allowed to have protected members, but consistency trumps quite a lot in the style guide) https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:66: generator = make_scoped_ptr(new ui::test::EventGenerator(GetContext(), nit: make_scoped_ptr is used mostly after a `return`. If you have the scoped_ptr itself, it's more conventional to do generator_.reset(new ...)
btw there's some stuff in https://codereview.chromium.org/1682603002/ that will probably merge-conflict but that hasn't landed yet
Have moved the simulated cursor position to coords at 10,10 after the event generator is created (it actually defaults to 100,100 as this is the centre of the button created). https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/menu_button_unittest.cc (right): https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:29: MenuButtonTest() : generator(nullptr), widget_(nullptr), button_(nullptr) {} On 2016/02/09 05:06:41, tapted wrote: > nit: doesn't need an initializer now that it's a scoped_ptr Done. https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:33: if (widget_ && !widget_->IsClosed()) On 2016/02/09 05:06:41, tapted wrote: > nit: although it's not crashing.... since the generator is constructed with the > Widget's window, and the window is about to be destroyed, we should reset the > generator before this to properly preserve nested object lifetimes. Done. https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:60: scoped_ptr<ui::test::EventGenerator> generator; On 2016/02/09 05:06:41, tapted wrote: > generator -> generator_ > > also, this should go back to private: so that it's consistent with widget() and > button() > > (test harnesses are allowed to have protected members, but consistency trumps > quite a lot in the style guide) Having the generator scoped_ptr be private means I can't return it from a getter function (can't move/copy scoped_ptrs). https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:66: generator = make_scoped_ptr(new ui::test::EventGenerator(GetContext(), On 2016/02/09 05:06:41, tapted wrote: > nit: make_scoped_ptr is used mostly after a `return`. If you have the scoped_ptr > itself, it's more conventional to do > > generator_.reset(new ...) Done.
https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/menu_button_unittest.cc (right): https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:60: scoped_ptr<ui::test::EventGenerator> generator; On 2016/02/12 04:42:20, Patti Lor wrote: > On 2016/02/09 05:06:41, tapted wrote: > > generator -> generator_ > > > > also, this should go back to private: so that it's consistent with widget() > and > > button() > > > > (test harnesses are allowed to have protected members, but consistency trumps > > quite a lot in the style guide) > > Having the generator scoped_ptr be private means I can't return it from a getter > function (can't move/copy scoped_ptrs). returning generator_.get() is fine for this kind of stuff :)
On 2016/02/12 04:44:54, tapted wrote: > https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... > File ui/views/controls/button/menu_button_unittest.cc (right): > > https://codereview.chromium.org/1670473002/diff/20001/ui/views/controls/butto... > ui/views/controls/button/menu_button_unittest.cc:60: > scoped_ptr<ui::test::EventGenerator> generator; > On 2016/02/12 04:42:20, Patti Lor wrote: > > On 2016/02/09 05:06:41, tapted wrote: > > > generator -> generator_ > > > > > > also, this should go back to private: so that it's consistent with widget() > > and > > > button() > > > > > > (test harnesses are allowed to have protected members, but consistency > trumps > > > quite a lot in the style guide) > > > > Having the generator scoped_ptr be private means I can't return it from a > getter > > function (can't move/copy scoped_ptrs). > > returning generator_.get() is fine for this kind of stuff :) Ah, ok. Thank you, done! :)
lgtm, just the CL description needs an update to match the theory that the failures were due to the state-initialization that was being done during button creation.
Description was changed from
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because the
cursor position is detected incorrectly. Though the ui::test::EventGenerator is
used to simulate cursor position, it isn't created until after the button widget
is created, which means an unexpected cursor position (whereever the cursor
happens to be while the test is running) is detected between the time of widget
creation and the time the EventGenerator is created. Using the
set_current_location() method also does not trigger any mouse events, which
means the button's hover state is never detected.
Fix is to create the EventGenerator object before the test itself so that
when mouse events are triggered the hit tests will detect mouse hover.
BUG=579380
==========
to
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because a
non-simulated cursor position is detected during button creation, where button
states are initialized, thus not detecting a hover state.
the ui::test::EventGenerator is used to simulate cursor position, but isn't
created until after the button widget, which means an unexpected cursor position
is detected between the time of widget creation and EventGenerator creation.
Additionally, using the set_current_location() method does not trigger mouse
events, which means the button's hover state is never detected.
Fix is to create the EventGenerator object before button creation so that
when mouse events are triggered the hit tests will detect mouse hover states.
BUG=579380
==========
The CQ bit was checked by patricialor@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1670473002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670473002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because a
non-simulated cursor position is detected during button creation, where button
states are initialized, thus not detecting a hover state.
the ui::test::EventGenerator is used to simulate cursor position, but isn't
created until after the button widget, which means an unexpected cursor position
is detected between the time of widget creation and EventGenerator creation.
Additionally, using the set_current_location() method does not trigger mouse
events, which means the button's hover state is never detected.
Fix is to create the EventGenerator object before button creation so that
when mouse events are triggered the hit tests will detect mouse hover states.
BUG=579380
==========
to
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because a
non-simulated cursor position is detected during button creation, where button
states are initialized, thus not detecting a hover state.
the ui::test::EventGenerator is used to simulate cursor position, but isn't
created until after the button widget, which means an unexpected cursor position
is detected between the time of widget creation and EventGenerator creation.
Additionally, using the set_current_location() method does not trigger mouse
events, which means the button's hover state is never detected.
Fix is to create the EventGenerator object before button creation so that
when mouse events are triggered the hit tests will detect mouse hover states.
BUG=579380
==========
patricialor@chromium.org changed reviewers: + sky@chromium.org
PTAL, thank you!
LGTM
The CQ bit was checked by patricialor@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1670473002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1670473002/80001
Message was sent while issue was closed.
Description was changed from
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because a
non-simulated cursor position is detected during button creation, where button
states are initialized, thus not detecting a hover state.
the ui::test::EventGenerator is used to simulate cursor position, but isn't
created until after the button widget, which means an unexpected cursor position
is detected between the time of widget creation and EventGenerator creation.
Additionally, using the set_current_location() method does not trigger mouse
events, which means the button's hover state is never detected.
Fix is to create the EventGenerator object before button creation so that
when mouse events are triggered the hit tests will detect mouse hover states.
BUG=579380
==========
to
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because a
non-simulated cursor position is detected during button creation, where button
states are initialized, thus not detecting a hover state.
the ui::test::EventGenerator is used to simulate cursor position, but isn't
created until after the button widget, which means an unexpected cursor position
is detected between the time of widget creation and EventGenerator creation.
Additionally, using the set_current_location() method does not trigger mouse
events, which means the button's hover state is never detected.
Fix is to create the EventGenerator object before button creation so that
when mouse events are triggered the hit tests will detect mouse hover states.
BUG=579380
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because a
non-simulated cursor position is detected during button creation, where button
states are initialized, thus not detecting a hover state.
the ui::test::EventGenerator is used to simulate cursor position, but isn't
created until after the button widget, which means an unexpected cursor position
is detected between the time of widget creation and EventGenerator creation.
Additionally, using the set_current_location() method does not trigger mouse
events, which means the button's hover state is never detected.
Fix is to create the EventGenerator object before button creation so that
when mouse events are triggered the hit tests will detect mouse hover states.
BUG=579380
==========
to
==========
MacViews: Fix views_unittests
MenuButtonTest.{ActivateDropDownOnMouseClick,DraggableMenuButtonActivatesOnRelease}
Tests added in r301015, regressed in r363547. They fail on Mac because a
non-simulated cursor position is detected during button creation, where button
states are initialized, thus not detecting a hover state.
the ui::test::EventGenerator is used to simulate cursor position, but isn't
created until after the button widget, which means an unexpected cursor position
is detected between the time of widget creation and EventGenerator creation.
Additionally, using the set_current_location() method does not trigger mouse
events, which means the button's hover state is never detected.
Fix is to create the EventGenerator object before button creation so that
when mouse events are triggered the hit tests will detect mouse hover states.
BUG=579380
Committed: https://crrev.com/1ddeaf4f19e4c98b7f489746bfdec7cf90218883
Cr-Commit-Position: refs/heads/master@{#376101}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1ddeaf4f19e4c98b7f489746bfdec7cf90218883 Cr-Commit-Position: refs/heads/master@{#376101} |
