Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(264)

Issue 2696073002: Merge ShelfItemDelegate::ItemSelected & LauncherItemDelegate::Activate. (Closed)

Created:
10 months ago by msw
Modified:
10 months ago
Reviewers:
James Cook
CC:
chromium-reviews, kalyank, sadrul, Matt Giuca, tfarina
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Merge ShelfItemDelegate::ItemSelected & LauncherItemDelegate::Activate. These functions have nearly identical purposes; merge them. (app-list used Activate via ChromeLauncherControllerImpl::ActivateApp) Split up the ui::Event argument into constituent pieces. (this will help when converting the function to support mojo) Move ash::LaunchSource (in src/chrome) -> ash::ShelfLaunchSource. Move ash::ShelfItemDelegate::PerformedAction -> ash::ShelfAction. (also update its kConstantValueNaming to ENUM_VALUE_NAMING) Add ash::wm::MoveWindowToDisplay helper; minor refactor; add test. BUG=557406 TEST=No behavior changes selecting/activating shelf or app-list items. R=jamescook@chromium.org Review-Url: https://codereview.chromium.org/2696073002 Cr-Commit-Position: refs/heads/master@{#451072} Committed: https://chromium.googlesource.com/chromium/src/+/c30c27728fcf046088ca6c0da51bda06e3a5ce63

Patch Set 1 #

Patch Set 2 : Cleanup #

Total comments: 20

Patch Set 3 : Address comments. #

Total comments: 2

Patch Set 4 : Add an ItemSelectedBySource helper and test helper. #

Total comments: 2

Patch Set 5 : nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+422 lines, -342 lines) Patch
M ash/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M ash/common/shelf/app_list_shelf_item_delegate.h View 1 chunk +4 lines, -2 lines 0 comments Download
M ash/common/shelf/app_list_shelf_item_delegate.cc View 1 2 1 chunk +5 lines, -3 lines 0 comments Download
M ash/common/shelf/shelf_button_pressed_metric_tracker.h View 2 chunks +2 lines, -3 lines 0 comments Download
M ash/common/shelf/shelf_button_pressed_metric_tracker.cc View 1 2 3 chunks +10 lines, -10 lines 0 comments Download
M ash/common/shelf/shelf_button_pressed_metric_tracker_unittest.cc View 1 2 11 chunks +21 lines, -24 lines 0 comments Download
M ash/common/shelf/shelf_controller.cc View 1 2 1 chunk +8 lines, -5 lines 0 comments Download
M ash/common/shelf/shelf_item_delegate.h View 1 2 3 4 2 chunks +18 lines, -27 lines 0 comments Download
A ash/common/shelf/shelf_item_delegate.cc View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download
M ash/common/shelf/shelf_view.cc View 1 2 2 chunks +8 lines, -6 lines 0 comments Download
M ash/common/shelf/shelf_window_watcher_item_delegate.h View 1 2 chunks +4 lines, -3 lines 0 comments Download
M ash/common/shelf/shelf_window_watcher_item_delegate.cc View 1 2 2 chunks +13 lines, -9 lines 0 comments Download
M ash/common/shelf/wm_shelf.cc View 1 2 2 chunks +3 lines, -7 lines 0 comments Download
M ash/common/test/test_shelf_item_delegate.h View 1 chunk +4 lines, -2 lines 0 comments Download
M ash/common/test/test_shelf_item_delegate.cc View 1 2 2 chunks +8 lines, -5 lines 0 comments Download
M ash/public/cpp/shelf_types.h View 1 2 1 chunk +30 lines, -0 lines 0 comments Download
M ash/shelf/shelf_view_unittest.cc View 1 2 5 chunks +10 lines, -12 lines 0 comments Download
M ash/shell/window_watcher_shelf_item_delegate.h View 1 chunk +4 lines, -2 lines 0 comments Download
M ash/shell/window_watcher_shelf_item_delegate.cc View 1 2 1 chunk +7 lines, -4 lines 0 comments Download
M ash/wm/window_util.h View 1 2 chunks +7 lines, -1 line 0 comments Download
M ash/wm/window_util.cc View 1 2 3 chunks +28 lines, -9 lines 0 comments Download
M ash/wm/window_util_unittest.cc View 1 2 4 chunks +43 lines, -6 lines 0 comments Download
M chrome/browser/ui/ash/app_list/app_list_controller_ash.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/app_list/app_list_controller_ash.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.h View 2 chunks +5 lines, -6 lines 0 comments Download
M chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc View 1 2 3 chunks +13 lines, -15 lines 0 comments Download
M chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h View 1 2 2 chunks +10 lines, -11 lines 0 comments Download
M chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.cc View 1 2 3 chunks +10 lines, -17 lines 0 comments Download
M chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.h View 1 chunk +4 lines, -6 lines 0 comments Download
M chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.cc View 1 2 2 chunks +6 lines, -8 lines 0 comments Download
M chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.h View 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/launcher/arc_app_window_launcher_item_controller.cc View 1 2 1 chunk +14 lines, -10 lines 0 comments Download
M chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.h View 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc View 1 2 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h View 1 2 2 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc View 1 2 5 chunks +20 lines, -22 lines 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller.h View 1 2 3 chunks +7 lines, -8 lines 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc View 1 2 3 4 chunks +9 lines, -12 lines 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc View 1 2 3 9 chunks +25 lines, -24 lines 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc View 1 2 1 chunk +5 lines, -7 lines 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller_mus.cc View 1 2 2 chunks +3 lines, -4 lines 0 comments Download
D chrome/browser/ui/ash/launcher/chrome_launcher_types.h View 1 chunk +0 lines, -19 lines 0 comments Download
M chrome/browser/ui/ash/launcher/launcher_controller_helper.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/launcher/launcher_controller_helper.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/launcher/launcher_item_controller.h View 2 chunks +0 lines, -6 lines 0 comments Download

