Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(248)

Issue 1857213004: Refactor of LauncherContextMenu. (Closed)

Created:
4 years, 8 months ago by lgcheng
Modified:
4 years, 8 months ago
Reviewers:
stevenjb
CC:
chromium-reviews, kalyank, sadrul, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Refactor of exsiting LauncherContextMenu. Add derived classes ExtensionLauncherContextMenu to handle extension app and DesktopShellLauncherContextMenu to handle right click on desktopshell. This is a split part of patch https://codereview.chromium.org/1838263002/ Second split is: https://codereview.chromium.org/1869063002/ Committed: https://crrev.com/6b2dd155d13421f2e62bd8028f67bcba36afb6ea Cr-Commit-Position: refs/heads/master@{#385648}

Patch Set 1 #

Patch Set 2 : Nit #

Total comments: 9

Patch Set 3 : Issue Fix #

Total comments: 2

Patch Set 4 : Nits update #

Unified diffs Side-by-side diffs Delta from patch set Stats (+319 lines, -522 lines) Patch
M chrome/browser/ui/ash/chrome_shell_delegate.cc View 1 chunk +1 line, -1 line 0 comments Download
A + chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h View 1 chunk +19 lines, -18 lines 0 comments Download
A + chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.cc View 1 2 1 chunk +10 lines, -12 lines 0 comments Download
A + chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h View 1 chunk +21 lines, -34 lines 0 comments Download
A + chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc View 1 2 3 chunks +108 lines, -219 lines 0 comments Download
M chrome/browser/ui/ash/launcher/launcher_context_menu.h View 1 2 3 3 chunks +42 lines, -21 lines 0 comments Download
M chrome/browser/ui/ash/launcher/launcher_context_menu.cc View 1 2 7 chunks +81 lines, -216 lines 0 comments Download
M chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc View 3 chunks +33 lines, -1 line 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (9 generated)
lgcheng
Hi Steven PTAL at this patch This is the pure refactor of exsiting LauncherContextMenu with ...
4 years, 8 months ago (2016-04-06 20:12:12 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1857213004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857213004/20001
4 years, 8 months ago (2016-04-06 20:40:45 UTC) #6
stevenjb
Thanks for splitting this up. Much easier to do a more careful review this way. ...
4 years, 8 months ago (2016-04-06 22:19:23 UTC) #7
lgcheng
Hi Steven, PTAL at the update. Thanks, https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc#newcode183 chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:183: return LauncherContextMenu::ExecuteCommand(command_id, ...
4 years, 8 months ago (2016-04-06 22:59:54 UTC) #8
lgcheng
https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc#newcode183 chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:183: return LauncherContextMenu::ExecuteCommand(command_id, event_flags); On 2016/04/06 22:59:54, lgcheng wrote: > ...
4 years, 8 months ago (2016-04-06 23:14:25 UTC) #9
stevenjb
lgtm with a question and updated comment. https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/launcher/launcher_context_menu.cc File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/launcher/launcher_context_menu.cc#newcode71 chrome/browser/ui/ash/launcher/launcher_context_menu.cc:71: NOTREACHED(); On ...
4 years, 8 months ago (2016-04-07 00:40:34 UTC) #10
lgcheng
Nits updates. Thanks for review! I will create a separate cl for Arc integration. https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/launcher/launcher_context_menu.cc ...
4 years, 8 months ago (2016-04-07 01:28:09 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1857213004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857213004/60001
4 years, 8 months ago (2016-04-07 01:29:10 UTC) #14
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 8 months ago (2016-04-07 03:53:30 UTC) #16
commit-bot: I haz the power
4 years, 8 months ago (2016-04-07 03:54:43 UTC) #18
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/6b2dd155d13421f2e62bd8028f67bcba36afb6ea
Cr-Commit-Position: refs/heads/master@{#385648}

Powered by Google App Engine
This is Rietveld 408576698