New issue
Advanced search Search tips

Issue 901183 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocked on:
issue 904366



Sign in to add a comment

Event Handling extraction from Button class into separate class

Project Member Reported by cyan@chromium.org, Nov 2

Issue description

Project Member

Comment 1 by bugdroid1@chromium.org, Nov 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3275875871d7e5dd239acbe5ac6545d51a25cb3d

commit 3275875871d7e5dd239acbe5ac6545d51a25cb3d
Author: Charlene Yan <cyan@chromium.org>
Date: Sat Nov 10 01:03:54 2018

Remove menu_marker from MenuButton.

This is always false except for in tests and menu_example.cc which is
not used. This helps simplify MenuButton for the Button cleanup task.

Bug: 901183
Change-Id: If5a3cf7b6ba1ce5618548d112df1c4404b2ae181
Reviewed-on: https://chromium-review.googlesource.com/c/1324375
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Charlene Yan <cyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607061}
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/ash/shelf/login_shelf_view.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/chrome/browser/ui/views/frame/app_menu_button.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/chrome/browser/ui/views/hover_button.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/chrome/browser/ui/views/menu_model_adapter_test.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/chrome/browser/ui/views/menu_test_base.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/chrome/browser/ui/views/page_info/permission_selector_row.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/chrome/browser/ui/views/tab_icon_view.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/chrome/browser/ui/views/toolbar/toolbar_action_view_unittest.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/tools/gritsettings/startup_resources_win.txt
[delete] https://crrev.com/d1ed027ee25d2b4f4de591bdce93bcf8dc651ba5/ui/resources/default_100_percent/cros/menu_droparrow.png
[delete] https://crrev.com/d1ed027ee25d2b4f4de591bdce93bcf8dc651ba5/ui/resources/default_200_percent/cros/menu_droparrow.png
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/ui/resources/ui_resources.grd
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/ui/views/controls/button/button_unittest.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/ui/views/controls/button/menu_button.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/ui/views/controls/button/menu_button.h
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/ui/views/controls/button/menu_button_unittest.cc
[modify] https://crrev.com/3275875871d7e5dd239acbe5ac6545d51a25cb3d/ui/views/examples/menu_example.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af

commit 5148d1fe59a144bc7eba073d4f30e4eeefa8b9af
Author: John Budorick <jbudorick@chromium.org>
Date: Sat Nov 10 01:37:07 2018

Revert "Remove menu_marker from MenuButton."

This reverts commit 3275875871d7e5dd239acbe5ac6545d51a25cb3d.

Reason for revert: causes compile failures on windows similar to the ones that appeared in the CQ runs, e.g. https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win%20x64%20Builder/58688

Original change's description:
> Remove menu_marker from MenuButton.
> 
> This is always false except for in tests and menu_example.cc which is
> not used. This helps simplify MenuButton for the Button cleanup task.
> 
> Bug: 901183
> Change-Id: If5a3cf7b6ba1ce5618548d112df1c4404b2ae181
> Reviewed-on: https://chromium-review.googlesource.com/c/1324375
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Reviewed-by: Robert Flack <flackr@chromium.org>
> Reviewed-by: Trent Apted <tapted@chromium.org>
> Reviewed-by: Michael Wasserman <msw@chromium.org>
> Commit-Queue: Charlene Yan <cyan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607061}

TBR=msw@chromium.org,flackr@chromium.org,oshima@chromium.org,tapted@chromium.org,cyan@chromium.org

