Use mojo app list interfaces for mash and classic ash.

Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.
Move delegate factory ownership to the presenter impl object.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add TestAppListPresenter to track ash->chrome calls for tests.
Add TestAppListViewPresenterImpl for tests of impl/delegate behavior.
Add TestAppListViewDelegateFactory for ash test and shell users.
Add an ExampleAppListPresenter for ash_shell_with_content.
Add an integration interactive ui test; enable another.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=mfomitchev@chromium.org,xiyuan@chromium.org,derat@chromium.org,tsepez@chromium.org,yzshen@chromium.org

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

[msw, 2016-12-15 18:50:07]

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

[commit-bot: I haz the power, 2016-12-15 18:50:35]

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-12-15 18:58:02]

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

[commit-bot: I haz the power, 2016-12-15 18:58:03]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[msw, 2016-12-16 17:18:01]

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

[commit-bot: I haz the power, 2016-12-16 17:18:58]

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-12-16 18:22:07]

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

[commit-bot: I haz the power, 2016-12-16 18:22:08]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[msw, 2016-12-16 19:12:20]

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

[commit-bot: I haz the power, 2016-12-16 19:13:01]

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-12-16 20:14:29]

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

[commit-bot: I haz the power, 2016-12-16 20:14:30]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[msw, 2016-12-21 15:43:33]

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

[commit-bot: I haz the power, 2016-12-21 15:44:00]

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-12-21 15:49:20]

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

[commit-bot: I haz the power, 2016-12-21 15:49:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-12-21 17:24:45]

Description was changed from

==========
Use mojo app list interfaces for mash and classic ash.

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

to

==========
Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add a TestAppListPresenter to track ash->chrome calls for tests.
Add a TestAppListViewDelegateFactory for tests using the presenter impl.
Add an ExampleAppListPresenter for ash_shell_with_content.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=jamescook@chromium.org,mfomitchev@chromium.org
==========

[msw, 2016-12-21 17:24:46]

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

[msw, 2016-12-21 17:25:18]

Hey James and Mikhail, please take a look; thanks!

[msw, 2016-12-21 17:25:46]

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

[commit-bot: I haz the power, 2016-12-21 17:26:08]

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-12-21 18:49:18]

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

[commit-bot: I haz the power, 2016-12-21 18:49:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-12-21 20:53:03]

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

[commit-bot: I haz the power, 2016-12-21 20:53:35]

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-12-21 22:33:41]

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

[commit-bot: I haz the power, 2016-12-21 22:33:42]

Dry run: This issue passed the CQ dry run.

[mfomitchev, 2016-12-22 03:17:46]

Awesome, thanks for doing this!
Still reviewing, but here are some mid-way comments.

https://codereview.chromium.org/2576913002/diff/230055/ash/accelerators/accel...
File ash/accelerators/accelerator_controller_unittest.cc (left):

https://codereview.chromium.org/2576913002/diff/230055/ash/accelerators/accel...
ash/accelerators/accelerator_controller_unittest.cc:983: // When spoken feedback
is on, the AppList should not toggle.
Do we need to remove this? Should we create a bug/TODO to add this back?

