AppListController refactoring part 1: AppListShower implementation.

AppListController refactoring part 1: AppListShower implementation.

This is the first part of ash::AppListController refactoring preparing it
for the Mus+ash use.
The goal of the refactoring is to break up ash::AppListController into two
parts: AppListShower (implemented in ui/app_list/) and AppListDelegate
(implemented in ash/wm). In Mus+ash world, app list will live in the chrome
process, and mash/SysUI will interact with it via a thin interface.
More details in the design doc:
https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca6Q/edit?ts=56d0e9f0#heading=h.b2znwtuxd2x2

This CL adds interfaces for AppListShower and AppListDelegate, as well as the
default implementation of the shower: AppListShowerImpl and unit tests.

BUG=557408
Committed: https://crrev.com/216ff0ba63afa3ba3ec066d12fac41df95fdc7e0
Cr-Commit-Position: refs/heads/master@{#385526}

[mfomitchev, 2016-03-24 18:17:47]

PTAL

[mfomitchev, 2016-03-24 18:18:17]

mfomitchev@chromium.org changed reviewers:
+ mgiuca@chromium.org, sky@chromium.org, xiyuan@chromium.org

[mfomitchev, 2016-03-24 18:18:18]

PTAL

[mfomitchev, 2016-03-24 18:18:49]

Full CL for reference: https://codereview.chromium.org/1770993002/

[sky, 2016-03-24 19:03:42]

Xiyuan is the expert here. I'm hoping he can remove this.

On Thu, Mar 24, 2016 at 11:18 AM,  <mfomitchev@chromium.org> wrote:
> Full CL for reference: https://codereview.chromium.org/1770993002/
>
> https://codereview.chromium.org/1830293002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xiyuan, 2016-03-24 20:31:12]

Mostly good.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_delegate.h (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate.h:19: class AppListView;
nit: insert an empty line since we have an empty line at the namespace end.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate.h:21: class APP_LIST_EXPORT
AppListShowerDelegate {
nit: document the class?

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_delegate_factory.h (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate_factory.h:10: #include
"ui/app_list/app_list_shower_delegate.h"
nit: Since we forward declared AppListShower and AppListShowerDelegate, we don't
need the two headers. Or if we need to headers, then remove the forward decl.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate_factory.h:13: class AppListShower;
nit: insert an empty line

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate_factory.h:21: app_list::AppListShower*
shower) = 0;
nit: "app_list::" is redundant

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_impl.cc (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.cc:142: shower_delegate_.reset();
We still need to worry about shutdown owner because user can shutdown shell with
the app list open. Resetting it here would not help with that case.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_impl.h (left):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.h:148:
DISALLOW_COPY_AND_ASSIGN(AppListController);
Why removing this?

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_impl.h (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.h:54: void
set_view_delegate(AppListViewDelegate* view_delegate) {
Can we pass this in as an arg to ctor? Using a setter is more error prone. If we
want to use it, we need to at least DCHECK(view_) to make sure it is called
before |view_| is constructed.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.h:100: AppListShowerDelegateFactory* factory_;
nit: document ownership and "AppListShowerDelegateFactory* const" if it is not
going to changed.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.h:106: AppListViewDelegate* view_delegate_;
nit: prefer to use member initializer instead of in ctor.
i.e. 
  AppListViewDelegate* view_delegate_ = nullptr;


The C++11 style is allowed in:
https://chromium-cpp.appspot.com/ "Non-Static Class Member Initializers"

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_impl_unittest.cc (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl_unittest.cc:33: bool init_called() { return
init_called_; }
nit: const method for all getters.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl_unittest.cc:113: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/test/app_li...
File ui/app_list/test/app_list_shower_impl_test_api.h (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/test/app_li...
ui/app_list/test/app_list_shower_impl_test_api.h:16: // Accesses private data
from an AppListController for testing.
nit: insert an empty line before.

[mfomitchev, 2016-03-29 17:33:32]

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_delegate.h (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate.h:19: class AppListView;
On 2016/03/24 20:31:11, xiyuan wrote:
> nit: insert an empty line since we have an empty line at the namespace end.

Done.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate.h:21: class APP_LIST_EXPORT
AppListShowerDelegate {
On 2016/03/24 20:31:11, xiyuan wrote:
> nit: document the class?

Done.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_delegate_factory.h (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate_factory.h:10: #include
"ui/app_list/app_list_shower_delegate.h"
On 2016/03/24 20:31:11, xiyuan wrote:
> nit: Since we forward declared AppListShower and AppListShowerDelegate, we
don't
> need the two headers. Or if we need to headers, then remove the forward decl.

Done.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate_factory.h:13: class AppListShower;
On 2016/03/24 20:31:11, xiyuan wrote:
> nit: insert an empty line

Done.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_delegate_factory.h:21: app_list::AppListShower*
shower) = 0;
On 2016/03/24 20:31:11, xiyuan wrote:
> nit: "app_list::" is redundant

Done.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_impl.cc (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.cc:142: shower_delegate_.reset();
On 2016/03/24 20:31:11, xiyuan wrote:
> We still need to worry about shutdown owner because user can shutdown shell
with
> the app list open. Resetting it here would not help with that case.

When we have a shutdown with app list window open, we get OnWidgetDestroying()
callback, which calls ResetView(), and everything works fine in that case. I
could clarify the comment, but ATM I think I'll just get rid of it - it's
getting a bit too complicated to explain, and I am not sure we need it here -
the pattern isn't really so uncommon that we need to rationalize it. Let me know
if you think we should leave the comment.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_impl.h (left):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.h:148:
DISALLOW_COPY_AND_ASSIGN(AppListController);
On 2016/03/24 20:31:12, xiyuan wrote:
> Why removing this?

Fixed

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_impl.h (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.h:54: void
set_view_delegate(AppListViewDelegate* view_delegate) {
On 2016/03/24 20:31:12, xiyuan wrote:
> Can we pass this in as an arg to ctor? Using a setter is more error prone. If
we
> want to use it, we need to at least DCHECK(view_) to make sure it is called
> before |view_| is constructed.

Consider the AppListServiceAsh in the complete implementation
(crrev.com/1770993002). Unfortunately we have a circular dependency:
app_list_shower_ needs the view delegate, the view delegate needs the
controller_delegate_, and the controller_delegate_needs app_list_shower_. We
break this dependency by adding this setter. We do DCHECK for view_delegate_ in
AppListShowerImpl::Show, which is the only way to create the view.

The code could probably be refactored to eliminate the circular dependency, but
I think that means changing AppListControllerDelegate interface. That would
affect a bunch of non-ash applist classes, which would be a waste of considering
the work planned for M51 that mgiuca@ talked about.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.h:100: AppListShowerDelegateFactory* factory_;
On 2016/03/24 20:31:12, xiyuan wrote:
> nit: document ownership and "AppListShowerDelegateFactory* const" if it is not
> going to changed.

Done.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.h:106: AppListViewDelegate* view_delegate_;
On 2016/03/24 20:31:12, xiyuan wrote:
> nit: prefer to use member initializer instead of in ctor.
> i.e. 
>   AppListViewDelegate* view_delegate_ = nullptr;
> 
> 
> The C++11 style is allowed in:
> https://chromium-cpp.appspot.com/ "Non-Static Class Member Initializers"

Cool, done. Thanks for the link! I updated the other fields as well.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_impl_unittest.cc (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl_unittest.cc:33: bool init_called() { return
init_called_; }
On 2016/03/24 20:31:12, xiyuan wrote:
> nit: const method for all getters.

Done.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl_unittest.cc:113: };
On 2016/03/24 20:31:12, xiyuan wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/test/app_li...
File ui/app_list/test/app_list_shower_impl_test_api.h (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/test/app_li...
ui/app_list/test/app_list_shower_impl_test_api.h:16: // Accesses private data
from an AppListController for testing.
On 2016/03/24 20:31:12, xiyuan wrote:
> nit: insert an empty line before.

Done.

[xiyuan, 2016-03-29 17:44:34]

lgtm

Cool.

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
File ui/app_list/app_list_shower_impl.cc (right):

https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
ui/app_list/app_list_shower_impl.cc:142: shower_delegate_.reset();
On 2016/03/29 17:33:31, mfomitchev wrote:
> On 2016/03/24 20:31:11, xiyuan wrote:
> > We still need to worry about shutdown owner because user can shutdown shell
> with
> > the app list open. Resetting it here would not help with that case.
> 
> When we have a shutdown with app list window open, we get OnWidgetDestroying()
> callback, which calls ResetView(), and everything works fine in that case. I
> could clarify the comment, but ATM I think I'll just get rid of it - it's
> getting a bit too complicated to explain, and I am not sure we need it here -
> the pattern isn't really so uncommon that we need to rationalize it. Let me
know
> if you think we should leave the comment.

I am fine as long as the shutdown case is tested.

[mfomitchev, 2016-03-29 18:32:45]

On 2016/03/29 17:44:34, xiyuan wrote:
> lgtm
> 
> Cool.
> 
>
https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
> File ui/app_list/app_list_shower_impl.cc (right):
> 
>
https://codereview.chromium.org/1830293002/diff/20001/ui/app_list/app_list_sh...
> ui/app_list/app_list_shower_impl.cc:142: shower_delegate_.reset();
> On 2016/03/29 17:33:31, mfomitchev wrote:
> > On 2016/03/24 20:31:11, xiyuan wrote:
> > > We still need to worry about shutdown owner because user can shutdown
shell
> > with
> > > the app list open. Resetting it here would not help with that case.
> > 
> > When we have a shutdown with app list window open, we get
OnWidgetDestroying()
> > callback, which calls ResetView(), and everything works fine in that case. I
> > could clarify the comment, but ATM I think I'll just get rid of it - it's
> > getting a bit too complicated to explain, and I am not sure we need it here
-
> > the pattern isn't really so uncommon that we need to rationalize it. Let me
> know
> > if you think we should leave the comment.
> 
> I am fine as long as the shutdown case is tested.

It was already tested in ash unit tests. I tweaked AppListShowerImpl unit test a
bit to cover it as well.

[mfomitchev, 2016-03-29 18:32:54]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-03-29 18:32:55]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1830293002/#ps140001
(title: "Tweaking unit test TearDown() a bit for better shutdown test.")

[commit-bot: I haz the power, 2016-03-29 18:33:11]

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

[commit-bot: I haz the power, 2016-03-29 18:43:22]

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

[commit-bot: I haz the power, 2016-03-29 18:43:23]

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

[mfomitchev, 2016-03-30 18:09:15]

Description was changed from

==========
AppListController refactoring part 1: AppListShower implementation.

This is the first part of ash::AppListController refactoring preparing it
for the Mus+ash use.
The goal of the refactoring is to break up ash::AppListController into two
parts: AppListShower (implemented in ui/app_list/) and AppListDelegate
(implemented in ash/wm). In Mus+ash world, app list will live in the chrome
process, and mash/SysUI will interact with it via a thin interface.
More details in the design doc:
https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca...

This CL adds interfaces for AppListShower and AppListDelegate, as well as the
default implementation of the shower: AppListShowerImpl and unit tests.

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

to

==========
AppListController refactoring part 1: AppListShower implementation.

This is the first part of ash::AppListController refactoring preparing it
for the Mus+ash use.
The goal of the refactoring is to break up ash::AppListController into two
parts: AppListShower (implemented in ui/app_list/) and AppListDelegate
(implemented in ash/wm). In Mus+ash world, app list will live in the chrome
process, and mash/SysUI will interact with it via a thin interface.
More details in the design doc:
https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca...

This CL adds interfaces for AppListShower and AppListDelegate, as well as the
default implementation of the shower: AppListShowerImpl and unit tests.

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

[mfomitchev, 2016-03-30 18:09:15]

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

[mfomitchev, 2016-03-30 18:09:55]

+sadrul for added //ui/keyboard dependency in DEPS

[sadrul, 2016-03-30 18:59:55]

On 2016/03/30 18:09:55, mfomitchev wrote:
> +sadrul for added //ui/keyboard dependency in DEPS

Can we actually just remove this dependency (and the relevant code) instead?

[mfomitchev, 2016-03-30 21:10:55]

On 2016/03/30 18:59:55, sadrul wrote:
> On 2016/03/30 18:09:55, mfomitchev wrote:
> > +sadrul for added //ui/keyboard dependency in DEPS
> 
> Can we actually just remove this dependency (and the relevant code) instead?

Done.

[mfomitchev, 2016-03-30 21:11:05]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-03-30 21:11:06]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1830293002/#ps160001
(title: "Getting rid of the keyboard controller code - will put this into the
delegate.")

[commit-bot: I haz the power, 2016-03-30 21:11:32]

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

[mfomitchev, 2016-03-30 21:25:34]

The CQ bit was unchecked by mfomitchev@chromium.org

[mfomitchev, 2016-03-30 21:28:22]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-03-30 21:28:22]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1830293002/#ps180001
(title: "Updating gyp/gn to remove keyboard dependency.")

[commit-bot: I haz the power, 2016-03-30 21:29:04]

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

[mfomitchev, 2016-03-30 21:43:02]

The CQ bit was unchecked by mfomitchev@chromium.org

[mfomitchev, 2016-04-04 15:57:40]

Patchset #11 (id:200001) has been deleted

[mfomitchev, 2016-04-04 16:06:08]

Patchset #11 (id:220001) has been deleted

[xiyuan, 2016-04-04 17:09:30]

SLGTM

Since we are adding a new test binary app_list_shower_unittests, please remember
to follow up and add it to trybots and build bots. Otherwise, we lose test
coverage.

https://codereview.chromium.org/1830293002/diff/240001/.gn
File .gn (right):

https://codereview.chromium.org/1830293002/diff/240001/.gn#newcode106
.gn:106: "//ui/app_list/shower/*",
Would this be included in "//ui/app_list/*" line above?

https://codereview.chromium.org/1830293002/diff/240001/ui/app_list/app_list_s...
File ui/app_list/app_list_shower_delegate.h (right):

https://codereview.chromium.org/1830293002/diff/240001/ui/app_list/app_list_s...
ui/app_list/app_list_shower_delegate.h:34: void virtual Init(AppListView* view,
nit: not sure how I missed this. The prevailing style is to put "virtual" first.

[mfomitchev, 2016-04-04 18:59:06]

> Since we are adding a new test binary app_list_shower_unittests, please
remember
> to follow up and add it to trybots and build bots. Otherwise, we lose test
> coverage.

Done

https://codereview.chromium.org/1830293002/diff/240001/.gn
File .gn (right):

https://codereview.chromium.org/1830293002/diff/240001/.gn#newcode106
.gn:106: "//ui/app_list/shower/*",
On 2016/04/04 17:09:30, xiyuan wrote:
> Would this be included in "//ui/app_list/*" line above?

Yup, you are right, this is not needed. Removed.

https://codereview.chromium.org/1830293002/diff/240001/ui/app_list/app_list_s...
File ui/app_list/app_list_shower_delegate.h (right):

https://codereview.chromium.org/1830293002/diff/240001/ui/app_list/app_list_s...
ui/app_list/app_list_shower_delegate.h:34: void virtual Init(AppListView* view,
On 2016/04/04 17:09:30, xiyuan wrote:
> nit: not sure how I missed this. The prevailing style is to put "virtual"
first.

Done.

[mfomitchev, 2016-04-05 03:26:49]

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

[mfomitchev, 2016-04-05 03:26:50]

dpranke@chromium.org: Please review changes in 

testing/buildbot, BUILD.gn, build/all.gyp

[Dirk Pranke, 2016-04-05 17:05:37]

lgtm

[mfomitchev, 2016-04-05 17:13:44]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2016-04-05 17:13:44]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1830293002/#ps380001
(title: "Fixing target names in .isolate.")

[commit-bot: I haz the power, 2016-04-05 17:14:07]

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

[commit-bot: I haz the power, 2016-04-05 17:24:57]

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

[commit-bot: I haz the power, 2016-04-05 17:24:58]

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

[mfomitchev, 2016-04-05 17:29:13]

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

[mfomitchev, 2016-04-05 17:30:18]

+sievers for:
"ui/gl/test/gl_surface_test_support.h" dependency in
ui/app_list/shower/test/DEPS

[no sievers, 2016-04-06 17:17:54]

On 2016/04/05 17:30:18, mfomitchev wrote:
> +sievers for:
> "ui/gl/test/gl_surface_test_support.h" dependency in
> ui/app_list/shower/test/DEPS

lgtm. that was specifically added for gl_unittests, but I guess I don't see
anything wrong with using it for other unittests.

[mfomitchev, 2016-04-06 17:35:07]

On 2016/04/06 17:17:54, sievers wrote:
> On 2016/04/05 17:30:18, mfomitchev wrote:
> > +sievers for:
> > "ui/gl/test/gl_surface_test_support.h" dependency in
> > ui/app_list/shower/test/DEPS
> 
> lgtm. that was specifically added for gl_unittests, but I guess I don't see
> anything wrong with using it for other unittests.

Thanks! Note it's already used for app_list tests
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/test/D...)
and in a couple of other tests.

[mfomitchev, 2016-04-06 17:35:16]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2016-04-06 17:35:39]

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

[commit-bot: I haz the power, 2016-04-06 19:45:48]

Description was changed from

==========
AppListController refactoring part 1: AppListShower implementation.

This is the first part of ash::AppListController refactoring preparing it
for the Mus+ash use.
The goal of the refactoring is to break up ash::AppListController into two
parts: AppListShower (implemented in ui/app_list/) and AppListDelegate
(implemented in ash/wm). In Mus+ash world, app list will live in the chrome
process, and mash/SysUI will interact with it via a thin interface.
More details in the design doc:
https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca...

This CL adds interfaces for AppListShower and AppListDelegate, as well as the
default implementation of the shower: AppListShowerImpl and unit tests.

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

to

==========
AppListController refactoring part 1: AppListShower implementation.

This is the first part of ash::AppListController refactoring preparing it
for the Mus+ash use.
The goal of the refactoring is to break up ash::AppListController into two
parts: AppListShower (implemented in ui/app_list/) and AppListDelegate
(implemented in ash/wm). In Mus+ash world, app list will live in the chrome
process, and mash/SysUI will interact with it via a thin interface.
More details in the design doc:
https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca...

This CL adds interfaces for AppListShower and AppListDelegate, as well as the
default implementation of the shower: AppListShowerImpl and unit tests.

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

[commit-bot: I haz the power, 2016-04-06 19:45:49]

Committed patchset #18 (id:380001)

[commit-bot: I haz the power, 2016-04-06 19:48:20]

Description was changed from

==========
AppListController refactoring part 1: AppListShower implementation.

This is the first part of ash::AppListController refactoring preparing it
for the Mus+ash use.
The goal of the refactoring is to break up ash::AppListController into two
parts: AppListShower (implemented in ui/app_list/) and AppListDelegate
(implemented in ash/wm). In Mus+ash world, app list will live in the chrome
process, and mash/SysUI will interact with it via a thin interface.
More details in the design doc:
https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca...

This CL adds interfaces for AppListShower and AppListDelegate, as well as the
default implementation of the shower: AppListShowerImpl and unit tests.

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

to

==========
AppListController refactoring part 1: AppListShower implementation.

This is the first part of ash::AppListController refactoring preparing it
for the Mus+ash use.
The goal of the refactoring is to break up ash::AppListController into two
parts: AppListShower (implemented in ui/app_list/) and AppListDelegate
(implemented in ash/wm). In Mus+ash world, app list will live in the chrome
process, and mash/SysUI will interact with it via a thin interface.
More details in the design doc:
https://docs.google.com/document/d/1M9vqTTuprMssRXs8UIVaFjQUq3lUxSzwrWoPjPvca...

This CL adds interfaces for AppListShower and AppListDelegate, as well as the
default implementation of the shower: AppListShowerImpl and unit tests.

BUG=557408

Committed: https://crrev.com/216ff0ba63afa3ba3ec066d12fac41df95fdc7e0
Cr-Commit-Position: refs/heads/master@{#385526}
==========

[commit-bot: I haz the power, 2016-04-06 19:48:21]

Patchset 18 (id:??) landed as
https://crrev.com/216ff0ba63afa3ba3ec066d12fac41df95fdc7e0
Cr-Commit-Position: refs/heads/master@{#385526}

[Nico, 2016-04-08 21:39:37]

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

[Nico, 2016-04-08 21:39:38]

I'm not a native speaker, but "shower" isn't something I'd associate with "noun
form of 'to show'", but, well, a shower:
https://www.google.com/search?q=shower&espv=2&biw=1741&bih=1490&source=lnms&t...

Maybe this could have a better name.

(Sorry for bikeshedding.)

[mfomitchev, 2016-04-08 22:19:59]

On 2016/04/08 21:39:38, Nico (hiding until Fri) wrote:
> I'm not a native speaker, but "shower" isn't something I'd associate with
"noun
> form of 'to show'", but, well, a shower:
>
https://www.google.com/search?q=shower&espv=2&biw=1741&bih=1490&source=lnms&t...
> 
> Maybe this could have a better name.
> 
> (Sorry for bikeshedding.)

There is precedent: the functionality here is pretty similar to AppListShower
here: chrome/browser/ui/app_list/app_list_shower_views.h, so I thought it would
be reasonable to borrow the name. Also this old AppListShower should be going
away soon. I'd call the new class AppListController, except we already have one,
and it's doing something a bit different. I don't hate "shower", but if the
owners can agree to something better, I don't mind changing it.

[sky, 2016-04-11 15:03:36]

Presenter?

On Fri, Apr 8, 2016 at 3:19 PM,  <mfomitchev@chromium.org> wrote:
> On 2016/04/08 21:39:38, Nico (hiding until Fri) wrote:
>> I'm not a native speaker, but "shower" isn't something I'd associate with
> "noun
>> form of 'to show'", but, well, a shower:
>>
>
https://www.google.com/search?q=shower&espv=2&biw=1741&bih=1490&source=lnms&t...
>>
>> Maybe this could have a better name.
>>
>> (Sorry for bikeshedding.)
>
> There is precedent: the functionality here is pretty similar to
> AppListShower
> here: chrome/browser/ui/app_list/app_list_shower_views.h, so I thought it
> would
> be reasonable to borrow the name. Also this old AppListShower should be
> going
> away soon. I'd call the new class AppListController, except we already have
> one,
> and it's doing something a bit different. I don't hate "shower", but if the
> owners can agree to something better, I don't mind changing it.
>
> https://codereview.chromium.org/1830293002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mfomitchev, 2016-04-13 21:17:30]

On 2016/04/11 15:03:36, sky wrote:
> Presenter?
> 
> On Fri, Apr 8, 2016 at 3:19 PM,  <mailto:mfomitchev@chromium.org> wrote:
> > On 2016/04/08 21:39:38, Nico (hiding until Fri) wrote:
> >> I'm not a native speaker, but "shower" isn't something I'd associate with
> > "noun
> >> form of 'to show'", but, well, a shower:
> >>
> >
>
https://www.google.com/search?q=shower&espv=2&biw=1741&bih=1490&source=lnms&t...
> >>
> >> Maybe this could have a better name.
> >>
> >> (Sorry for bikeshedding.)
> >
> > There is precedent: the functionality here is pretty similar to
> > AppListShower
> > here: chrome/browser/ui/app_list/app_list_shower_views.h, so I thought it
> > would
> > be reasonable to borrow the name. Also this old AppListShower should be
> > going
> > away soon. I'd call the new class AppListController, except we already have
> > one,
> > and it's doing something a bit different. I don't hate "shower", but if the
> > owners can agree to something better, I don't mind changing it.
> >
> > https://codereview.chromium.org/1830293002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

https://codereview.chromium.org/1890583002/

[mfomitchev, 2016-04-13 21:17:31]

On 2016/04/11 15:03:36, sky wrote:
> Presenter?
> 
> On Fri, Apr 8, 2016 at 3:19 PM,  <mailto:mfomitchev@chromium.org> wrote:
> > On 2016/04/08 21:39:38, Nico (hiding until Fri) wrote:
> >> I'm not a native speaker, but "shower" isn't something I'd associate with
> > "noun
> >> form of 'to show'", but, well, a shower:
> >>
> >
>
https://www.google.com/search?q=shower&espv=2&biw=1741&bih=1490&source=lnms&t...
> >>
> >> Maybe this could have a better name.
> >>
> >> (Sorry for bikeshedding.)
> >
> > There is precedent: the functionality here is pretty similar to
> > AppListShower
> > here: chrome/browser/ui/app_list/app_list_shower_views.h, so I thought it
> > would
> > be reasonable to borrow the name. Also this old AppListShower should be
> > going
> > away soon. I'd call the new class AppListController, except we already have
> > one,
> > and it's doing something a bit different. I don't hate "shower", but if the
> > owners can agree to something better, I don't mind changing it.
> >
> > https://codereview.chromium.org/1830293002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

https://codereview.chromium.org/1890583002/