Fix cannot select and activate pinned v2app through launcher context menu

Fix cannot select and activate pinned v2app through launcher context menu

Changes:
Bug happens when v2app like Files is pinned, the ash::ShelfItemType for this case is TYPE_APP_SHORTCUT.

BUG=675032
TEST=emulator test saw bug fixed

Review-Url: https://codereview.chromium.org/2628023002
Cr-Commit-Position: refs/heads/master@{#443078}
Committed: https://chromium.googlesource.com/chromium/src/+/2ad11d9a384d881c1ea962e4bcc6a269b7781259

[Qiang(Joe) Xu, 2017-01-11 19:16:45]

Description was changed from

==========
Fix cannot select and activate pinned v2app through launcher context menu

BUG=675032
TEST=emulator test saw bug fixed
==========

to

==========
Fix cannot select and activate pinned v2app through launcher context menu

Changes:
Bug happens when v2app like Files is pinned, the ash::ShelfItemType for this
case is TYPE_APP_SHORTCUT.

BUG=675032
TEST=emulator test saw bug fixed
==========

[Qiang(Joe) Xu, 2017-01-11 19:16:45]

warx@chromium.org changed reviewers:
+ jamescook@chromium.org

[Qiang(Joe) Xu, 2017-01-11 19:16:51]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 19:17:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2017-01-11 19:19:28]

Hi James, ptal, thanks!

[James Cook, 2017-01-11 19:29:23]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2017-01-11 19:29:23]

LGTM but please have msw@ look also, since this might have been caused by his
recent changes.

msw, could this be a problem elsewhere in the code?

[commit-bot: I haz the power, 2017-01-11 19:54:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 19:55:00]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:654:
static_cast<AppWindowLauncherItemController*>(controller);
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.

[msw, 2017-01-11 20:44:14]

Please also check my latest comment on the bug. Perhaps the defect is just in
the menu item ordering?

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

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:654:
static_cast<AppWindowLauncherItemController*>(controller);
On 2017/01/11 20:01:55, msw wrote:
> 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.

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

[msw, 2017-01-11 21:19:24]

On 2017/01/11 21:09:14, Qiang(Joe) Xu wrote:
>
https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
> File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc
(right):
> 
>
https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
> chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:654:
> static_cast<AppWindowLauncherItemController*>(controller);
> On 2017/01/11 20:01:55, msw wrote:
> > 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.
> 
> ok thanks, let me first bisect to check why it becomes TYPE_APP_SHORTCUT.


The defect was introduced by my CL: https://codereview.chromium.org/2545923004
I'll be investigating. FYI: I bisected to find the revision range:
https://chromium.googlesource.com/chromium/src/+log/11c7629beff558f7f5822067d...

[msw, 2017-01-11 21:54:45]

Please take a look at my analysis and lmk what you think.

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:654:
static_cast<AppWindowLauncherItemController*>(controller);
On 2017/01/11 21:09:14, Qiang(Joe) Xu wrote:
> On 2017/01/11 20:01:55, msw wrote:
> > 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.
> 
> ok thanks, let me first bisect to check why it becomes TYPE_APP_SHORTCUT.

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]

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:654:
static_cast<AppWindowLauncherItemController*>(controller);
On 2017/01/11 21:54:44, msw wrote:
> On 2017/01/11 21:09:14, Qiang(Joe) Xu wrote:
> > On 2017/01/11 20:01:55, msw wrote:
> > > 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.
> > 
> > ok thanks, let me first bisect to check why it becomes TYPE_APP_SHORTCUT.
> 
> 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...
<<<<<<<<<<<<<<<<Thanks for this info.
> 
> 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.
<<<<<<<<<<<< I agree with this.
<<<<<<<<<<<< Since we get the controller* and cast it to execute in this limited
scope, for me it should be fine.


> 
> 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)

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

lgtm with a nit (comment or enforcement)

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:654:
static_cast<AppWindowLauncherItemController*>(controller);
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]

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2628023002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:654:
static_cast<AppWindowLauncherItemController*>(controller);
On 2017/01/11 22:34:21, Qiang(Joe) Xu wrote:
> On 2017/01/11 21:54:44, msw wrote:
> > On 2017/01/11 21:09:14, Qiang(Joe) Xu wrote:
> > > On 2017/01/11 20:01:55, msw wrote:
> > > > 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.
> > > 
> > > ok thanks, let me first bisect to check why it becomes TYPE_APP_SHORTCUT.
> > 
> > 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...
> <<<<<<<<<<<<<<<<Thanks for this info.
> > 
> > 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.
> <<<<<<<<<<<< I agree with this.
> <<<<<<<<<<<< Since we get the controller* and cast it to execute in this
limited
> scope, for me it should be fine.
> 
> 
> > 
> > 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)
> 

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.

[James Cook, 2017-01-11 22:56:46]

Ah, msw beat me to it. I like his suggestions.

[Qiang(Joe) Xu, 2017-01-11 23:32:22]

Patchset #2 (id:20001) has been deleted

[Qiang(Joe) Xu, 2017-01-11 23:35:01]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 23:35:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Qiang(Joe) Xu, 2017-01-11 23:40:39]

Hi all, please reexamine this CL based on msw@'s suggestion, thanks!

[msw, 2017-01-11 23:42:59]

lgtm

[commit-bot: I haz the power, 2017-01-12 00:09:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 00:09:57]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-01-12 00:09:59]

LGTM

[Qiang(Joe) Xu, 2017-01-12 00:11:09]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2017-01-12 00:12:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 00:24:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484179869606320,
"parent_rev": "eff8bd896afe40d5d2646857939ed03b76ca3f12", "commit_rev":
"2ad11d9a384d881c1ea962e4bcc6a269b7781259"}

[commit-bot: I haz the power, 2017-01-12 00:25:35]

Description was changed from

==========
Fix cannot select and activate pinned v2app through launcher context menu

Changes:
Bug happens when v2app like Files is pinned, the ash::ShelfItemType for this
case is TYPE_APP_SHORTCUT.

BUG=675032
TEST=emulator test saw bug fixed
==========

to

==========
Fix cannot select and activate pinned v2app through launcher context menu

Changes:
Bug happens when v2app like Files is pinned, the ash::ShelfItemType for this
case is TYPE_APP_SHORTCUT.

BUG=675032
TEST=emulator test saw bug fixed

Review-Url: https://codereview.chromium.org/2628023002
Cr-Commit-Position: refs/heads/master@{#443078}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ad11d9a384d881c1ea962e4bcc6...
==========

[commit-bot: I haz the power, 2017-01-12 00:25:36]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2ad11d9a384d881c1ea962e4bcc6...