Change-Id: I9c311a3c8cea56c7670b2dace3e0410618151034
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 901183
Reviewed-on: https://chromium-review.googlesource.com/c/1330841
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607072}
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/ash/shelf/login_shelf_view.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/chrome/browser/ui/views/frame/app_menu_button.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/chrome/browser/ui/views/hover_button.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/chrome/browser/ui/views/menu_model_adapter_test.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/chrome/browser/ui/views/menu_test_base.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/chrome/browser/ui/views/page_info/permission_selector_row.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/chrome/browser/ui/views/tab_icon_view.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/chrome/browser/ui/views/toolbar/toolbar_action_view_unittest.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/tools/gritsettings/startup_resources_win.txt
[add] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/ui/resources/default_100_percent/cros/menu_droparrow.png
[add] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/ui/resources/default_200_percent/cros/menu_droparrow.png
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/ui/resources/ui_resources.grd
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/ui/views/controls/button/button_unittest.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/ui/views/controls/button/menu_button.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/ui/views/controls/button/menu_button.h
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/ui/views/controls/button/menu_button_unittest.cc
[modify] https://crrev.com/5148d1fe59a144bc7eba073d4f30e4eeefa8b9af/ui/views/examples/menu_example.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f7e3821183eb407fbd932aabdd0ace2e8e0942d

commit 7f7e3821183eb407fbd932aabdd0ace2e8e0942d
Author: Peter Boström <pbos@chromium.org>
Date: Mon Nov 12 18:53:00 2018

Reland "Remove menu_marker from MenuButton."

This reverts commit 5148d1fe59a144bc7eba073d4f30e4eeefa8b9af.

Reason for revert: Speculative reland, root cause should be fixed with r607193.

Original change's description:
> Revert "Remove menu_marker from MenuButton."
>
> This reverts commit 3275875871d7e5dd239acbe5ac6545d51a25cb3d.
>
> Reason for revert: causes compile failures on windows similar to the ones that appeared in the CQ runs, e.g. https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win%20x64%20Builder/58688
>
> Original change's description:
> > Remove menu_marker from MenuButton.
> >
> > This is always false except for in tests and menu_example.cc which is
> > not used. This helps simplify MenuButton for the Button cleanup task.
> >
> > Bug: 901183
> > Change-Id: If5a3cf7b6ba1ce5618548d112df1c4404b2ae181
> > Reviewed-on: https://chromium-review.googlesource.com/c/1324375
> > Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> > Reviewed-by: Robert Flack <flackr@chromium.org>
> > Reviewed-by: Trent Apted <tapted@chromium.org>
> > Reviewed-by: Michael Wasserman <msw@chromium.org>
> > Commit-Queue: Charlene Yan <cyan@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#607061}
>
> TBR=msw@chromium.org,flackr@chromium.org,oshima@chromium.org,tapted@chromium.org,cyan@chromium.org
>
> Change-Id: I9c311a3c8cea56c7670b2dace3e0410618151034
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 901183
> Reviewed-on: https://chromium-review.googlesource.com/c/1330841
> Reviewed-by: John Budorick <jbudorick@chromium.org>
> Commit-Queue: John Budorick <jbudorick@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607072}

TBR=msw@chromium.org,flackr@chromium.org,oshima@chromium.org,tapted@chromium.org,jbudorick@chromium.org,cyan@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 901183
Change-Id: I8a54361c857ac0a18acfc41dedb9537180aae4ce
Reviewed-on: https://chromium-review.googlesource.com/c/1331137
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607294}
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/ash/shelf/login_shelf_view.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/chrome/browser/ui/views/frame/app_menu_button.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/chrome/browser/ui/views/hover_button.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/chrome/browser/ui/views/menu_model_adapter_test.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/chrome/browser/ui/views/menu_test_base.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/chrome/browser/ui/views/page_info/permission_selector_row.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/chrome/browser/ui/views/tab_icon_view.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/chrome/browser/ui/views/toolbar/toolbar_action_view_unittest.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/tools/gritsettings/startup_resources_win.txt
[delete] https://crrev.com/c014ab347ec6ce6988329c123bd456b38d6b5d92/ui/resources/default_100_percent/cros/menu_droparrow.png
[delete] https://crrev.com/c014ab347ec6ce6988329c123bd456b38d6b5d92/ui/resources/default_200_percent/cros/menu_droparrow.png
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/ui/resources/ui_resources.grd
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/ui/views/controls/button/button_unittest.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/ui/views/controls/button/menu_button.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/ui/views/controls/button/menu_button.h
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/ui/views/controls/button/menu_button_unittest.cc
[modify] https://crrev.com/7f7e3821183eb407fbd932aabdd0ace2e8e0942d/ui/views/examples/menu_example.cc

