Adding ChromeLauncherController interface.

Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into ChromeLauncherController interface and ChromeLauncherControllerImpl implementation. With a few exceptions, classes that interact with ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement ChromeLauncherController interface. Various parts of the code rely on this functionality. Particularly on methods from ash::ShelfDelegate that it implements. This was resolved in one of three ways:
- Using ash::Shell::GetShelfDelegate() (the reason this couldn't be used everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=619605
Committed: https://crrev.com/5621dd87cde37a495f22eeaaee24337eada0f826
Cr-Commit-Position: refs/heads/master@{#399527}

[mfomitchev, 2016-06-10 18:10:22]

Description was changed from

==========
NOT FOR REVIEW: ChromeLauncherController interface.

BUG=NONE
==========

to

==========
Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into
ChromeLauncherController interface and ChromeLauncherControllerImpl
implementation. With a few exceptions, classes that interact with
ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of
ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement
ChromeLauncherController interface. Various parts of the code rely on this
functionality. Particularly on methods from ash::ShelfDelegate that it
implements. This was resolved in one of three ways:
- using ash::Shell::GetShelfDelegate() (the reason this couldn't be used
everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=NONE
==========

[mfomitchev, 2016-06-10 18:12:09]

Description was changed from

==========
Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into
ChromeLauncherController interface and ChromeLauncherControllerImpl
implementation. With a few exceptions, classes that interact with
ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of
ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement
ChromeLauncherController interface. Various parts of the code rely on this
functionality. Particularly on methods from ash::ShelfDelegate that it
implements. This was resolved in one of three ways:
- using ash::Shell::GetShelfDelegate() (the reason this couldn't be used
everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=NONE
==========

to

==========
Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into
ChromeLauncherController interface and ChromeLauncherControllerImpl
implementation. With a few exceptions, classes that interact with
ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of
ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement
ChromeLauncherController interface. Various parts of the code rely on this
functionality. Particularly on methods from ash::ShelfDelegate that it
implements. This was resolved in one of three ways:
- Using ash::Shell::GetShelfDelegate() (the reason this couldn't be used
everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=NONE
==========

[mfomitchev, 2016-06-10 18:14:24]

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

[msw, 2016-06-10 19:24:59]

I haven't taken a full look, but wanted to run an idea by you.

https://codereview.chromium.org/2052013002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/bookmark_app_helper.cc (right):

https://codereview.chromium.org/2052013002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/bookmark_app_helper.cc:720:
ash::Shell::GetInstance()->GetShelfDelegate()->PinAppWithID(extension->id());
We should avoid adding Shell::GetInstance calls to chrome/browser code...
Maybe still use ChromeLauncherController::instance() like arc_app_utils.cc, etc?

https://codereview.chromium.org/2052013002/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/2052013002/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:41: return
ash::Shell::GetInstance()->GetShelfDelegate()->IsAppPinned(app_id);
Ditto, we should avoid adding Shell::GetInstance calls to chrome code. Perhaps
we could add an interface ("ChromeShelfDelegate"?) to access ShelfDelegate-like
functions from Chrome code, and then the non-mash chromeos impl can call
directly into the corresponding ash::Shell::GetInstance()->GetShelfDelegate()
function, while the mash impl caches the ShelfDelegate state and does everything
over mojo interfaces. Does that sound reasonable? Do you think it'd be a
reasonable precursor to this CL?

[mfomitchev, 2016-06-10 20:42:47]

The usage of ash::ShellDelegate in chrome is certainly is a problem. This CL
makes the problem a bit more obvious, but IMO this is not necessarily a bad
thing. In my view using ChromeLauncherController::instance() "as an"
ash::ShellDelegate from chrome isn't much better than using ash::Shell.
Before this CL those uses fail because ChromeLauncherController::instance() is
null. After this CL they will fail because ash::Shell() is null (in chrome).
Yeah, we are linking against ash::Shell, which sucks, but we were linking
against a bunch of ash classes though ChromeLauncherController before anyway.

There is a similar problem with LauncherAppUpdater::Delegate, although that is
easier to solve, since it doesn't need to happen over IPC in Mustash.

At the same time, this CL together with https://crrev.com/2059963002/ makes it
possible to launch apps in Mustash, and also lays the foundation for 
implementing the rest of the functionality. So it does move the needle forward.

So, to sum up, my view is that we don't need to block this CL on solving the
ash::ShellDelegate problem. Finally, I need to focus on animation work next
week,
and if we wait - this code will rot.

[msw, 2016-06-10 21:43:01]

On 2016/06/10 20:42:47, mfomitchev wrote:
> The usage of ash::ShellDelegate in chrome is certainly is a problem. This CL
> makes the problem a bit more obvious, but IMO this is not necessarily a bad
> thing. In my view using ChromeLauncherController::instance() "as an"
> ash::ShellDelegate from chrome isn't much better than using ash::Shell.
> Before this CL those uses fail because ChromeLauncherController::instance() is
> null. After this CL they will fail because ash::Shell() is null (in chrome).
> Yeah, we are linking against ash::Shell, which sucks, but we were linking
> against a bunch of ash classes though ChromeLauncherController before anyway.
> 
> There is a similar problem with LauncherAppUpdater::Delegate, although that is
> easier to solve, since it doesn't need to happen over IPC in Mustash.
> 
> At the same time, this CL together with https://crrev.com/2059963002/ makes it
> possible to launch apps in Mustash, and also lays the foundation for 
> implementing the rest of the functionality. So it does move the needle
forward.
> 
> So, to sum up, my view is that we don't need to block this CL on solving the
> ash::ShellDelegate problem. Finally, I need to focus on animation work next
> week,
> and if we wait - this code will rot.

Okay, we can defer fixing the ash::Shell uses; I'll take a closer look now.

[msw, 2016-06-10 22:44:39]

Mostly nits, just one bigger LauncherContextMenu question.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:49: return
ChromeLauncherController::instance()->IsOpen(id);
optional nit: |return id && ...| to combine with the conditional above.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:430:
shelf_id = shelf_delegate_->GetShelfIDForAppID(app_id);
nit: just call ash::Shell::GetInstance()->GetShelfDelegate() here and remove the
|shelf_delegate_| member?

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:8: #include <map>
nit: not needed?

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:60: static void
SetInstance(ChromeLauncherController* instance) {
nit: set_instance for simple accessor

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:67: // 
ChromeLauncherController();
nit: Remove

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:74: virtual
ash::ShelfID CreateAppLauncherItem(LauncherItemController* controller,
Just a note, eventually we'll probably want to avoid using ShelfIDs throughout
this interface, and just operate on |app_id|s.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:435:
AppListControllerDelegate::Pinnable ChromeLauncherControllerImpl::GetPinnable(
nit: the order of definitions should match the order of declarations, but I
guess that was broken beforehand, so it's not a blocker.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:53: // A list
of the elements which makes up a simple menu description.
nit: you should be able to remove this and just inherit the typedef in
chrome_launcher_controller.h

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:159: //
ash::ShelfDelegate:
nit: if possible, we should make the base class overrides private.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:399:

optional nit: remove blank line

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:907:
GetChromeLauncherControllerImpl()->LaunchApp(last_loaded_extension_id(),
nit: use |controller_| here and in other fixtures below?

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:142:
if (app_shelf_id == app_id)
nit: curly braces

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:52: const std::string&
app_id = controller->GetAppIDForShelfID(item->id);
Can this instead call
ash::Shell::GetInstance()->GetShelfDelegate()->GetAppIDForShelfID(), and then
can LauncherContextMenu and its subclasses take a pointer to the
ChromeLauncherController interface, or is there some other reason that launcher
context menus need pointers to the impl subclass?

[mfomitchev, 2016-06-10 23:29:10]

Ack to most nits, posted responses to some.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:430:
shelf_id = shelf_delegate_->GetShelfIDForAppID(app_id);
On 2016/06/10 22:44:38, msw wrote:
> nit: just call ash::Shell::GetInstance()->GetShelfDelegate() here and remove
the
> |shelf_delegate_| member?

This is not so simple because of the unit tests - see the response to the
comment int launcher_context_menu.cc. If I change this, some tests will start
failing.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:435:
AppListControllerDelegate::Pinnable ChromeLauncherControllerImpl::GetPinnable(
On 2016/06/10 22:44:38, msw wrote:
> nit: the order of definitions should match the order of declarations, but I
> guess that was broken beforehand, so it's not a blocker.

Yeah, I also didn't want to move code around in the same CL as the CL that
changes the code, since that makes it a lot harder to review. I will update the
order in a follow-up (which won't change the code itself)

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:159: //
ash::ShelfDelegate:
On 2016/06/10 22:44:38, msw wrote:
> nit: if possible, we should make the base class overrides private.

Hmm.. this might be possible for some of them, but not for ash::ShelfDelegate,
since that is still used in a bunch of places for ChromeLauncherConrollerImpl.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:52: const std::string&
app_id = controller->GetAppIDForShelfID(item->id);
On 2016/06/10 22:44:39, msw wrote:
> Can this instead call
> ash::Shell::GetInstance()->GetShelfDelegate()->GetAppIDForShelfID(), and then
> can LauncherContextMenu and its subclasses take a pointer to the
> ChromeLauncherController interface, or is there some other reason that
launcher
> context menus need pointers to the impl subclass?

Yeah, that's what I tried to do at first. Problem is, this gets exercised by
unit tests (ChromeLauncherControllerTest, LauncherContextMenuTest) which create
their own  ChromeLauncherController instances that different from
ash::Shell::GetShelfDelegate(), and then the tests fail if we use
ash::Shell::GetShelfDelegate() here. 

I suppose we could rewrite the tests, but I think we can't use
LauncherContextMenu for Mash as-is anyway, so I think we can address this in a
follow-up CL once the usage of LauncherContextMenu becomes more clear.

[msw, 2016-06-10 23:42:52]

lgtm, hoping you'll address some nits

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:430:
shelf_id = shelf_delegate_->GetShelfIDForAppID(app_id);
On 2016/06/10 23:29:09, mfomitchev wrote:
> On 2016/06/10 22:44:38, msw wrote:
> > nit: just call ash::Shell::GetInstance()->GetShelfDelegate() here and remove
> the
> > |shelf_delegate_| member?
> 
> This is not so simple because of the unit tests - see the response to the
> comment int launcher_context_menu.cc. If I change this, some tests will start
> failing.

Acknowledged.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:435:
AppListControllerDelegate::Pinnable ChromeLauncherControllerImpl::GetPinnable(
On 2016/06/10 23:29:09, mfomitchev wrote:
> On 2016/06/10 22:44:38, msw wrote:
> > nit: the order of definitions should match the order of declarations, but I
> > guess that was broken beforehand, so it's not a blocker.
> 
> Yeah, I also didn't want to move code around in the same CL as the CL that
> changes the code, since that makes it a lot harder to review. I will update
the
> order in a follow-up (which won't change the code itself)

Acknowledged.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:159: //
ash::ShelfDelegate:
On 2016/06/10 23:29:09, mfomitchev wrote:
> On 2016/06/10 22:44:38, msw wrote:
> > nit: if possible, we should make the base class overrides private.
> 
> Hmm.. this might be possible for some of them, but not for ash::ShelfDelegate,
> since that is still used in a bunch of places for ChromeLauncherConrollerImpl.

Acknowledged.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:52: const std::string&
app_id = controller->GetAppIDForShelfID(item->id);
On 2016/06/10 23:29:09, mfomitchev wrote:
> On 2016/06/10 22:44:39, msw wrote:
> > Can this instead call
> > ash::Shell::GetInstance()->GetShelfDelegate()->GetAppIDForShelfID(), and
then
> > can LauncherContextMenu and its subclasses take a pointer to the
> > ChromeLauncherController interface, or is there some other reason that
> launcher
> > context menus need pointers to the impl subclass?
> 
> Yeah, that's what I tried to do at first. Problem is, this gets exercised by
> unit tests (ChromeLauncherControllerTest, LauncherContextMenuTest) which
create
> their own  ChromeLauncherController instances that different from
> ash::Shell::GetShelfDelegate(), and then the tests fail if we use
> ash::Shell::GetShelfDelegate() here. 
> 
> I suppose we could rewrite the tests, but I think we can't use
> LauncherContextMenu for Mash as-is anyway, so I think we can address this in a
> follow-up CL once the usage of LauncherContextMenu becomes more clear.

Acknowledged.

[mfomitchev, 2016-06-13 15:55:32]

Description was changed from

==========
Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into
ChromeLauncherController interface and ChromeLauncherControllerImpl
implementation. With a few exceptions, classes that interact with
ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of
ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement
ChromeLauncherController interface. Various parts of the code rely on this
functionality. Particularly on methods from ash::ShelfDelegate that it
implements. This was resolved in one of three ways:
- Using ash::Shell::GetShelfDelegate() (the reason this couldn't be used
everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=NONE
==========

to

==========
Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into
ChromeLauncherController interface and ChromeLauncherControllerImpl
implementation. With a few exceptions, classes that interact with
ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of
ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement
ChromeLauncherController interface. Various parts of the code rely on this
functionality. Particularly on methods from ash::ShelfDelegate that it
implements. This was resolved in one of three ways:
- Using ash::Shell::GetShelfDelegate() (the reason this couldn't be used
everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=619605
==========

[mfomitchev, 2016-06-13 17:29:18]

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:49: return
ChromeLauncherController::instance()->IsOpen(id);
On 2016/06/10 22:44:38, msw wrote:
> optional nit: |return id && ...| to combine with the conditional above.

Done.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:8: #include <map>
On 2016/06/10 22:44:38, msw wrote:
> nit: not needed?

Done.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:60: static void
SetInstance(ChromeLauncherController* instance) {
On 2016/06/10 22:44:38, msw wrote:
> nit: set_instance for simple accessor

Done.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:67: // 
ChromeLauncherController();
On 2016/06/10 22:44:38, msw wrote:
> nit: Remove

Done.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:74: virtual
ash::ShelfID CreateAppLauncherItem(LauncherItemController* controller,
On 2016/06/10 22:44:38, msw wrote:
> Just a note, eventually we'll probably want to avoid using ShelfIDs throughout
> this interface, and just operate on |app_id|s.

Acknowledged.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h (right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.h:53: // A list
of the elements which makes up a simple menu description.
On 2016/06/10 22:44:38, msw wrote:
> nit: you should be able to remove this and just inherit the typedef in
> chrome_launcher_controller.h

Done.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:399:

On 2016/06/10 22:44:39, msw wrote:
> optional nit: remove blank line

Done.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_browsertest.cc:907:
GetChromeLauncherControllerImpl()->LaunchApp(last_loaded_extension_id(),
On 2016/06/10 22:44:38, msw wrote:
> nit: use |controller_| here and in other fixtures below?

Done.

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2052013002/diff/140001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/extension_app_window_launcher_controller.cc:142:
if (app_shelf_id == app_id)
On 2016/06/10 22:44:39, msw wrote:
> nit: curly braces

Done.

[mfomitchev, 2016-06-13 17:29:36]

mfomitchev@chromium.org changed reviewers:
+ rockot@chromium.org

[mfomitchev, 2016-06-13 17:29:37]

rockot@chromium.org: Please review changes in 

chrome/browser/extensions/bookmark_app_helper.cc

[Ken Rockot(use gerrit already), 2016-06-13 17:37:23]

lgtm

[mfomitchev, 2016-06-13 17:57:36]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-06-13 17:57:37]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2052013002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-13 17:58:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052013002/180001

[commit-bot: I haz the power, 2016-06-13 20:35:49]

Description was changed from

==========
Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into
ChromeLauncherController interface and ChromeLauncherControllerImpl
implementation. With a few exceptions, classes that interact with
ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of
ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement
ChromeLauncherController interface. Various parts of the code rely on this
functionality. Particularly on methods from ash::ShelfDelegate that it
implements. This was resolved in one of three ways:
- Using ash::Shell::GetShelfDelegate() (the reason this couldn't be used
everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=619605
==========

to

==========
Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into
ChromeLauncherController interface and ChromeLauncherControllerImpl
implementation. With a few exceptions, classes that interact with
ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of
ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement
ChromeLauncherController interface. Various parts of the code rely on this
functionality. Particularly on methods from ash::ShelfDelegate that it
implements. This was resolved in one of three ways:
- Using ash::Shell::GetShelfDelegate() (the reason this couldn't be used
everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=619605
==========

[commit-bot: I haz the power, 2016-06-13 20:35:51]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-13 20:36:16]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-13 20:37:35]

Description was changed from

==========
Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into
ChromeLauncherController interface and ChromeLauncherControllerImpl
implementation. With a few exceptions, classes that interact with
ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of
ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement
ChromeLauncherController interface. Various parts of the code rely on this
functionality. Particularly on methods from ash::ShelfDelegate that it
implements. This was resolved in one of three ways:
- Using ash::Shell::GetShelfDelegate() (the reason this couldn't be used
everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=619605
==========

to

==========
Adding ChromeLauncherController interface.

Breaking up the existing ChromeLauncherController class into
ChromeLauncherController interface and ChromeLauncherControllerImpl
implementation. With a few exceptions, classes that interact with
ChromeLauncherController now do it through ChromeLauncherController interface.

This is the first step in adding a Mustash implementation of
ChromeLauncherController.

ChromeLauncherControllerImpl does a lot more than implement
ChromeLauncherController interface. Various parts of the code rely on this
functionality. Particularly on methods from ash::ShelfDelegate that it
implements. This was resolved in one of three ways:
- Using ash::Shell::GetShelfDelegate() (the reason this couldn't be used
everywhere is unit tests)
- Using ChromeLauncherControllerImpl instead of ChromeLauncherController
- Storing separate pointers for ChromeLauncherController and ash::ShellDelegate.

BUG=619605

Committed: https://crrev.com/5621dd87cde37a495f22eeaaee24337eada0f826
Cr-Commit-Position: refs/heads/master@{#399527}
==========

[commit-bot: I haz the power, 2016-06-13 20:37:37]

Patchset 10 (id:??) landed as
https://crrev.com/5621dd87cde37a495f22eeaaee24337eada0f826
Cr-Commit-Position: refs/heads/master@{#399527}