Refactor of LauncherContextMenu.

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}

[lgcheng, 2016-04-06 20:01:35]

Description was changed from

==========
Refactor of exsiting LauncherContextMenu. Add derived classes
ExtensionLauncherContextMenu to handle extension app and
DesktopShellLauncherContextMenu to handle right click on desktopshell.
Nits


Pure refactor of Launcher Context Menu


Test


Nits


Nit


Arc implementation for shelf launcher context menu


 Arc app integration in shelf launcher context menu


Rebase--Arc app integration for shelf launcher context menu


Arc app integration in shelf launcher context menu


Arc app integration in shelf laucnher context menu


Comment update


Implement context menu for Arc app. Move implementation to right class.


Implement context menu for Arc app


Revert one patch. Put specific code in the right class


Implement context menu for Arc app.


Refactor. Same command id falls into same code


Implemetation context menu for Arc app in shelf.


nit


nit


update


nit


nit


nit


nit


nit


Refactor and include files clean up


Refactor


nit


Nit


Patch


Implement contextmenu for Arc app


FREEZE.unindexed


FREEZE.indexed

BUG=
==========

to

==========
Refactor of exsiting LauncherContextMenu. Add derived classes
ExtensionLauncherContextMenu to handle extension app and
DesktopShellLauncherContextMenu to handle right click on desktopshell.
==========

[lgcheng, 2016-04-06 20:01:35]

lgcheng@google.com changed reviewers:
+ stevenjb@chromium.org

[lgcheng, 2016-04-06 20:12:12]

Hi Steven

PTAL at this patch

This is the pure refactor of exsiting LauncherContextMenu with all the Arc
related code removed.

Thanks,

[lgcheng, 2016-04-06 20:14:29]

Description was changed from

==========
Refactor of exsiting LauncherContextMenu. Add derived classes
ExtensionLauncherContextMenu to handle extension app and
DesktopShellLauncherContextMenu to handle right click on desktopshell.
==========

to

==========
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/
==========

[lgcheng, 2016-04-06 20:40:12]

The CQ bit was checked by lgcheng@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 20:40:45]

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

[stevenjb, 2016-04-06 22:19:23]

Thanks for splitting this up. Much easier to do a more careful review this way.

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right):

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:183: return
LauncherContextMenu::ExecuteCommand(command_id, event_flags);
Instead of adding a separate method here, have Execute return true or false,
e.g.:

if (LauncherContextMenu::ExecuteCommand(command_id, event_flags))
  return;

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:71: NOTREACHED();
As long as we never instantiate a LauncherContextMenu, you should be able to
just not implement this (which will also ensure at the compiler level that we
never instantiate LauncherContextMenu). Same with IsItemForCommandIdDynamic() I
suspect.

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:160: void
LauncherContextMenu::AddAutohideAlignmentWallpaperMenu() {
nit: I think this would be better as 'AddShelfOptionsMenu' with a header comment
'Add common shelf options, e.g. autohide, alignment, and set wallpaper'.
Otherwise it's a little confusing why we add these to the context menu for, e.g.
an extension.

[lgcheng, 2016-04-06 22:59:54]

Hi Steven,

PTAL at the update.

Thanks,

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right):

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:183: return
LauncherContextMenu::ExecuteCommand(command_id, event_flags);
On 2016/04/06 22:19:23, stevenjb wrote:
> Instead of adding a separate method here, have Execute return true or false,
> e.g.:
> 
> if (LauncherContextMenu::ExecuteCommand(command_id, event_flags))
>   return;

Done. This is better implementation.

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:71: NOTREACHED();
On 2016/04/06 22:19:23, stevenjb wrote:
> As long as we never instantiate a LauncherContextMenu, you should be able to
> just not implement this (which will also ensure at the compiler level that we
> never instantiate LauncherContextMenu). Same with IsItemForCommandIdDynamic()
I
> suspect.

The behavior of following implementations here:

LauncherContextMenu::GetLabelForCommandId(int command_id)
IsItemForCommandIdDynamic(int command_id)

is shared by sub classes DesktopLauncherContextMenu and ArcLauncherContextMenu
(comes in next patch). Do you think it is better to leave them here? Otherwise I
will have to have duplicate code in two separate sub classes.

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:160: void
LauncherContextMenu::AddAutohideAlignmentWallpaperMenu() {
On 2016/04/06 22:19:23, stevenjb wrote:
> nit: I think this would be better as 'AddShelfOptionsMenu' with a header
comment
> 'Add common shelf options, e.g. autohide, alignment, and set wallpaper'.
> Otherwise it's a little confusing why we add these to the context menu for,
e.g.
> an extension.
> 

Done.

[lgcheng, 2016-04-06 23:14:25]

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right):

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
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:
> On 2016/04/06 22:19:23, stevenjb wrote:
> > Instead of adding a separate method here, have Execute return true or false,
> > e.g.:
> > 
> > if (LauncherContextMenu::ExecuteCommand(command_id, event_flags))
> >   return;
> 
> Done. This is better implementation.

