AppListController refactoring part 3: Switching over to use AppListShower in Ash.

AppListController refactoring part 3: Switching over to use AppListShower in Ash.

Part 1 of the refactoring: https://crrev.com/1830293002
Part 2 of the refactoring: https://crrev.com/1856943003
Design doc: https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca6Q/edit?ts=56d0e9f0#heading=h.b2znwtuxd2x2

This CL makes Ash use app_list::AppListShowerImpl + ash::AppListShowerDelegate
tandem instead of ash::AppListController. AppListShower is owned by
AppListServiceAsh, and Ash goes to AppListServiceAsh (through
ChromeShellDelegate) when it needs to hide or show the app list.

BUG=557408
Committed: https://crrev.com/2b8b5cae9e892e575f6dcff36993f5a0694fa33b
Cr-Commit-Position: refs/heads/master@{#386136}

[mfomitchev, 2016-04-07 03:32:45]

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

[xiyuan, 2016-04-07 18:45:09]

lgtm

https://codereview.chromium.org/1861013004/diff/1/ash/test/test_shell_delegate.h
File ash/test/test_shell_delegate.h (right):

https://codereview.chromium.org/1861013004/diff/1/ash/test/test_shell_delegat...
ash/test/test_shell_delegate.h:39: app_list::AppListShowerImpl*
app_list_shower() {
nit: move accessors to the end of declarations

https://codereview.chromium.org/1861013004/diff/1/ash/test/test_shell_delegat...
ash/test/test_shell_delegate.h:86:
scoped_ptr<app_list::AppListShowerDelegateFactory>
nit: std::unqiue_ptr in new code

[mfomitchev, 2016-04-07 19:46:40]

https://codereview.chromium.org/1861013004/diff/1/ash/test/test_shell_delegate.h
File ash/test/test_shell_delegate.h (right):

https://codereview.chromium.org/1861013004/diff/1/ash/test/test_shell_delegat...
ash/test/test_shell_delegate.h:39: app_list::AppListShowerImpl*
app_list_shower() {
On 2016/04/07 18:45:09, xiyuan wrote:
> nit: move accessors to the end of declarations

Done, but.. is there a style guide for this? Should set_multi_profiles_enabled
also be moved? 

FWIW, I actually like it to have all the non-overriden methods (including the
accessors) before the overridden ones - this way it is clear where the
overridden block begins and where it ends, since every block ends with either
the comment for the next overridden block or the begginning of the next section
('private:' in this case).

https://codereview.chromium.org/1861013004/diff/1/ash/test/test_shell_delegat...
ash/test/test_shell_delegate.h:86:
scoped_ptr<app_list::AppListShowerDelegateFactory>
On 2016/04/07 18:45:09, xiyuan wrote:
> nit: std::unqiue_ptr in new code

Done.

[mfomitchev, 2016-04-07 19:48:05]

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

[mfomitchev, 2016-04-07 19:48:05]

@sky: Can you PTAL at the files that are not in chrome/browser/ui/app_list/?

[xiyuan, 2016-04-07 20:08:29]

https://codereview.chromium.org/1861013004/diff/1/ash/test/test_shell_delegate.h
File ash/test/test_shell_delegate.h (right):

https://codereview.chromium.org/1861013004/diff/1/ash/test/test_shell_delegat...
ash/test/test_shell_delegate.h:39: app_list::AppListShowerImpl*
app_list_shower() {
On 2016/04/07 19:46:40, mfomitchev wrote:
> On 2016/04/07 18:45:09, xiyuan wrote:
> > nit: move accessors to the end of declarations
> 
> Done, but.. is there a style guide for this? Should set_multi_profiles_enabled
> also be moved? 
> 
> FWIW, I actually like it to have all the non-overriden methods (including the
> accessors) before the overridden ones - this way it is clear where the
> overridden block begins and where it ends, since every block ends with either
> the comment for the next overridden block or the begginning of the next
section
> ('private:' in this case).

The style guide does not call this out explicitly. My impression comes from
existing code the sample in the style guide [1]. But I think keeping
non-overridden ones before overridden ones also makes sense. 

This is just a nit. So you can ignore my comment. If we move, let's move
set_multi_profiles_enabled as well to be consistent.

[1] https://google.github.io/styleguide/cppguide.html#Class_Format

[sky, 2016-04-07 21:43:28]

LGTM

https://codereview.chromium.org/1861013004/diff/20001/ash/shell/shell_delegat...
File ash/shell/shell_delegate_impl.h (right):

https://codereview.chromium.org/1861013004/diff/20001/ash/shell/shell_delegat...
ash/shell/shell_delegate_impl.h:9: #include <string>
include <utility>

https://codereview.chromium.org/1861013004/diff/20001/ash/test/test_shell_del...
File ash/test/test_shell_delegate.h (right):

https://codereview.chromium.org/1861013004/diff/20001/ash/test/test_shell_del...
ash/test/test_shell_delegate.h:9: #include <string>
include <utility>

[mfomitchev, 2016-04-08 15:50:44]

https://codereview.chromium.org/1861013004/diff/20001/ash/shell/shell_delegat...
File ash/shell/shell_delegate_impl.h (right):

https://codereview.chromium.org/1861013004/diff/20001/ash/shell/shell_delegat...
ash/shell/shell_delegate_impl.h:9: #include <string>
On 2016/04/07 21:43:27, sky wrote:
> include <utility>

I believe unique_ptr is in <memory>:
http://en.cppreference.com/w/cpp/memory/unique_ptr

https://codereview.chromium.org/1861013004/diff/20001/ash/test/test_shell_del...
File ash/test/test_shell_delegate.h (right):

https://codereview.chromium.org/1861013004/diff/20001/ash/test/test_shell_del...
ash/test/test_shell_delegate.h:9: #include <string>
On 2016/04/07 21:43:28, sky wrote:
> include <utility>

I believe unique_ptr is in <memory>:
http://en.cppreference.com/w/cpp/memory/unique_ptr

[sky, 2016-04-08 16:35:51]

Yep, SLGTM

[mfomitchev, 2016-04-08 17:49:36]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-04-08 17:49:37]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1861013004/#ps60001
(title: "Rebase.")

[mfomitchev, 2016-04-08 17:49:50]

The CQ bit was unchecked by mfomitchev@chromium.org

[mfomitchev, 2016-04-08 17:54:11]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2016-04-08 17:54:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861013004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861013004/60001

[commit-bot: I haz the power, 2016-04-08 18:41:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-08 18:43:10]

Description was changed from

==========
AppListController refactoring part 3: Switching over to use AppListShower in
Ash.

Part 1 of the refactoring: https://crrev.com/1830293002
Part 2 of the refactoring: https://crrev.com/1856943003
Design doc:
https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca...

This CL makes Ash use app_list::AppListShowerImpl + ash::AppListShowerDelegate
tandem instead of ash::AppListController. AppListShower is owned by
AppListServiceAsh, and Ash goes to AppListServiceAsh (through
ChromeShellDelegate) when it needs to hide or show the app list.

BUG=557408
==========

to

==========
AppListController refactoring part 3: Switching over to use AppListShower in
Ash.

Part 1 of the refactoring: https://crrev.com/1830293002
Part 2 of the refactoring: https://crrev.com/1856943003
Design doc:
https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca...

This CL makes Ash use app_list::AppListShowerImpl + ash::AppListShowerDelegate
tandem instead of ash::AppListController. AppListShower is owned by
AppListServiceAsh, and Ash goes to AppListServiceAsh (through
ChromeShellDelegate) when it needs to hide or show the app list.

BUG=557408

Committed: https://crrev.com/2b8b5cae9e892e575f6dcff36993f5a0694fa33b
Cr-Commit-Position: refs/heads/master@{#386136}
==========

[commit-bot: I haz the power, 2016-04-08 18:43:11]

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

[mfomitchev, 2016-04-08 20:14:07]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1866403006/ by mfomitchev@chromium.org.

The reason for reverting is: crrev.com/1856943003 was reverted, so this needs to
be reverted as well..