Fix cannot select and activate pinned v2app through launcher context menu

chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc

Index: chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
index 045b01a9d2877720e16dcade1608d86a6c64e609..5a7096e3693c165565324fd48ebe6fa7685422e6 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
@@ -647,7 +647,8 @@ ChromeLauncherControllerImpl::GetV1ApplicationsFromAppId(
 void ChromeLauncherControllerImpl::ActivateShellApp(const std::string& app_id,
                                                     int window_index) {
   const ash::ShelfItem* item = GetItem(GetShelfIDForAppID(app_id));
-  if (item && item->type == ash::TYPE_APP) {
+  if (item &&
+      (item->type == ash::TYPE_APP || item->type == ash::TYPE_APP_SHORTCUT)) {
     LauncherItemController* controller = GetLauncherItemController(item->id);
     AppWindowLauncherItemController* app_window_controller =
         static_cast<AppWindowLauncherItemController*>(controller);

[msw, 2017/01/11 20:01:55]

It is really safe to static-cast the controller for a shortcut item to a
AppWindowLauncherItemController? Won't some of those items instead be associated
with AppShortcutLauncherItemController instances? This seems potentially
dangerous. Perhaps something should be changing the item type to APP when it
starts running?

It'd be nice if LauncherItemController had a virtual function that could be
called here (like Launch or Activate), overridden to handle apps and shortcuts
as needed... We really need to to reduce the complexity here.

It's possible that I broke this recently, but AFAICT, this function did not run
for shortcuts before my these recent changes:
https://codereview.chromium.org/2545923004
https://codereview.chromium.org/2551243002
I'd like a bisect if possible to find how this defect was introduced.

[Qiang(Joe) Xu, 2017/01/11 21:09:14]

ok thanks, let me first bisect to check why it becomes TYPE_APP_SHORTCUT.

[msw, 2017/01/11 21:54:44]

Your current change seems correct, albeit lacking clear safeguards.

Prior to my CL, ash::TYPE_APP_SHORTCUT (pinned) items would get a *separate*
LauncherItemController::TYPE_APP type value (and have their controller replaced)
here:
https://codereview.chromium.org/2545923004/diff/80001/chrome/browser/ui/ash/l...
And my CL changed ChromeLauncherControllerImpl::ActivateShellApp to check the
ash::ShelfItemType designation instead of the secondary
LauncherItemController::Type designation:
https://codereview.chromium.org/2545923004/diff/80001/chrome/browser/ui/ash/l...

That same code path (ExtensionAppWindowLauncherController::RegisterApp) still
replaces the controller for ash::TYPE_APP_SHORTCUT (pinned) items. So, a pinned,
not-running shortcut item with an associated AppShortcutLauncherItemController
*could* call through to  ChromeLauncherControllerImpl::ActivateShellApp, but
that shouldn't be done in practice.

ActivateShellApp is only called by ChromeLauncherAppMenuItemV2App::Execute, and
those are menu items are only instantiated in
ExtensionAppWindowLauncherItemController::GetApplicationList and
ArcAppWindowLauncherItemController::GetApplicationList; so the controller
*should be* one of those two items, and both are subclasses of
AppWindowLauncherItemController, so it should be safe to use this static cast.

Let me know what you think here; it may just be okay to land as-is.

Side note: We don't want to change pre-pinned ash::TYPE_APP_SHORTCUT items to
ash::TYPE_APP items while they're running, because the opposite is done for
unpinned running items that are then explicitly pinned
(ChromeLauncherControllerImpl::Pin converts TYPE_APP->TYPE_APP_SHORTCUT; I'm not
sure if the controller is changed anywhere in that process, likely not)

[Qiang(Joe) Xu, 2017/01/11 22:34:21]

<<<<<<<<<<<<<<<<Thanks for this info.
<<<<<<<<<<<< I agree with this.
<<<<<<<<<<<< Since we get the controller* and cast it to execute in this limited
scope, for me it should be fine.

[msw, 2017/01/11 22:52:26]

On 2017/01/11 22:34:21, Qiang(Joe) Xu wrote:
< Since we get the controller* and cast it to execute in this limited scope, for
me it should be fine.

nit: It would help if you add a comment explaining that "This should only ever
be called for items associated with AppWindowLauncherItemController instances."
or similar; at least then someone can dig up this CL if they want to see why we
think that's the case for now.

Otherwise, we could add a function like:
AppWindowLauncherItemController*
LauncherItemController::AsAppWindowLauncherItemController() { return nullptr; }
which would be overridden by the AppWindowLauncherItemController subclass to
|return this;|.

I really hope this is cleaned up soon; hopefully I'll get to it or someone else
is working on simplifying shelf code...

[James Cook, 2017/01/11 22:55:24]

Can you add some documentation in the code explaining why this is safe?

It would also be nice if there was something we could DCHECK. I'm not sure what
that would be, however.