Blockedon: 904366
Cc: jbudorick@chromium.org pbos@chromium.org
Components: Internals>Services>Ash Internals>Views
FYI, the revert above caused lots of compile failures on Windows bots. cwallez@ fixed these up in  Issue 904366 .

The original landing caused lots of compile failures on windows bots :/
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de8970759a44d64902b6541ab797d2d4bdf78470

commit de8970759a44d64902b6541ab797d2d4bdf78470
Author: Charlene Yan <cyan@chromium.org>
Date: Tue Nov 13 22:58:53 2018

New MenuButtonEventHandler class used by MenuButton.

MenuButtonEventHandler includes the old PressedLock and all other logic
for controlling what a button should do (migrated from MenuButton).

Bug: 901183

Change-Id: Ie2dbdfb088c8b2818ddc71445c88c349039bd84c
Reviewed-on: https://chromium-review.googlesource.com/c/1297589
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607791}
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/chrome/browser/ui/views/infobars/infobar_view.cc
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/chrome/browser/ui/views/toolbar/toolbar_action_view.h
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/ui/views/BUILD.gn
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/ui/views/controls/button/menu_button.cc
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/ui/views/controls/button/menu_button.h
[add] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/ui/views/controls/button/menu_button_event_handler.cc
[add] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/ui/views/controls/button/menu_button_event_handler.h
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/ui/views/controls/button/menu_button_unittest.cc
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/de8970759a44d64902b6541ab797d2d4bdf78470/ui/views/controls/menu/menu_controller.h

Labels: Hotlist-DesktopUIValid Hotlist-DesktopUIChecked
***UI Mass Triage***

Since work is in progress, adding appropriate labels.
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7edf9a1030a554baac22b61aaa376b3cd81ca67d

commit 7edf9a1030a554baac22b61aaa376b3cd81ca67d
Author: Charlene Yan <cyan@chromium.org>
Date: Wed Nov 28 19:09:55 2018

Change InkDropGestureHandler to handle mouse events as well.

Changing InkDropGestureHandler to InkDropEventHandler to
unconditionally handle mouse events in addition to gesture events for
ink drops.
This makes the ink drop less error prone as previously InkDropHostView
depended on parents to call InkDropHostView::OnMouseEvent after
handling it themselves

Bug: 901183
Change-Id: Iee9c259b44d1057b2ba5d0c81b29602c9f6b77a4
Reviewed-on: https://chromium-review.googlesource.com/c/1344262
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611807}
[modify] https://crrev.com/7edf9a1030a554baac22b61aaa376b3cd81ca67d/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/7edf9a1030a554baac22b61aaa376b3cd81ca67d/ui/views/animation/ink_drop_host_view.h
[modify] https://crrev.com/7edf9a1030a554baac22b61aaa376b3cd81ca67d/ui/views/animation/ink_drop_host_view_unittest.cc
[modify] https://crrev.com/7edf9a1030a554baac22b61aaa376b3cd81ca67d/ui/views/animation/test/ink_drop_host_view_test_api.cc
[modify] https://crrev.com/7edf9a1030a554baac22b61aaa376b3cd81ca67d/ui/views/animation/test/ink_drop_host_view_test_api.h
[modify] https://crrev.com/7edf9a1030a554baac22b61aaa376b3cd81ca67d/ui/views/controls/button/button.cc
[modify] https://crrev.com/7edf9a1030a554baac22b61aaa376b3cd81ca67d/ui/views/controls/button/image_button_factory_unittest.cc

Sign in to add a comment