Remove LauncherItemController::Launch, cleanup.

Remove LauncherItemController::Launch, cleanup.

Inline the common implementation in ChromeLauncherController.
(avoid roundabout calls between the item delegate and controller)
(helps ShelfItemDelegate support ash<->chrome mojo apis for mash)
Also consolidate LaunchAppWithLaunchId and LaunchApp.

BUG=557406
TEST=Automated; no behavior changes when selecting chrome os shelf items.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2684723003
Cr-Commit-Position: refs/heads/master@{#449193}
Committed: https://chromium.googlesource.com/chromium/src/+/6c8f3be195f6688a8ee21bf6d982703d191335b3

[msw, 2017-02-08 01:46:39]

Description was changed from

==========
Remove LauncherItemController::Launch, cleanup.

BUG=557406
TEST=No behavior changes when selecting chrome os shelf items.
R=jamescook@chromium.org
==========

to

==========
Remove LauncherItemController::Launch, cleanup.

Inline the common implementation in ChromeLauncherController.
(avoid roundabout calls between the item delegate and controller)
Also consolidate LaunchAppWithLaunchId and LaunchApp.

BUG=557406
TEST=Automated; no behavior changes when selecting chrome os shelf items.
R=jamescook@chromium.org
==========

[msw, 2017-02-08 01:48:40]

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

[commit-bot: I haz the power, 2017-02-08 01:49:15]

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

[msw, 2017-02-08 02:16:15]

Hey James, please take a look; thanks!

[commit-bot: I haz the power, 2017-02-08 02:25:23]

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

[commit-bot: I haz the power, 2017-02-08 02:25:24]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-02-08 17:37:00]

Direction looks good. You might want to mention mash/mustash as a motivating
factor in the CL description.

+khmel and stevenjb just FYI, as they've been reviewing code for some Intel
folks who added the AppLauncherId thing that I'm recommending you use. We might
wanna FYI one of the Intel people.

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:141: const
std::string& launch_id,
I think this would be better if it took an AppLauncherId (from
chrome_launcher_prefs.h, though it really ought to be in chrome_launcher_types.h
or its own file). That class encapsulates both app_id and launch_id so you don't
have to pass two adjacent strings as arguments.

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_controller_helper.h (right):

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_controller_helper.h:45: const
std::string& launch_id,
ditto

[khmel, 2017-02-08 17:41:26]

khmel@chromium.org changed reviewers:
+ khmel@chromium.org

[khmel, 2017-02-08 17:41:27]

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:141: const
std::string& launch_id,
On 2017/02/08 17:37:00, James Cook wrote:
> I think this would be better if it took an AppLauncherId (from
> chrome_launcher_prefs.h, though it really ought to be in
chrome_launcher_types.h
> or its own file). That class encapsulates both app_id and launch_id so you
don't
> have to pass two adjacent strings as arguments.

+1 to this. There is also CL: https://codereview.chromium.org/2684853002/ where
AppLauncherId is moved to separate file.

[stevenjb, 2017-02-08 18:06:46]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2017-02-08 18:06:46]

cc:rafael.antognolli@intel.com FYI. This should not interfere with ongoing
launcher id work.

[msw, 2017-02-09 00:19:39]

Description was changed from

==========
Remove LauncherItemController::Launch, cleanup.

Inline the common implementation in ChromeLauncherController.
(avoid roundabout calls between the item delegate and controller)
Also consolidate LaunchAppWithLaunchId and LaunchApp.

BUG=557406
TEST=Automated; no behavior changes when selecting chrome os shelf items.
R=jamescook@chromium.org
==========

to

==========
Remove LauncherItemController::Launch, cleanup.

Inline the common implementation in ChromeLauncherController.
(avoid roundabout calls between the item delegate and controller)
(helps ShelfItemDelegate support ash<->chrome mojo apis for mash)
Also consolidate LaunchAppWithLaunchId and LaunchApp.

BUG=557406
TEST=Automated; no behavior changes when selecting chrome os shelf items.
R=jamescook@chromium.org
==========

[msw, 2017-02-09 00:20:15]

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

[commit-bot: I haz the power, 2017-02-09 00:21:04]

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

[msw, 2017-02-09 00:24:58]

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

[msw, 2017-02-09 00:25:06]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-02-09 00:25:28]

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

[msw, 2017-02-09 00:25:48]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-02-09 00:27:48]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2017-02-09 00:42:00]

Done, please take another look; thanks.

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:141: const
std::string& launch_id,
On 2017/02/08 17:41:27, khmel wrote:
> On 2017/02/08 17:37:00, James Cook wrote:
> > I think this would be better if it took an AppLauncherId (from
> > chrome_launcher_prefs.h, though it really ought to be in
> chrome_launcher_types.h
> > or its own file). That class encapsulates both app_id and launch_id so you
> don't
> > have to pass two adjacent strings as arguments.
> 
> +1 to this. There is also CL: https://codereview.chromium.org/2684853002/
where
> AppLauncherId is moved to separate file.

Done.

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_controller_helper.h (right):

https://codereview.chromium.org/2684723003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_controller_helper.h:45: const
std::string& launch_id,
On 2017/02/08 17:37:00, James Cook wrote:
> ditto

Done.

[msw, 2017-02-09 00:42:08]

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

[commit-bot: I haz the power, 2017-02-09 00:42:57]

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

[stevenjb, 2017-02-09 00:56:54]

lgtm

[commit-bot: I haz the power, 2017-02-09 01:18:13]

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

[commit-bot: I haz the power, 2017-02-09 01:18:14]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-02-09 01:29:28]

LGTM. I wish ash::launcher::AppLauncherId wasn't in namespace launcher, but that
shouldn't block this CL.

[msw, 2017-02-09 02:27:33]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2017-02-09 02:27:55]

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-02-09 02:33:52]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486607253983450,
"parent_rev": "13084c992e7c9d28ecac0b5967e1cafaca49714d", "commit_rev":
"6c8f3be195f6688a8ee21bf6d982703d191335b3"}

[commit-bot: I haz the power, 2017-02-09 02:34:25]

Description was changed from

==========
Remove LauncherItemController::Launch, cleanup.

Inline the common implementation in ChromeLauncherController.
(avoid roundabout calls between the item delegate and controller)
(helps ShelfItemDelegate support ash<->chrome mojo apis for mash)
Also consolidate LaunchAppWithLaunchId and LaunchApp.

BUG=557406
TEST=Automated; no behavior changes when selecting chrome os shelf items.
R=jamescook@chromium.org
==========

to

==========
Remove LauncherItemController::Launch, cleanup.

Inline the common implementation in ChromeLauncherController.
(avoid roundabout calls between the item delegate and controller)
(helps ShelfItemDelegate support ash<->chrome mojo apis for mash)
Also consolidate LaunchAppWithLaunchId and LaunchApp.

BUG=557406
TEST=Automated; no behavior changes when selecting chrome os shelf items.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2684723003
Cr-Commit-Position: refs/heads/master@{#449193}
Committed:
https://chromium.googlesource.com/chromium/src/+/6c8f3be195f6688a8ee21bf6d982...
==========

[commit-bot: I haz the power, 2017-02-09 02:34:26]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6c8f3be195f6688a8ee21bf6d982...