mash: Migrate shelf app list button to wm common types

mash: Migrate shelf app list button to wm common types

This is needed to port the shelf to mash.

* Move app list show/hide/visible functions to WmShell
* Remove window parameter from ShowAppList/ToggleAppList because non-test
code always shows and hides on the default display
* Migrate ShelfModel ownership to WmShell

BUG=629250
TEST=ash_unittests, chrome unit_tests
TBR=dmazzoni@chromium.org

Committed: https://crrev.com/abe9e24cab6f3245c8583d773644d3b62d84d71a
Cr-Commit-Position: refs/heads/master@{#406646}

[James Cook, 2016-07-20 16:59:48]

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

[James Cook, 2016-07-20 17:00:05]

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

[James Cook, 2016-07-20 17:00:06]

msw, please take a look.

https://codereview.chromium.org/2169533002/diff/40001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.h (left):

https://codereview.chromium.org/2169533002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
I don't know why Reitveld shows these as modified and not deleted, but since
it's all git under the hood it shouldn't matter.

[commit-bot: I haz the power, 2016-07-20 17:00:18]

Dry run: 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, 2016-07-20 17:25:18]

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

[commit-bot: I haz the power, 2016-07-20 17:25:19]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[msw, 2016-07-20 17:33:50]

lgtm with optional nits

https://codereview.chromium.org/2169533002/diff/40001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2169533002/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:159: void ToggleAppList();
optional nit: This only really has two callers (HandleToggleAppList in
accelerator_controller.cc and AppListShelfItemDelegate::ItemSelected); it could
be removed and those places could call into the presenter directly or inline
similar behavior and then even the presenter method could go away.

https://codereview.chromium.org/2169533002/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:162: // GetAppListTargetVisibility() when hiding animation
is still in flight.
optional nit: It'd also be nice to only have a single function for visibility.

https://codereview.chromium.org/2169533002/diff/40001/ash/shelf/app_list_butt...
File ash/shelf/app_list_button.h (left):

https://codereview.chromium.org/2169533002/diff/40001/ash/shelf/app_list_butt...
ash/shelf/app_list_button.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2016/07/20 17:00:06, James Cook wrote:
> I don't know why Reitveld shows these as modified and not deleted, but since
> it's all git under the hood it shouldn't matter.

Acknowledged.

https://codereview.chromium.org/2169533002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2169533002/diff/40001/ash/shell.cc#newcode517
ash/shell.cc:517: std::unique_ptr<ShelfItemDelegate> controller(new
AppListShelfItemDelegate);
future: This should probably happen in shelf_view.cc with the button creation.

[James Cook, 2016-07-20 18:35:40]

Thanks for the review!

https://codereview.chromium.org/2169533002/diff/40001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2169533002/diff/40001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:159: void ToggleAppList();
On 2016/07/20 17:33:49, msw wrote:
> optional nit: This only really has two callers (HandleToggleAppList in
> accelerator_controller.cc and AppListShelfItemDelegate::ItemSelected); it
could
> be removed and those places could call into the presenter directly or inline
> similar behavior and then even the presenter method could go away.

I'm going to keep the applist functions as-is. It feels like either WmShell
should expose all 5, or it should just expose the AppListPresenter, and doing
the latter involving inlining the default-display-lookup code in a bunch of
places.

https://codereview.chromium.org/2169533002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2169533002/diff/40001/ash/shell.cc#newcode517
ash/shell.cc:517: std::unique_ptr<ShelfItemDelegate> controller(new
AppListShelfItemDelegate);
On 2016/07/20 17:33:50, msw wrote:
> future: This should probably happen in shelf_view.cc with the button creation.

Acknowledged.

[James Cook, 2016-07-20 18:50:01]

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

[James Cook, 2016-07-20 18:50:02]

TBR dmazzoni for mechanical change touching
chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc

[James Cook, 2016-07-20 19:09:04]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-07-20 19:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2169533002/#ps70001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-20 19:10:11]

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, 2016-07-20 19:19:52]

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

[commit-bot: I haz the power, 2016-07-20 19:19:53]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[James Cook, 2016-07-20 19:23:18]

Description was changed from

==========
mash: Migrate shelf app list button to wm common types

This is needed to port the shelf to mash.

* Move app list show/hide/visible functions to WmShell
* Remove window parameter from ShowAppList/ToggleAppList because non-test
code always shows and hides on the default display
* Migrate ShelfModel ownership to WmShell

BUG=629250
TEST=ash_unittests, chrome unit_tests
==========

to

==========
mash: Migrate shelf app list button to wm common types

This is needed to port the shelf to mash.

* Move app list show/hide/visible functions to WmShell
* Remove window parameter from ShowAppList/ToggleAppList because non-test
code always shows and hides on the default display
* Migrate ShelfModel ownership to WmShell

BUG=629250
TEST=ash_unittests, chrome unit_tests
TBR=dmazzoni@chromium.org
==========

[James Cook, 2016-07-20 19:23:50]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-07-20 19:24:19]

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, 2016-07-20 20:04:16]

Description was changed from

==========
mash: Migrate shelf app list button to wm common types

This is needed to port the shelf to mash.

* Move app list show/hide/visible functions to WmShell
* Remove window parameter from ShowAppList/ToggleAppList because non-test
code always shows and hides on the default display
* Migrate ShelfModel ownership to WmShell

BUG=629250
TEST=ash_unittests, chrome unit_tests
TBR=dmazzoni@chromium.org
==========

to

==========
mash: Migrate shelf app list button to wm common types

This is needed to port the shelf to mash.

* Move app list show/hide/visible functions to WmShell
* Remove window parameter from ShowAppList/ToggleAppList because non-test
code always shows and hides on the default display
* Migrate ShelfModel ownership to WmShell

BUG=629250
TEST=ash_unittests, chrome unit_tests
TBR=dmazzoni@chromium.org
==========

[commit-bot: I haz the power, 2016-07-20 20:04:18]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-07-20 20:06:15]

Description was changed from

==========
mash: Migrate shelf app list button to wm common types

This is needed to port the shelf to mash.

* Move app list show/hide/visible functions to WmShell
* Remove window parameter from ShowAppList/ToggleAppList because non-test
code always shows and hides on the default display
* Migrate ShelfModel ownership to WmShell

BUG=629250
TEST=ash_unittests, chrome unit_tests
TBR=dmazzoni@chromium.org
==========

to

==========
mash: Migrate shelf app list button to wm common types

This is needed to port the shelf to mash.

* Move app list show/hide/visible functions to WmShell
* Remove window parameter from ShowAppList/ToggleAppList because non-test
code always shows and hides on the default display
* Migrate ShelfModel ownership to WmShell

BUG=629250
TEST=ash_unittests, chrome unit_tests
TBR=dmazzoni@chromium.org

Committed: https://crrev.com/abe9e24cab6f3245c8583d773644d3b62d84d71a
Cr-Commit-Position: refs/heads/master@{#406646}
==========

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

Patchset 5 (id:??) landed as
https://crrev.com/abe9e24cab6f3245c8583d773644d3b62d84d71a
Cr-Commit-Position: refs/heads/master@{#406646}