Done. ExecuteComand is an overridden function. I call it in the wrapper function
and return true/false. Your suggestion is better implementation

[stevenjb, 2016-04-07 00:40:34]

lgtm with a question and updated comment.

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:71: NOTREACHED();
On 2016/04/06 22:59:54, lgcheng wrote:
> On 2016/04/06 22:19:23, stevenjb wrote:
> > As long as we never instantiate a LauncherContextMenu, you should be able to
> > just not implement this (which will also ensure at the compiler level that
we
> > never instantiate LauncherContextMenu). Same with
IsItemForCommandIdDynamic()
> I
> > suspect.
> 
> The behavior of following implementations here:
> 
> LauncherContextMenu::GetLabelForCommandId(int command_id)
> IsItemForCommandIdDynamic(int command_id)
> 
> is shared by sub classes DesktopLauncherContextMenu and ArcLauncherContextMenu
> (comes in next patch). Do you think it is better to leave them here? Otherwise
I
> will have to have duplicate code in two separate sub classes.

Is this NOTREACHED() in Desktop and Arc? If the implementation is indeed common
to both, that's fine, you can leave these here.

https://codereview.chromium.org/1857213004/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right):

https://codereview.chromium.org/1857213004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_context_menu.h:76: // by base class.
Handler it if true is returned.
Update comment, e.g.:
// Helper method to execute common commands. Returns true if handled.

[lgcheng, 2016-04-07 01:28:09]

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/l...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

https://codereview.chromium.org/1857213004/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:71: NOTREACHED();
On 2016/04/07 00:40:34, stevenjb wrote:
> On 2016/04/06 22:59:54, lgcheng wrote:
> > On 2016/04/06 22:19:23, stevenjb wrote:
> > > As long as we never instantiate a LauncherContextMenu, you should be able
to
> > > just not implement this (which will also ensure at the compiler level that
> we
> > > never instantiate LauncherContextMenu). Same with
> IsItemForCommandIdDynamic()
> > I
> > > suspect.
> > 
> > The behavior of following implementations here:
> > 
> > LauncherContextMenu::GetLabelForCommandId(int command_id)
> > IsItemForCommandIdDynamic(int command_id)
> > 
> > is shared by sub classes DesktopLauncherContextMenu and
ArcLauncherContextMenu
> > (comes in next patch). Do you think it is better to leave them here?
Otherwise
> I
> > will have to have duplicate code in two separate sub classes.
> 
> Is this NOTREACHED() in Desktop and Arc? If the implementation is indeed
common
> to both, that's fine, you can leave these here.

This code is called only when IsItemForCommandIdDynamic(int command_id) returns
true. So it should never be reached both in Desktop Shell and Arc. Arc and
Desktop shell don't have dynamic label for context menu item

https://codereview.chromium.org/1857213004/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right):

https://codereview.chromium.org/1857213004/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_context_menu.h:76: // by base class.
Handler it if true is returned.
On 2016/04/07 00:40:34, stevenjb wrote:
> Update comment, e.g.:
> // Helper method to execute common commands. Returns true if handled.

Done.

[lgcheng, 2016-04-07 01:28:17]

The CQ bit was checked by lgcheng@google.com

[lgcheng, 2016-04-07 01:28:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1857213004/#ps60001
(title: "Nits update")

[commit-bot: I haz the power, 2016-04-07 01:29:10]

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

[commit-bot: I haz the power, 2016-04-07 03:53:29]

Description was changed from

==========
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/
==========

to

==========
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/
==========

[commit-bot: I haz the power, 2016-04-07 03:53:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-07 03:54:42]

Description was changed from

==========
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/
==========

to

==========
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/

Committed: https://crrev.com/6b2dd155d13421f2e62bd8028f67bcba36afb6ea
Cr-Commit-Position: refs/heads/master@{#385648}
==========

[commit-bot: I haz the power, 2016-04-07 03:54:43]

Patchset 4 (id:??) landed as
https://crrev.com/6b2dd155d13421f2e62bd8028f67bcba36afb6ea
Cr-Commit-Position: refs/heads/master@{#385648}

[lgcheng, 2016-04-07 17:48:24]

Description was changed from

==========
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/

Committed: https://crrev.com/6b2dd155d13421f2e62bd8028f67bcba36afb6ea
Cr-Commit-Position: refs/heads/master@{#385648}
==========

to

==========
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}
==========