https://codereview.chromium.org/2576913002/diff/230055/ash/shelf/shelf_layout...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2576913002/diff/230055/ash/shelf/shelf_layout...
ash/shelf/shelf_layout_manager_unittest.cc:959: AppListPresenterDelegateFactory
app_list_presenter_delegate_factory(
This repeats in a bunch of places (and not just in this test suite). Can we
perhaps create a helper class in app_list::test::AppListInitializer and add a
method GetAppListPresenter() to it? Then TestAppListViewDelegateFactory and
AppListPresenterDelegateFactory creation could be encapsulated inside the test
helper class.

https://codereview.chromium.org/2576913002/diff/230055/ash/shelf/shelf_layout...
ash/shelf/shelf_layout_manager_unittest.cc:968:
GetShelfLayoutManager()->UpdateVisibilityState();
Why is this necessary? If everything is set up properly, the app list delegate
should call UpdateAutoHideState() on the shelf's layout manager, which should
update shelf's visibility. It seems like this reduces the utility of the test -
if something goes wrong in prod code with visibility updates, we may not catch
it.

https://codereview.chromium.org/2576913002/diff/230055/ash/shell/content/clie...
File ash/shell/content/client/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/2576913002/diff/230055/ash/shell/content/clie...
ash/shell/content/client/shell_browser_main_parts.cc:152:
example_app_list_presenter_ = base::MakeUnique<ExampleAppListPresenter>();
Wait, this is executed in production code. If ExampleAppListPresenter is needed
for production code, it shouldn't be called "ExampleAppListPresenter"...

https://codereview.chromium.org/2576913002/diff/230055/ui/app_list/presenter/...
File ui/app_list/presenter/app_list_presenter_impl.h (right):

https://codereview.chromium.org/2576913002/diff/230055/ui/app_list/presenter/...
ui/app_list/presenter/app_list_presenter_impl.h:37: class
APP_LIST_PRESENTER_EXPORT AppListPresenterImpl
If we are getting rid of AppListPresenter interface, this should be just
AppListPresenter, not AppListPresenterImpl IMO.

https://codereview.chromium.org/2576913002/diff/230055/ui/app_list/presenter/...
File ui/app_list/presenter/test/test_app_list_presenter.h (right):

https://codereview.chromium.org/2576913002/diff/230055/ui/app_list/presenter/...
ui/app_list/presenter/test/test_app_list_presenter.h:16: class
TestAppListPresenter : public app_list::mojom::AppListPresenter {
This and other test classes should probably be in test namespace (like
AppListPresenterImplTestApi). At the very least, we should be consistent.

[msw, 2016-12-22 16:29:27]

Description was changed from

==========
Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add a TestAppListPresenter to track ash->chrome calls for tests.
Add a TestAppListViewDelegateFactory for tests using the presenter impl.
Add an ExampleAppListPresenter for ash_shell_with_content.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=jamescook@chromium.org,mfomitchev@chromium.org
==========

to

==========
Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.
Move delegate factory ownership to the presenter impl object.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add TestAppListPresenter to track ash->chrome calls for tests.
Add TestAppListPresenterImpl for tests that rely on impl/delegate behavior.
Add TestAppListViewDelegateFactory for ash test and shell users.
Add an ExampleAppListPresenter for ash_shell_with_content.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=mfomitchev@chromium.org,xiyuan@chromium.org,derat@chromium.org
==========

[msw, 2016-12-22 16:29:27]

msw@chromium.org changed reviewers:
+ derat@chromium.org, xiyuan@chromium.org
- jamescook@chromium.org

[msw, 2016-12-22 16:31:52]

Comments addressed, please take another look; thanks!
+Xiyuan for ui/app_list/OWNERS review.
+Dan for ash/OWNERS and c/b/ui/ash/OWNERS review.

https://codereview.chromium.org/2576913002/diff/230055/ash/accelerators/accel...
File ash/accelerators/accelerator_controller_unittest.cc (left):

https://codereview.chromium.org/2576913002/diff/230055/ash/accelerators/accel...
ash/accelerators/accelerator_controller_unittest.cc:983: // When spoken feedback
is on, the AppList should not toggle.
On 2016/12/22 03:17:45, mfomitchev wrote:
> Do we need to remove this? Should we create a bug/TODO to add this back?

It's redundant with the block above, especially now that the test is checking
for calls to toggle the app list, rather than the expected visibility. Let me
know if I'm missing something.

https://codereview.chromium.org/2576913002/diff/230055/ash/shelf/shelf_layout...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2576913002/diff/230055/ash/shelf/shelf_layout...
ash/shelf/shelf_layout_manager_unittest.cc:959: AppListPresenterDelegateFactory
app_list_presenter_delegate_factory(
On 2016/12/22 03:17:45, mfomitchev wrote:
> This repeats in a bunch of places (and not just in this test suite). Can we
> perhaps create a helper class in app_list::test::AppListInitializer and add a
> method GetAppListPresenter() to it? Then TestAppListViewDelegateFactory and
> AppListPresenterDelegateFactory creation could be encapsulated inside the test
> helper class.

Yup, unfortunately, some tests depend on the presenter and delegate
implementations' behavior. I constructed a TestAppListPresenterImpl, which led
me to change the ownership of the delegate factory, which seems good anyway.
These all seem like good changes, but they add a little more delta to this CL.

https://codereview.chromium.org/2576913002/diff/230055/ash/shelf/shelf_layout...
ash/shelf/shelf_layout_manager_unittest.cc:968:
GetShelfLayoutManager()->UpdateVisibilityState();
On 2016/12/22 03:17:45, mfomitchev wrote:
> Why is this necessary? If everything is set up properly, the app list delegate
> should call UpdateAutoHideState() on the shelf's layout manager, which should
> update shelf's visibility. It seems like this reduces the utility of the test
-
> if something goes wrong in prod code with visibility updates, we may not catch
> it.

Done (I left in the one originally at line 996 with the comment).

https://codereview.chromium.org/2576913002/diff/230055/ash/shell/content/clie...
File ash/shell/content/client/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/2576913002/diff/230055/ash/shell/content/clie...
ash/shell/content/client/shell_browser_main_parts.cc:152:
example_app_list_presenter_ = base::MakeUnique<ExampleAppListPresenter>();
On 2016/12/22 03:17:45, mfomitchev wrote:
> Wait, this is executed in production code. If ExampleAppListPresenter is
needed
> for production code, it shouldn't be called "ExampleAppListPresenter"...

This is //ash/shell, which is only used for the ash_shell_with_content
test/example target. Note the use of ash/shell/shell_delegate_impl.h here, etc.
This is *not* production Chrome OS (or desktop Chrome Browser) code.

https://codereview.chromium.org/2576913002/diff/230055/ui/app_list/presenter/...
File ui/app_list/presenter/app_list_presenter_impl.h (right):

https://codereview.chromium.org/2576913002/diff/230055/ui/app_list/presenter/...
ui/app_list/presenter/app_list_presenter_impl.h:37: class
APP_LIST_PRESENTER_EXPORT AppListPresenterImpl
On 2016/12/22 03:17:45, mfomitchev wrote:
> If we are getting rid of AppListPresenter interface, this should be just
> AppListPresenter, not AppListPresenterImpl IMO.

I totally agree, I'll do that in a followup! It should also fix the conflict of
app_list::AppListPresenter[Impl] and app_list::mojom::AppListPresenter, or make
the non-mojom class actually inherit the mojo interface.

https://codereview.chromium.org/2576913002/diff/230055/ui/app_list/presenter/...
File ui/app_list/presenter/test/test_app_list_presenter.h (right):

https://codereview.chromium.org/2576913002/diff/230055/ui/app_list/presenter/...
ui/app_list/presenter/test/test_app_list_presenter.h:16: class
TestAppListPresenter : public app_list::mojom::AppListPresenter {
On 2016/12/22 03:17:45, mfomitchev wrote:
> This and other test classes should probably be in test namespace (like
> AppListPresenterImplTestApi). At the very least, we should be consistent.

Done.

[msw, 2016-12-22 16:32:06]

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

[commit-bot: I haz the power, 2016-12-22 16:32: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-12-22 16:36:58]

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

[commit-bot: I haz the power, 2016-12-22 16:36:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-12-22 16:44:21]

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

[commit-bot: I haz the power, 2016-12-22 16:44:36]

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-12-22 17:03:32]

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

[commit-bot: I haz the power, 2016-12-22 17:03:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-12-22 17:32:02]

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

[commit-bot: I haz the power, 2016-12-22 17:32:19]

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-12-22 18:00:03]

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

[commit-bot: I haz the power, 2016-12-22 18:00:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-12-22 18:12:35]

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

[commit-bot: I haz the power, 2016-12-22 18:12:49]

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

[Daniel Erat, 2016-12-22 18:17:58]

generally lgtm as an owner, but i have basically zero app list experience and
will defer to others on that part of the review.

https://codereview.chromium.org/2576913002/diff/290001/ash/shell/example_app_...
File ash/shell/example_app_list_presenter.cc (right):

https://codereview.chromium.org/2576913002/diff/290001/ash/shell/example_app_...
ash/shell/example_app_list_presenter.cc:19: app_list::AppListViewDelegate*
GetDelegate() override {
ui/app_list/presenter/app_list_view_delegate_factory.h doesn't have any
comments, but should this be named CreateDelegate() instead? i'd expect a "get"
method to return some existing object. does ownership get passed?

https://codereview.chromium.org/2576913002/diff/290001/ash/shell/example_app_...
File ash/shell/example_app_list_presenter.h (right):

https://codereview.chromium.org/2576913002/diff/290001/ash/shell/example_app_...
ash/shell/example_app_list_presenter.h:18: // An example implementation of
AppListPresenter used for ash_shell.
how will this example class be used? is it a stub for tests? should it live
somewhere else?

https://codereview.chromium.org/2576913002/diff/310001/ash/shell/example_app_...
File ash/shell/example_app_list_presenter.cc (right):

https://codereview.chromium.org/2576913002/diff/310001/ash/shell/example_app_...
ash/shell/example_app_list_presenter.cc:41:
ExampleAppListPresenter::~ExampleAppListPresenter() {}
is "= default;" preferred in cases like these now? i guess that "{}" is fewer
characters...

https://codereview.chromium.org/2576913002/diff/330001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/2576913002/diff/330001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:103:
app_list_presenter_service_ = base::MakeUnique<AppListPresenterService>();
is this dependent on other classes being initialized first? most of the other
member objects seem to be initialized in the c'tor, so if there's a reason this
isn't, it'd be good to document it in a comment.

[Daniel Erat, 2016-12-22 18:18:27]

(note that my comments ended up being spread across multiple patch sets)

[mfomitchev, 2016-12-22 18:26:56]

LGTM with a few nits - I leave it up to you if you want to address any of them
(and in which CLs).

[mfomitchev, 2016-12-22 18:27:11]

https://codereview.chromium.org/2576913002/diff/230055/ash/accelerators/accel...
File ash/accelerators/accelerator_controller_unittest.cc (left):

https://codereview.chromium.org/2576913002/diff/230055/ash/accelerators/accel...
ash/accelerators/accelerator_controller_unittest.cc:983: // When spoken feedback
is on, the AppList should not toggle.
On 2016/12/22 16:31:52, msw wrote:
> On 2016/12/22 03:17:45, mfomitchev wrote:
> > Do we need to remove this? Should we create a bug/TODO to add this back?
> 
> It's redundant with the block above, especially now that the test is checking
> for calls to toggle the app list, rather than the expected visibility. Let me
> know if I'm missing something.

Ah, missed that the block above was doing virtually the same thing.

https://codereview.chromium.org/2576913002/diff/230055/ash/shell/content/clie...
File ash/shell/content/client/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/2576913002/diff/230055/ash/shell/content/clie...
ash/shell/content/client/shell_browser_main_parts.cc:152:
example_app_list_presenter_ = base::MakeUnique<ExampleAppListPresenter>();
On 2016/12/22 16:31:52, msw wrote:
> On 2016/12/22 03:17:45, mfomitchev wrote:
> > Wait, this is executed in production code. If ExampleAppListPresenter is
> needed
> > for production code, it shouldn't be called "ExampleAppListPresenter"...
> 
> This is //ash/shell, which is only used for the ash_shell_with_content
> test/example target. Note the use of ash/shell/shell_delegate_impl.h here,
etc.
> This is *not* production Chrome OS (or desktop Chrome Browser) code.

Ah, ok, thanks for the clarification

https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:1477: app_list_button->OnAppListShown();
I am not sure if we actually have any tests that verify that OnAppListShown() is
called on the button when app list is shown? I couldn't immediately find any.
This one does at least indirectly, and we are removing this part..

https://codereview.chromium.org/2576913002/diff/310001/ash/test/test_app_list...
File ash/test/test_app_list_presenter_impl.h (right):

https://codereview.chromium.org/2576913002/diff/310001/ash/test/test_app_list...
ash/test/test_app_list_presenter_impl.h:17: class TestAppListPresenterImpl :
public app_list::AppListPresenterImpl {
Nit: Maybe call this AppListPresenterWrapper or AppListPresenterInitializer?
This is not really a "test" implementation like
app_list::test::TestAppListPresenter - it's a convenience wrapper around a real
implementation.

https://codereview.chromium.org/2576913002/diff/310001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/2576913002/diff/310001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:103:
app_list_presenter_service_ = base::MakeUnique<AppListPresenterService>();
It's pretty subtle that this is where all the magic happens. Maybe add a comment
clarifying this is establishing all the mojo connections. I guess there is a
good reason we need to do it in Init rather than the way it was done before?
Might be good to clarify that as well if it is relevant.

[commit-bot: I haz the power, 2016-12-22 19:18:30]

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

[commit-bot: I haz the power, 2016-12-22 19:18:31]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-12-22 19:39:06]

ui/app_list lgtm

[msw, 2016-12-23 03:57:10]

Description was changed from

==========
Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.
Move delegate factory ownership to the presenter impl object.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add TestAppListPresenter to track ash->chrome calls for tests.
Add TestAppListPresenterImpl for tests that rely on impl/delegate behavior.
Add TestAppListViewDelegateFactory for ash test and shell users.
Add an ExampleAppListPresenter for ash_shell_with_content.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=mfomitchev@chromium.org,xiyuan@chromium.org,derat@chromium.org
==========

to

==========
Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.
Move delegate factory ownership to the presenter impl object.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add TestAppListPresenter to track ash->chrome calls for tests.
Add TestAppListViewPresenterImpl for tests of impl/delegate behavior.
Add TestAppListViewDelegateFactory for ash test and shell users.
Add an ExampleAppListPresenter for ash_shell_with_content.
Add an integration interactive ui test; enable another.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=mfomitchev@chromium.org,xiyuan@chromium.org,derat@chromium.org,tsepez@chrom...
==========

[msw, 2016-12-23 03:57:10]

msw@chromium.org changed reviewers:
+ tsepez@chromium.org, yzshen@chromium.org

[msw, 2016-12-23 03:59:34]

Comments addressed; take another look if you're interested.
+Tom for app_list_presenter.mojom review.
+Yuzhu for ash/accelerators/DEPS on mojo review.
Please take a look; thanks!

https://codereview.chromium.org/2576913002/diff/290001/ash/shell/example_app_...
File ash/shell/example_app_list_presenter.cc (right):

https://codereview.chromium.org/2576913002/diff/290001/ash/shell/example_app_...
ash/shell/example_app_list_presenter.cc:19: app_list::AppListViewDelegate*
GetDelegate() override {
On 2016/12/22 18:17:57, Daniel Erat wrote:
> ui/app_list/presenter/app_list_view_delegate_factory.h doesn't have any
> comments, but should this be named CreateDelegate() instead? i'd expect a
"get"
> method to return some existing object. does ownership get passed?

This indeed passes a pointer to an existing (or lazily constructed) object owned
elsewhere (and I fixed ExampleAppListViewDelegateFactory to do the same). I
added a comment to the declaration. FYI, I tried changing the ownership model
here, but it gets hairy around AppListServiceImpl, etc. I hope this can be
simplified in the future (avoid the factory altogether or similar).

https://codereview.chromium.org/2576913002/diff/290001/ash/shell/example_app_...
File ash/shell/example_app_list_presenter.h (right):

https://codereview.chromium.org/2576913002/diff/290001/ash/shell/example_app_...
ash/shell/example_app_list_presenter.h:18: // An example implementation of
AppListPresenter used for ash_shell.
On 2016/12/22 18:17:57, Daniel Erat wrote:
> how will this example class be used? is it a stub for tests? should it live
> somewhere else?

Hopefully the comment is clear enough, it's used for ash_shell[_with_content],
which is the example ash shell environment. This is somewhat different than the
implementation used for testing, and certainly different than the implementation
used for Chrome OS. I hope to simplify further down the line; the app_list code
is a mess :(

https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:1477: app_list_button->OnAppListShown();
On 2016/12/22 18:27:11, mfomitchev wrote:
> I am not sure if we actually have any tests that verify that OnAppListShown()
is
> called on the button when app list is shown? I couldn't immediately find any.
> This one does at least indirectly, and we are removing this part..

I added an integration interactive ui test to check that the app list is toggled
by pressing the app list button, and I re-enabled
AppListServiceInteractiveTest.ShowAndDismiss (not sure this ever ran as an
interactive_ui_test on Chrome OS, but it's passing for me locally, hopefully the
bots like it...). Some unit tests ensure that pressing the button calls the
presenter to toggle the app list, and other tests ensure that instructing the
presenter to show changes the [target] visibility, etc.

https://codereview.chromium.org/2576913002/diff/310001/ash/shell/example_app_...
File ash/shell/example_app_list_presenter.cc (right):

https://codereview.chromium.org/2576913002/diff/310001/ash/shell/example_app_...
ash/shell/example_app_list_presenter.cc:41:
ExampleAppListPresenter::~ExampleAppListPresenter() {}
On 2016/12/22 18:17:57, Daniel Erat wrote:
> is "= default;" preferred in cases like these now? i guess that "{}" is fewer
> characters...

The style guide doesn't state a preference, and I don't have a preference nor
see one in the surrounding code, I left this as-is, but I can change this (and
do an audit of new/touched ctors/dtors?) if you want.

https://codereview.chromium.org/2576913002/diff/310001/ash/test/test_app_list...
File ash/test/test_app_list_presenter_impl.h (right):

https://codereview.chromium.org/2576913002/diff/310001/ash/test/test_app_list...
ash/test/test_app_list_presenter_impl.h:17: class TestAppListPresenterImpl :
public app_list::AppListPresenterImpl {
On 2016/12/22 18:27:11, mfomitchev wrote:
> Nit: Maybe call this AppListPresenterWrapper or AppListPresenterInitializer?
> This is not really a "test" implementation like
> app_list::test::TestAppListPresenter - it's a convenience wrapper around a
real
> implementation.

Technically this is an AppListPresenterImpl that uses a
TestAppListViewDelegateFactory under the hood; in that sense it's a presenter
subclass used for testing purposes. But you're right, the name is a bit
off-point; I changed it to TestAppListViewPresenterImpl, "a presenter impl of
the test app list view...".

https://codereview.chromium.org/2576913002/diff/310001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/2576913002/diff/310001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:103:
app_list_presenter_service_ = base::MakeUnique<AppListPresenterService>();
On 2016/12/22 18:27:11, mfomitchev wrote:
> It's pretty subtle that this is where all the magic happens. Maybe add a
comment
> clarifying this is establishing all the mojo connections. I guess there is a
> good reason we need to do it in Init rather than the way it was done before?
> Might be good to clarify that as well if it is relevant.

I added a comment explaining why this is done here instead of the ctor, and
added a bit explaining that this initializes the mojo connections.

https://codereview.chromium.org/2576913002/diff/330001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/2576913002/diff/330001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:103:
app_list_presenter_service_ = base::MakeUnique<AppListPresenterService>();
On 2016/12/22 18:17:57, Daniel Erat wrote:
> is this dependent on other classes being initialized first? most of the other
> member objects seem to be initialized in the c'tor, so if there's a reason
this
> isn't, it'd be good to document it in a comment.

Yes, good call, I added a comment.

[msw, 2016-12-23 03:59:45]

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

[commit-bot: I haz the power, 2016-12-23 03:59:57]

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

[Daniel Erat, 2016-12-23 04:33:36]

thanks for the replies. lgtm as-is

[commit-bot: I haz the power, 2016-12-23 04:52:59]

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

[commit-bot: I haz the power, 2016-12-23 04:53:00]

Dry run: This issue passed the CQ dry run.

[mfomitchev, 2016-12-23 16:49:27]

https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:1477: app_list_button->OnAppListShown();
On 2016/12/23 03:59:34, msw wrote:
> On 2016/12/22 18:27:11, mfomitchev wrote:
> > I am not sure if we actually have any tests that verify that
OnAppListShown()
> is
> > called on the button when app list is shown? I couldn't immediately find
any.
> > This one does at least indirectly, and we are removing this part..
> 
> I added an integration interactive ui test to check that the app list is
toggled
> by pressing the app list button, and I re-enabled
> AppListServiceInteractiveTest.ShowAndDismiss (not sure this ever ran as an
> interactive_ui_test on Chrome OS, but it's passing for me locally, hopefully
the
> bots like it...). Some unit tests ensure that pressing the button calls the
> presenter to toggle the app list, and other tests ensure that instructing the
> presenter to show changes the [target] visibility, etc.

Sorry, I don't think this actually tests that OnAppListShown() is called on the
AppListButton when the app list is shown? Or am I missing something?

[msw, 2016-12-23 17:34:15]

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

[commit-bot: I haz the power, 2016-12-23 17:34:25]

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

[msw, 2016-12-23 17:34:27]

https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:1477: app_list_button->OnAppListShown();
On 2016/12/23 16:49:27, mfomitchev wrote:
> On 2016/12/23 03:59:34, msw wrote:
> > On 2016/12/22 18:27:11, mfomitchev wrote:
> > > I am not sure if we actually have any tests that verify that
> OnAppListShown()
> > is
> > > called on the button when app list is shown? I couldn't immediately find
> any.
> > > This one does at least indirectly, and we are removing this part..
> > 
> > I added an integration interactive ui test to check that the app list is
> toggled
> > by pressing the app list button, and I re-enabled
> > AppListServiceInteractiveTest.ShowAndDismiss (not sure this ever ran as an
> > interactive_ui_test on Chrome OS, but it's passing for me locally, hopefully
> the
> > bots like it...). Some unit tests ensure that pressing the button calls the
> > presenter to toggle the app list, and other tests ensure that instructing
the
> > presenter to show changes the [target] visibility, etc.
> 
> Sorry, I don't think this actually tests that OnAppListShown() is called on
the
> AppListButton when the app list is shown? Or am I missing something?

Done. (in the new AppListTest.PressAppListButtonToShowAndDismiss)

[mfomitchev, 2016-12-23 18:33:24]

On 2016/12/23 17:34:27, msw wrote:
>
https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
> File ash/shelf/shelf_view_unittest.cc (right):
> 
>
https://codereview.chromium.org/2576913002/diff/310001/ash/shelf/shelf_view_u...
> ash/shelf/shelf_view_unittest.cc:1477: app_list_button->OnAppListShown();
> On 2016/12/23 16:49:27, mfomitchev wrote:
> > On 2016/12/23 03:59:34, msw wrote:
> > > On 2016/12/22 18:27:11, mfomitchev wrote:
> > > > I am not sure if we actually have any tests that verify that
> > OnAppListShown()
> > > is
> > > > called on the button when app list is shown? I couldn't immediately find
> > any.
> > > > This one does at least indirectly, and we are removing this part..
> > > 
> > > I added an integration interactive ui test to check that the app list is
> > toggled
> > > by pressing the app list button, and I re-enabled
> > > AppListServiceInteractiveTest.ShowAndDismiss (not sure this ever ran as an
> > > interactive_ui_test on Chrome OS, but it's passing for me locally,
hopefully
> > the
> > > bots like it...). Some unit tests ensure that pressing the button calls
the
> > > presenter to toggle the app list, and other tests ensure that instructing
> the
> > > presenter to show changes the [target] visibility, etc.
> > 
> > Sorry, I don't think this actually tests that OnAppListShown() is called on
> the
> > AppListButton when the app list is shown? Or am I missing something?
> 
> Done. (in the new AppListTest.PressAppListButtonToShowAndDismiss)

Thanks! Still LGTM.

[commit-bot: I haz the power, 2016-12-23 18:50:12]

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

[commit-bot: I haz the power, 2016-12-23 18:50:14]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-01-03 19:05:13]

mojom LGTM

[yzshen1, 2017-01-03 22:19:04]

On 2017/01/03 19:05:13, Tom Sepez wrote:
> mojom LGTM

ash/accelerators/DEPS LGTM

[yzshen1, 2017-01-03 22:19:09]

On 2017/01/03 19:05:13, Tom Sepez wrote:
> mojom LGTM

ash/accelerators/DEPS LGTM

[msw, 2017-01-04 01:34:02]

The CQ bit was checked by msw@chromium.org

[msw, 2017-01-04 01:34:03]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2576913002/#ps370001
(title: "Check AppListButton::is_showing_app_list for Shown/Dismissed
notifications")

[commit-bot: I haz the power, 2017-01-04 01:34: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-04 02:54:02]

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

[commit-bot: I haz the power, 2017-01-04 02:54:04]

Failed to apply patch for ash/common/wm_shell.cc:
While running git apply --index -p1;
  error: patch failed: ash/common/wm_shell.cc:49
  error: ash/common/wm_shell.cc: patch does not apply

Patch:       ash/common/wm_shell.cc
Index: ash/common/wm_shell.cc
diff --git a/ash/common/wm_shell.cc b/ash/common/wm_shell.cc
index
0bd31c79726c93aee3144a0d5907229ba092c74d..42d6250629db39051e255ee6a8f4efd912c6d947
100644
--- a/ash/common/wm_shell.cc
+++ b/ash/common/wm_shell.cc
@@ -49,7 +49,6 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "ui/app_list/presenter/app_list.h"
-#include "ui/app_list/presenter/app_list_presenter.h"
 #include "ui/display/display.h"
 #include "ui/views/focus/focus_manager_factory.h"
 
@@ -330,28 +329,25 @@ void WmShell::OnModalWindowRemoved(WmWindow* removed) {
 
 void WmShell::ShowAppList() {
   // Show the app list on the default display for new windows.
-  int64_t display_id =
-      GetRootWindowForNewWindows()->GetDisplayNearestWindow().id();
-  delegate_->GetAppListPresenter()->Show(display_id);
+ 
app_list_->Show(GetRootWindowForNewWindows()->GetDisplayNearestWindow().id());
 }
 
 void WmShell::DismissAppList() {
-  delegate_->GetAppListPresenter()->Dismiss();
+  app_list_->Dismiss();
 }
 
 void WmShell::ToggleAppList() {
-  // Show the app list on the default display for new windows.
-  int64_t display_id =
-      GetRootWindowForNewWindows()->GetDisplayNearestWindow().id();
-  delegate_->GetAppListPresenter()->ToggleAppList(display_id);
+  // Toggle the app list on the default display for new windows.
+  app_list_->ToggleAppList(
+      GetRootWindowForNewWindows()->GetDisplayNearestWindow().id());
 }
 
 bool WmShell::IsApplistVisible() const {
-  return delegate_->GetAppListPresenter()->IsVisible();
+  return app_list_->IsVisible();
 }
 
 bool WmShell::GetAppListTargetVisibility() const {
-  return delegate_->GetAppListPresenter()->GetTargetVisibility();
+  return app_list_->GetTargetVisibility();
 }
 
 void WmShell::SetKeyboardUI(std::unique_ptr<KeyboardUI> keyboard_ui) {

[msw, 2017-01-04 21:34:56]

The CQ bit was checked by msw@chromium.org

[msw, 2017-01-04 21:34:57]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
xiyuan@chromium.org, derat@chromium.org, mfomitchev@chromium.org,
yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2576913002/#ps390001
(title: "Sync and rebase")

[commit-bot: I haz the power, 2017-01-04 21:35:21]

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-04 22:50:09]

CQ is committing da patch.

Bot data: {"patchset_id": 390001, "attempt_start_ts": 1483565696476600,
"parent_rev": "ee70e2d21ad43c880745a670b5053e485798e74f", "commit_rev":
"b7a91528f838b77f137f07aae530618f902e62bd"}

[commit-bot: I haz the power, 2017-01-04 22:50:41]

Description was changed from

==========
Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.
Move delegate factory ownership to the presenter impl object.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add TestAppListPresenter to track ash->chrome calls for tests.
Add TestAppListViewPresenterImpl for tests of impl/delegate behavior.
Add TestAppListViewDelegateFactory for ash test and shell users.
Add an ExampleAppListPresenter for ash_shell_with_content.
Add an integration interactive ui test; enable another.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=mfomitchev@chromium.org,xiyuan@chromium.org,derat@chromium.org,tsepez@chrom...
==========

to

==========
Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.
Move delegate factory ownership to the presenter impl object.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add TestAppListPresenter to track ash->chrome calls for tests.
Add TestAppListViewPresenterImpl for tests of impl/delegate behavior.
Add TestAppListViewDelegateFactory for ash test and shell users.
Add an ExampleAppListPresenter for ash_shell_with_content.
Add an integration interactive ui test; enable another.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=mfomitchev@chromium.org,xiyuan@chromium.org,derat@chromium.org,tsepez@chrom...

Review-Url: https://codereview.chromium.org/2576913002
==========

[commit-bot: I haz the power, 2017-01-04 22:50:43]

Committed patchset #21 (id:390001)

[commit-bot: I haz the power, 2017-01-04 22:54:24]

Description was changed from

==========
Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.
Move delegate factory ownership to the presenter impl object.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add TestAppListPresenter to track ash->chrome calls for tests.
Add TestAppListViewPresenterImpl for tests of impl/delegate behavior.
Add TestAppListViewDelegateFactory for ash test and shell users.
Add an ExampleAppListPresenter for ash_shell_with_content.
Add an integration interactive ui test; enable another.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=mfomitchev@chromium.org,xiyuan@chromium.org,derat@chromium.org,tsepez@chrom...

Review-Url: https://codereview.chromium.org/2576913002
==========

to

==========
Use mojo app list interfaces for mash and classic ash.

Consolidate WmShell, etc. to always use mojo (app list -> presenter).
Remove ShellDelegate::GetAppListPresenter and classic/shell members.
Remove AppListPresenterMus, combine AppListPresenter and *Impl.
Move delegate factory ownership to the presenter impl object.

Add app_list::mojom::AppList functions for reporting visibility changes.
Add helpers to get ash's cached visibility state, reported by chrome.

Add TestAppListPresenter to track ash->chrome calls for tests.
Add TestAppListViewPresenterImpl for tests of impl/delegate behavior.
Add TestAppListViewDelegateFactory for ash test and shell users.
Add an ExampleAppListPresenter for ash_shell_with_content.
Add an integration interactive ui test; enable another.
Update tests; cleanup ifdefs (ash is Chrome OS only now).

TODO: App list toggling in --mash causes ink drop DCHECKs (unchanged by this CL)
TODO: Consolidate and simplify more app list code...

BUG=557408
TEST=No Chrome OS app list behavior changes.
R=mfomitchev@chromium.org,xiyuan@chromium.org,derat@chromium.org,tsepez@chrom...

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

[commit-bot: I haz the power, 2017-01-04 22:54:25]

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