Messages

Total messages: 40 (26 generated)
msw
Hey James, please take a look; thanks!
10 months ago (2017-02-14 23:00:02 UTC) #9
James Cook
I really like collapsing the functions! I'm concerned about the default values for the parameters ...
10 months ago (2017-02-15 00:44:40 UTC) #11
msw
Please take another look, thanks! (I also added a test) https://codereview.chromium.org/2696073002/diff/20001/ash/common/shelf/app_list_shelf_item_delegate.cc File ash/common/shelf/app_list_shelf_item_delegate.cc (right): https://codereview.chromium.org/2696073002/diff/20001/ash/common/shelf/app_list_shelf_item_delegate.cc#newcode40 ...
10 months ago (2017-02-15 19:59:18 UTC) #17
James Cook
LGTM modulo offline discussion about overloading ItemSelected(). https://codereview.chromium.org/2696073002/diff/40001/ash/wm/window_util_unittest.cc File ash/wm/window_util_unittest.cc (right): https://codereview.chromium.org/2696073002/diff/40001/ash/wm/window_util_unittest.cc#newcode146 ash/wm/window_util_unittest.cc:146: } Thanks ...
10 months ago (2017-02-15 23:06:12 UTC) #20
James Cook
LGTM https://codereview.chromium.org/2696073002/diff/60001/ash/common/shelf/shelf_item_delegate.h File ash/common/shelf/shelf_item_delegate.h (right): https://codereview.chromium.org/2696073002/diff/60001/ash/common/shelf/shelf_item_delegate.h#newcode26 ash/common/shelf/shelf_item_delegate.h:26: ShelfAction ItemSelectedBySource(ShelfLaunchSource source); nit: I would put this ...
10 months ago (2017-02-16 00:08:19 UTC) #21
msw
comments addressed; landing. Thanks! https://codereview.chromium.org/2696073002/diff/40001/ash/wm/window_util_unittest.cc File ash/wm/window_util_unittest.cc (right): https://codereview.chromium.org/2696073002/diff/40001/ash/wm/window_util_unittest.cc#newcode146 ash/wm/window_util_unittest.cc:146: } On 2017/02/15 23:06:12, James ...
10 months ago (2017-02-16 00:12:20 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2696073002/80001
10 months ago (2017-02-16 00:13:53 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on ...
10 months ago (2017-02-16 02:16:55 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2696073002/80001
10 months ago (2017-02-16 02:20:21 UTC) #29
commit-bot: I haz the power
Exceeded global retry quota
10 months ago (2017-02-16 02:51:43 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2696073002/80001
10 months ago (2017-02-16 04:33:36 UTC) #33
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/391336)
10 months ago (2017-02-16 08:44:58 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2696073002/80001
10 months ago (2017-02-16 17:45:13 UTC) #37
commit-bot: I haz the power
10 months ago (2017-02-16 20:09:18 UTC) #40
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c30c27728fcf046088ca6c0da51b...

Powered by Google App Engine
This is Rietveld 0eb02b776