Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1838263002:
Arc app integration in shelf launcher context menu (Closed)
|
Created:
4 years, 8 months ago by lgcheng Modified:
4 years, 8 months ago 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. |
DescriptionImplement base class for shelf launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. Half of the patch is for moving the implementation for extension from LancherContextMenu to its subclass ExtensionLancherContextMenu.
BUG=http://b/27418199
TEST= Manual test on minnie after setting Arc to multi-window mode. Unit test covers menu item checks after the creation of DesktopShellLauncherContextMenu and ArcLauncherContextMenu.
I am closing this cl. The issue is covered by two splitted cls
1. Pure refactor https://codereview.chromium.org/1857213004
2. Arc integration https://codereview.chromium.org/1869063002/
Patch Set 1 #Patch Set 2 : nit #Patch Set 3 : #
Total comments: 6
Patch Set 4 : Refactor #
Total comments: 12
Patch Set 5 : Refactor and comments&inlucdes cleanup #
Total comments: 24
Patch Set 6 : Issue Correction #
Total comments: 6
Patch Set 7 : Nits update #
Total comments: 4
Patch Set 8 : Move implementation into right class #Patch Set 9 : Comment update #
Total comments: 2
Patch Set 10 : Nits #Patch Set 11 : Rebase #
Total comments: 18
Patch Set 12 : Member method privay update #Messages
Total messages: 76 (45 generated)
Description was changed from ========== Patch Implement base class for launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. FREEZE.unindexed FREEZE.indexed BUG=http://b/27418199 ========== to ========== Implement base class for launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. BUG=http://b/27418199 ==========
Description was changed from ========== Implement base class for launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. BUG=http://b/27418199 ========== to ========== Implement base class for launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. Half of the patch is for moving the implementation for extension from LancherContextMenu to its subclass ExtensionLancherContextMenu. BUG=http://b/27418199 ==========
Description was changed from ========== Implement base class for launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. Half of the patch is for moving the implementation for extension from LancherContextMenu to its subclass ExtensionLancherContextMenu. BUG=http://b/27418199 ========== to ========== Implement base class for launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. Half of the patch is for moving the implementation for extension from LancherContextMenu to its subclass ExtensionLancherContextMenu. BUG=http://b/27418199 TEST= Manual test on minnie after setting Arc to multi-window mode. Unit test covers menu item checks after the creation of DesktopShellLauncherContextMenu and ArcLauncherContextMenu. ==========
lgcheng@google.com changed reviewers: + khmel@chromium.org
PTAL
https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:45: return l10n_util::GetStringUTF16(IDS_APP_CONTEXT_MENU_ACTIVATE_ARC); Why do you need return label here? it is already set in ::Init. Can we use non-dynamic label? You have MENU_CLOSE, which is handled differently for example. https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:55: return controller()->IsOpen(item().id); Why not to handle it in base class? MENU_CLOSE, MENU_PIN, MENU_CHANGE_WALLPAPER... is not arc specific. https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.h:24: static LauncherContextMenu* CreateLauncherContextMenu( nit: Probably Create sounds better.
PTAL. Previous issues fixed. https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:45: return l10n_util::GetStringUTF16(IDS_APP_CONTEXT_MENU_ACTIVATE_ARC); On 2016/03/29 20:31:27, khmel wrote: > Why do you need return label here? it is already set in ::Init. Can we use > non-dynamic label? You have MENU_CLOSE, which is handled differently for > example. Done. https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:55: return controller()->IsOpen(item().id); On 2016/03/29 20:31:27, khmel wrote: > Why not to handle it in base class? > > MENU_CLOSE, MENU_PIN, MENU_CHANGE_WALLPAPER... is not arc specific. Reimplemented https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/40001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/launcher_context_menu.h:24: static LauncherContextMenu* CreateLauncherContextMenu( On 2016/03/29 20:31:27, khmel wrote: > nit: Probably Create sounds better. Done.
Sorry, has no time to pass through all review, Please process comments so far, pay attention to copyrights headers, includes, comments. https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:7: #include <string> Do you really use std::string? https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:9: #include "ash/shelf/shelf.h" Please check unused headers, I see only controller is in use and resources. https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:14: #include "grit/ash_strings.h" Used? https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. (c) is not required https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:8: #include "ash/shelf/shelf_item_types.h" Is it in use? You have the same include in .cc, please double check all headers. https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:14: // Class for context menu shown for desktop shell Showing 'desktop shell'?
Description was changed from ========== Implement base class for launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. Half of the patch is for moving the implementation for extension from LancherContextMenu to its subclass ExtensionLancherContextMenu. BUG=http://b/27418199 TEST= Manual test on minnie after setting Arc to multi-window mode. Unit test covers menu item checks after the creation of DesktopShellLauncherContextMenu and ArcLauncherContextMenu. ========== to ========== Implement base class for shelf launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. Half of the patch is for moving the implementation for extension from LancherContextMenu to its subclass ExtensionLancherContextMenu. BUG=http://b/27418199 TEST= Manual test on minnie after setting Arc to multi-window mode. Unit test covers menu item checks after the creation of DesktopShellLauncherContextMenu and ArcLauncherContextMenu. ==========
Patchset #9 (id:160001) has been deleted
Patchset #8 (id:140001) has been deleted
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
PTAL. Fix previous issues and several self check and correction https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:7: #include <string> On 2016/03/29 22:11:17, khmel wrote: > Do you really use std::string? Removed. Removed for other .cc when not used https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:9: #include "ash/shelf/shelf.h" On 2016/03/29 22:11:17, khmel wrote: > Please check unused headers, I see only controller is in use and resources. Done. https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:14: #include "grit/ash_strings.h" On 2016/03/29 22:11:17, khmel wrote: > Used? Removed. Removed for other .cc when not used https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/03/29 22:11:17, khmel wrote: > (c) is not required Done. https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:8: #include "ash/shelf/shelf_item_types.h" On 2016/03/29 22:11:17, khmel wrote: > Is it in use? You have the same include in .cc, please double check all headers. Removed. Removed for other .h and .cc when not used https://codereview.chromium.org/1838263002/diff/60001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:14: // Class for context menu shown for desktop shell On 2016/03/29 22:11:17, khmel wrote: > Showing 'desktop shell'? Done.
https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:23: AddItem(MENU_OPEN_NEW, Please use AddItemWithStringId (and at other places). You can reduce dependencies (ui/base/l10n/l10n_util.h) https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:10: namespace ash { class Shelf; class ShelfItem; } https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h:10: As at comment above: namespace ash { ... } https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h:26: #endif // CHROME_BROWSER_UI_ASH_LAUNCHER_DESKTOP_SHELL_LAUNCHER_CONTEXT_MENU_H_ nit: empty line https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:27: } // name space // namespace https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:128: if (item().type == ash::TYPE_PLATFORM_APP) { {} is not required for one-line https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h:12: class ChromeLauncherController; namespace ash { ... } https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:92: return shelf_->GetAutoHideBehavior() == you have > 1 line here, Please { } https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:100: case MENU_CLOSE: Please don't add untested code, this is functionality change. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.h:29: virtual void Init() = 0; You don't use Init in this class, please remove it. It is not virtual https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:57: LauncherContextMenu* CreateLauncherContextMenuForArcApp() { This must be under #if defined(OS_CHROMEOS) #endif https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:142: #if defined(OS_CHROMEOS) Whole test must be under #if defined(OS_CHROMEOS) #endif
Patchset #6 (id:200001) has been deleted
PTAL when free. Issue fixed. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.cc:23: AddItem(MENU_OPEN_NEW, On 2016/03/30 00:58:46, khmel wrote: > Please use AddItemWithStringId (and at other places). > You can reduce dependencies (ui/base/l10n/l10n_util.h) Done. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:10: On 2016/03/30 00:58:46, khmel wrote: > namespace ash { > class Shelf; > class ShelfItem; > } Use > namespace ash { > class Shelf; > struct ShelfItem; > } Using class ShelfItem works for chrome build but breaks the build of unit_tests. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h:10: On 2016/03/30 00:58:46, khmel wrote: > As at comment above: > namespace ash { > ... > } Done. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h:26: #endif // CHROME_BROWSER_UI_ASH_LAUNCHER_DESKTOP_SHELL_LAUNCHER_CONTEXT_MENU_H_ On 2016/03/30 00:58:46, khmel wrote: > nit: empty line Done. Same thing done with ArcLauncherContextMenu https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:27: } // name space On 2016/03/30 00:58:46, khmel wrote: > // namespace Done. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:128: if (item().type == ash::TYPE_PLATFORM_APP) { On 2016/03/30 00:58:46, khmel wrote: > {} is not required for one-line Done. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h:12: class ChromeLauncherController; On 2016/03/30 00:58:46, khmel wrote: > namespace ash { > ... > } Done. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:92: return shelf_->GetAutoHideBehavior() == On 2016/03/30 00:58:46, khmel wrote: > you have > 1 line here, > Please { > } Done. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:100: case MENU_CLOSE: On 2016/03/30 00:58:46, khmel wrote: > Please don't add untested code, this is functionality change. Changed back to original implementation https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.h:29: virtual void Init() = 0; On 2016/03/30 00:58:46, khmel wrote: > You don't use Init in this class, please remove it. It is not virtual Done. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc (right): https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:57: LauncherContextMenu* CreateLauncherContextMenuForArcApp() { On 2016/03/30 00:58:46, khmel wrote: > This must be under > #if defined(OS_CHROMEOS) > #endif Done. https://codereview.chromium.org/1838263002/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu_unittest.cc:142: #if defined(OS_CHROMEOS) On 2016/03/30 00:58:46, khmel wrote: > Whole test must be under > #if defined(OS_CHROMEOS) > #endif Done.
Please see my comments, At this stage you may add OWNER to review. https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:95: NOTREACHED(); Are you sure that MENU_PIN (as example) does not come here? You may not see this assert in release build. I would recommend to build Chrome OS build debug on Linux and test that cases. https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:112: NOTREACHED(); Check here as in my comment above. https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.h:14: class ChromeLauncherController; nit: do we need namespace ash { class Shelf; } It is not clear for me that Shelf is included from here.
lgcheng@google.com changed reviewers: + skuhne@chromium.org
Fix the issues. Hi Stephen, PTAL at this path. It is context menu implementation for Arc app. A big portion of the patch is refactor the implementation from base class LauncherContextMenu to subclass ExtensionLauncherContextMenu. https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:95: NOTREACHED(); On 2016/03/31 18:23:12, khmel wrote: > Are you sure that MENU_PIN (as example) does not come here? > You may not see this assert in release build. > > I would recommend to build Chrome OS build debug on Linux and test that cases. I can't find it can reach here anytime when I tested on Linux. But I'd remove this command as it may cause confusion. In the original implementation it directly returned false. https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:112: NOTREACHED(); On 2016/03/31 18:23:12, khmel wrote: > Check here as in my comment above. It should not reach here. For extension, this function is called only if the command id falls in {MENU_CLOSE, MENU_PIN, MENU_AUTO_HIDE, MENU_ALIGNMENT_MENU,MENU_CHANGE_WALLPAPER}. As what I understood and tested, Desktop shell and Arc app should not have item outside MENU_ITEM_COUNT https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/190012/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.h:14: class ChromeLauncherController; On 2016/03/31 18:23:12, khmel wrote: > nit: do we need > namespace ash { > class Shelf; > } > It is not clear for me that Shelf is included from here. Done.
lgtm with comments Deffer to OWNERs https://codereview.chromium.org/1838263002/diff/230001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/230001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:151: case MENU_AUTO_HIDE: Please Use handling MENU_AUTO_HIDE in base class https://codereview.chromium.org/1838263002/diff/230001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/230001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:95: return false; I would recommend you to have something like: if (command_id < MENU_ITEM_COUNT) return false; and for extension call base class for this. Or: DCHECK(command_id < MENU_ITEM_COUNT); return false; Both arc, and extension should come to this base class to handle these (common) ids if it is possible.
PTAL. Fix last issues https://codereview.chromium.org/1838263002/diff/230001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/230001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:95: return false; On 2016/03/31 21:41:57, khmel wrote: > I would recommend you to have something like: > > if (command_id < MENU_ITEM_COUNT) > return false; > and for extension call base class for this. > > Or: > DCHECK(command_id < MENU_ITEM_COUNT); > return false; > > Both arc, and extension should come to this base class to handle these (common) > ids if it is possible. Done.
https://codereview.chromium.org/1838263002/diff/250001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/250001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:93: case LAUNCH_TYPE_PINNED_TAB: As discussed, please handle extension specific commands in ExtensionContextMenu. If command used in both, arc and extension and we can handle it the same way, move it to base class.
Patchset #8 (id:250001) has been deleted
Move the implementation back to the right class. Make sure commandid falls into right piece of code. PTAL https://codereview.chromium.org/1838263002/diff/230001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/230001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:151: case MENU_AUTO_HIDE: On 2016/03/31 21:41:57, khmel wrote: > Please Use handling MENU_AUTO_HIDE in base class Done.
Nice! LGTM. Sorry for the late reply, but I was OOO. https://codereview.chromium.org/1838263002/diff/280001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/280001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:52: AddItemWithStringId(MENU_CLOSE, IDS_LAUNCHER_CONTEXT_MENU_CLOSE); can you please either add an empty line behind this - or brackets? Otherwise this is hard to read.
Nits Fixed! Thanks for review! https://codereview.chromium.org/1838263002/diff/280001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/280001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:52: AddItemWithStringId(MENU_CLOSE, IDS_LAUNCHER_CONTEXT_MENU_CLOSE); On 2016/04/04 15:58:07, Mr4D wrote: > can you please either add an empty line behind this - or brackets? Otherwise > this is hard to read. Done.
The CQ bit was checked by lgcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@chromium.org, skuhne@chromium.org Link to the patchset: https://codereview.chromium.org/1838263002/#ps300001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838263002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838263002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
The CQ bit was unchecked by lgcheng@google.com
Patchset #11 (id:320001) has been deleted
Patchset #11 (id:340001) has been deleted
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838263002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838263002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lgcheng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@chromium.org, skuhne@chromium.org Link to the patchset: https://codereview.chromium.org/1838263002/#ps360001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838263002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838263002/360001
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...)
lgcheng@google.com changed reviewers: + stevenjb@chromium.org
Hi Steven, Can you help me to take a look at this patch. It is the Arc app integration in shelf launcher context menu. I had update on chrome/browser/ui/ash/chrome_shell_delegate.cc and I need owner's approval. Thanks, Long
lgcheng@google.com changed reviewers: - khmel@chromium.org, skuhne@chromium.org
Forget to mark PTAL Thanks, Long
Patchset #12 (id:380001) has been deleted
The CQ bit was checked by lgcheng@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1838263002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1838263002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:24: ~ArcLauncherContextMenu() override; blank line https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:25: void Init(); Can this be private? If not Add a comment to when/why/how Init() should be called. +blank line after. https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h:24: ~DesktopShellLauncherContextMenu() override; blank line https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h:25: void Init(); Private or comment. https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:39: void ExtensionLauncherContextMenu::Init() { Can you try using a lower --similarity value with git cl upload so that we can maybe get a diff from launcher_context_menu.cc? I assume this logic hasn't changed? https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h:30: ~ExtensionLauncherContextMenu() override; blank line https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h:31: void Init(); private or comment blank line https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:60: } no {}
Patchset #12 (id:400001) has been deleted
Patchset #12 (id:420001) has been deleted
Patchset #12 (id:440001) has been deleted
Patchset #12 (id:460001) has been deleted
Patchset #12 (id:480001) has been deleted
Patchset #12 (id:500001) has been deleted
Patchset #12 (id:520001) has been deleted
Patchset #12 (id:540001) has been deleted
Patchset #12 (id:560001) has been deleted
Patchset #12 (id:580001) has been deleted
Patchset #12 (id:600001) has been deleted
Patchset #12 (id:620001) has been deleted
Patchset #12 (id:640001) has been deleted
Hi Steven, Thanks for your comment! PTAL at my updates. Thanks, Long https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:24: ~ArcLauncherContextMenu() override; On 2016/04/06 00:06:48, stevenjb wrote: > blank line Done. https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h:25: void Init(); On 2016/04/06 00:06:48, stevenjb wrote: > Can this be private? If not Add a comment to when/why/how Init() should be > called. > +blank line after. I agree this method should be private. I left public because it was a public method in base class before I made updates to the file laucher_context_menu.cc, although it was never called outside the class itself. I'd like to move this method private with your acknowledgement. And same update will be made to other subclasses. https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h:24: ~DesktopShellLauncherContextMenu() override; On 2016/04/06 00:06:48, stevenjb wrote: > blank line Done. https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/desktop_shell_launcher_context_menu.h:25: void Init(); On 2016/04/06 00:06:48, stevenjb wrote: > Private or comment. Done. https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:39: void ExtensionLauncherContextMenu::Init() { On 2016/04/06 00:06:49, stevenjb wrote: > Can you try using a lower --similarity value with git cl upload so that we can > maybe get a diff from launcher_context_menu.cc? I assume this logic hasn't > changed? This logic is not changed, but I change the implementation a little: In original implementation, it was: if (item_.id != 0) { details chunk....... } Now there is no need to check item id in this function, so the details chunk is moved 2 spaces forward, causing git sees the whole chunk as new implementation. Direct access to private member item_ , controller_, shelf_ in original implementation is also changed to protected method of base class item(), controller(), shelf(). These make the similarity of ExtensionLauncherContextMenu.cc and LauncherContextMenu.cc very low. I need to set the similarity value really low(26%) to make git treat launcher_context_menu.cc as the base file of extension_launcher_context_menu.cc. But this would cause some head files treat unrelated SimpleMenuModel subclass's head files(other contextmenu implementation, e.g applist contextmenu) in other directory as base files, which would also annoy the review. Do you have any suggestions? https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h:30: ~ExtensionLauncherContextMenu() override; On 2016/04/06 00:06:49, stevenjb wrote: > blank line Done. https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.h:31: void Init(); On 2016/04/06 00:06:49, stevenjb wrote: > private or comment > blank line Done. https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/launcher_context_menu.cc:60: } On 2016/04/06 00:06:49, stevenjb wrote: > no {} Done.
https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:39: void ExtensionLauncherContextMenu::Init() { On 2016/04/06 17:02:42, lgcheng wrote: > On 2016/04/06 00:06:49, stevenjb wrote: > > Can you try using a lower --similarity value with git cl upload so that we can > > maybe get a diff from launcher_context_menu.cc? I assume this logic hasn't > > changed? > > This logic is not changed, but I change the implementation a little: > In original implementation, it was: > if (item_.id != 0) { > details chunk....... > } > > Now there is no need to check item id in this function, so the details chunk is > moved 2 spaces forward, causing git sees the whole chunk as new implementation. > Direct access to private member item_ , controller_, shelf_ in original > implementation is also changed to protected method of base class item(), > controller(), shelf(). These make the similarity of > ExtensionLauncherContextMenu.cc and LauncherContextMenu.cc very low. > > I need to set the similarity value really low(26%) to make git treat > launcher_context_menu.cc as the base file of extension_launcher_context_menu.cc. > But this would cause some head files treat unrelated SimpleMenuModel subclass's > head files(other contextmenu implementation, e.g applist contextmenu) in other > directory as base files, which would also annoy the review. Do you have any > suggestions? I think the thing to do then is to split this CL up: a) Everything except the addition of arc_launcher_context_menu.* (and the changes integrating it) b) Add arc_launcher_context_menu.* Then you can use a low similarity value for a) and a normal one for b). Otherwise reviewing the changes ot this file is going to be a lot more work.
PTAL at my update. Thanks, Long https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right): https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:39: void ExtensionLauncherContextMenu::Init() { On 2016/04/06 17:16:55, stevenjb wrote: > On 2016/04/06 17:02:42, lgcheng wrote: > > On 2016/04/06 00:06:49, stevenjb wrote: > > > Can you try using a lower --similarity value with git cl upload so that we > can > > > maybe get a diff from launcher_context_menu.cc? I assume this logic hasn't > > > changed? > > > > This logic is not changed, but I change the implementation a little: > > In original implementation, it was: > > if (item_.id != 0) { > > details chunk....... > > } > > > > Now there is no need to check item id in this function, so the details chunk > is > > moved 2 spaces forward, causing git sees the whole chunk as new > implementation. > > Direct access to private member item_ , controller_, shelf_ in original > > implementation is also changed to protected method of base class item(), > > controller(), shelf(). These make the similarity of > > ExtensionLauncherContextMenu.cc and LauncherContextMenu.cc very low. > > > > I need to set the similarity value really low(26%) to make git treat > > launcher_context_menu.cc as the base file of > extension_launcher_context_menu.cc. > > But this would cause some head files treat unrelated SimpleMenuModel > subclass's > > head files(other contextmenu implementation, e.g applist contextmenu) in other > > directory as base files, which would also annoy the review. Do you have any > > suggestions? > > I think the thing to do then is to split this CL up: > a) Everything except the addition of arc_launcher_context_menu.* (and the > changes integrating it) > b) Add arc_launcher_context_menu.* > > Then you can use a low similarity value for a) and a normal one for b). > > Otherwise reviewing the changes ot this file is going to be a lot more work. Ignoring Arc related updates. When I try to lower the similarity threshold (30%), the head file of extension implementation itself first detects unrelated head file as base file. (See detail below) chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc | 220 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ chrome/browser/ui/{app_list/extension_app_context_menu.h => ash/launcher/extension_launcher_context_menu.h} | 55 ++++++-------- --------------------------------------------------------------------------------------------- Which means even if I split the Arc related code out of this cl, it still doesn't work. Is it possible that we have this workaround. I create a "fake cl" only for review with only one file extension_launcher_context_menu.cc. lower the threshold so that launcher_context_menu.cc can be detected as base file of this for you to review and leave this cl unchanged? Otherwise, I can't find a clean solution (I have asked everybody around for ideas, but none has a clean solution either). What do you suggest? Thanks
That's fine, we can ignore the bogus diff for the new file(s). I've had to do that before. I think that is better than attempting to review a "fake cl". On Wed, Apr 6, 2016 at 11:04 AM, <lgcheng@google.com> wrote: > PTAL at my update. > > Thanks, > > Long > > > > https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc > (right): > > > https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:39: > void ExtensionLauncherContextMenu::Init() { > On 2016/04/06 17:16:55, stevenjb wrote: > > On 2016/04/06 17:02:42, lgcheng wrote: > > > On 2016/04/06 00:06:49, stevenjb wrote: > > > > Can you try using a lower --similarity value with git cl upload so > that we > > can > > > > maybe get a diff from launcher_context_menu.cc? I assume this > logic hasn't > > > > changed? > > > > > > This logic is not changed, but I change the implementation a little: > > > In original implementation, it was: > > > if (item_.id != 0) { > > > details chunk....... > > > } > > > > > > Now there is no need to check item id in this function, so the > details chunk > > is > > > moved 2 spaces forward, causing git sees the whole chunk as new > > implementation. > > > Direct access to private member item_ , controller_, shelf_ in > original > > > implementation is also changed to protected method of base class > item(), > > > controller(), shelf(). These make the similarity of > > > ExtensionLauncherContextMenu.cc and LauncherContextMenu.cc very low. > > > > > > I need to set the similarity value really low(26%) to make git treat > > > launcher_context_menu.cc as the base file of > > extension_launcher_context_menu.cc. > > > But this would cause some head files treat unrelated SimpleMenuModel > > subclass's > > > head files(other contextmenu implementation, e.g applist > contextmenu) in other > > > directory as base files, which would also annoy the review. Do you > have any > > > suggestions? > > > > I think the thing to do then is to split this CL up: > > a) Everything except the addition of arc_launcher_context_menu.* (and > the > > changes integrating it) > > b) Add arc_launcher_context_menu.* > > > > Then you can use a low similarity value for a) and a normal one for > b). > > > > Otherwise reviewing the changes ot this file is going to be a lot more > work. > > Ignoring Arc related updates. > When I try to lower the similarity threshold (30%), the head file of > extension implementation itself first detects unrelated head file as > base file. (See detail below) > > chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc > | 220 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > chrome/browser/ui/{app_list/extension_app_context_menu.h => > ash/launcher/extension_launcher_context_menu.h} | 55 ++++++-------- > > > --------------------------------------------------------------------------------------------- > Which means even if I split the Arc related code out of this cl, it > still doesn't work. > Is it possible that we have this workaround. > I create a "fake cl" only for review with only one file > extension_launcher_context_menu.cc. lower the threshold so that > launcher_context_menu.cc can be detected as base file of this for you to > review and leave this cl unchanged? > Otherwise, I can't find a clean solution (I have asked everybody around > for ideas, but none has a clean solution either). > > What do you suggest? > > Thanks > > https://codereview.chromium.org/1838263002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/06 18:09:18, stevenjb wrote: > That's fine, we can ignore the bogus diff for the new file(s). I've had to > do that before. I think that is better than attempting to review a "fake > cl". > > > > On Wed, Apr 6, 2016 at 11:04 AM, <mailto:lgcheng@google.com> wrote: > > > PTAL at my update. > > > > Thanks, > > > > Long > > > > > > > > > https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... > > File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc > > (right): > > > > > > > https://codereview.chromium.org/1838263002/diff/360001/chrome/browser/ui/ash/... > > chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:39: > > void ExtensionLauncherContextMenu::Init() { > > On 2016/04/06 17:16:55, stevenjb wrote: > > > On 2016/04/06 17:02:42, lgcheng wrote: > > > > On 2016/04/06 00:06:49, stevenjb wrote: > > > > > Can you try using a lower --similarity value with git cl upload so > > that we > > > can > > > > > maybe get a diff from launcher_context_menu.cc? I assume this > > logic hasn't > > > > > changed? > > > > > > > > This logic is not changed, but I change the implementation a little: > > > > In original implementation, it was: > > > > if (item_.id != 0) { > > > > details chunk....... > > > > } > > > > > > > > Now there is no need to check item id in this function, so the > > details chunk > > > is > > > > moved 2 spaces forward, causing git sees the whole chunk as new > > > implementation. > > > > Direct access to private member item_ , controller_, shelf_ in > > original > > > > implementation is also changed to protected method of base class > > item(), > > > > controller(), shelf(). These make the similarity of > > > > ExtensionLauncherContextMenu.cc and LauncherContextMenu.cc very low. > > > > > > > > I need to set the similarity value really low(26%) to make git treat > > > > launcher_context_menu.cc as the base file of > > > extension_launcher_context_menu.cc. > > > > But this would cause some head files treat unrelated SimpleMenuModel > > > subclass's > > > > head files(other contextmenu implementation, e.g applist > > contextmenu) in other > > > > directory as base files, which would also annoy the review. Do you > > have any > > > > suggestions? > > > > > > I think the thing to do then is to split this CL up: > > > a) Everything except the addition of arc_launcher_context_menu.* (and > > the > > > changes integrating it) > > > b) Add arc_launcher_context_menu.* > > > > > > Then you can use a low similarity value for a) and a normal one for > > b). > > > > > > Otherwise reviewing the changes ot this file is going to be a lot more > > work. > > > > Ignoring Arc related updates. > > When I try to lower the similarity threshold (30%), the head file of > > extension implementation itself first detects unrelated head file as > > base file. (See detail below) > > > > chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc > > | 220 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > chrome/browser/ui/{app_list/extension_app_context_menu.h => > > ash/launcher/extension_launcher_context_menu.h} | 55 ++++++-------- > > > > > > > --------------------------------------------------------------------------------------------- > > Which means even if I split the Arc related code out of this cl, it > > still doesn't work. > > Is it possible that we have this workaround. > > I create a "fake cl" only for review with only one file > > extension_launcher_context_menu.cc. lower the threshold so that > > launcher_context_menu.cc can be detected as base file of this for you to > > review and leave this cl unchanged? > > Otherwise, I can't find a clean solution (I have asked everybody around > > for ideas, but none has a clean solution either). > > > > What do you suggest? > > > > Thanks > > > > https://codereview.chromium.org/1838263002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. https://codereview.chromium.org/1857213004/# Arc free split of this patch.
Description was changed from ========== Implement base class for shelf launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. Half of the patch is for moving the implementation for extension from LancherContextMenu to its subclass ExtensionLancherContextMenu. BUG=http://b/27418199 TEST= Manual test on minnie after setting Arc to multi-window mode. Unit test covers menu item checks after the creation of DesktopShellLauncherContextMenu and ArcLauncherContextMenu. ========== to ========== Implement base class for shelf launcher context menu. Implement subclasses of launcher context menu for extensions, Arc app and desktop shell. Half of the patch is for moving the implementation for extension from LancherContextMenu to its subclass ExtensionLancherContextMenu. BUG=http://b/27418199 TEST= Manual test on minnie after setting Arc to multi-window mode. Unit test covers menu item checks after the creation of DesktopShellLauncherContextMenu and ArcLauncherContextMenu. I am closing this cl. The issue is covered by two splitted cls 1. Pure refactor https://codereview.chromium.org/1857213004 2. Arc integration https://codereview.chromium.org/1869063002/ ==========
lgcheng@google.com changed reviewers: - stevenjb@chromium.org |
