Refactor views app list services to allow more code sharing

Refactor views app list services to allow more code sharing

To run the toolkit-views app launcher on Mac, it needs a new app list
service + supporting classes. It's going to look a lot like the Linux
one. This CL starts by sharing what can be shared between Windows and
Linux, in preparation for using it on Mac as well.

It does this principally by:
- Moving `app_list_shower` to `app_list_shower_views`, making it views-specific;
- Adding AppListServiceViews, which Win/Linux (and soon Mac) inherit from;
- Rearranging the delegation to eliminate some boilerplate.

Other changes:
- Removed AppList container abstract interface (now just access AppListView);
- Removed AppListFactory (now AppListShower::MakeViewForCurrentProfile());
- Moved AppListControllerDelegateLinux to AppListControllerDelegateViews;
- Most of AppListServiceLinux moves to AppListServiceViews;
- Added AppListShowerDelegate and AppListShower given virtual method
hooks to assist testability (previously AppListShower was given a NULL
service/delegate in tests);
- Positioning logic in AppListWin/AppListLinux made static/stateless;
- Removes the `on_should_dismiss_` closure (now just dismiss via the service;
- Big trim of AppListServiceWin and AppListServiceLinux - now these are
just platform-specific stuff.

BUG=365977
TEST=Just refactoring, nothing should change

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267184

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267354

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268035

[tapted, 2014-04-23 03:06:53]

Note Rietveld is a bit generous with the diffstat. It should be:
421 insertions(+), 668 deletions(-)

https://codereview.chromium.org/225053004/diff/420001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views.cc (right):

https://codereview.chromium.org/225053004/diff/420001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views.cc:43: }
apart from "shower_->" becoming "shower_.", this is verbatim from
AppListServiceLinux::ShowForProfile() (== AppListServiceWin::ShowForProfile)

https://codereview.chromium.org/225053004/diff/420001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/activation_tracker_win.cc (right):

https://codereview.chromium.org/225053004/diff/420001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/activation_tracker_win.cc:28:
DCHECK(service_->shower().app_list());
turns out: the next line will not actually crash if this is NULL - it would just
pump the next message from the Windows message queue - bizarre. Hence: DCHECK

[benwells, 2014-04-24 01:32:24]

-me, +mgiuca to review.

[Matt Giuca, 2014-04-29 03:52:51]

Some initial comments. Let's discuss in person.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_factory.h (left):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_factory.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
I'm confused about this, is it Reitveld's fault?

This file, along with app_list_shower and app_list_shower_unittest, have been
entirely erased, but Reitveld shows them as M instead of D. Have the files
actually been deleted, or just emptied?

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_views.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.h:23: class AppListShower {
Shouldn't this be called AppListShowerViews?

(Actually, I'm not sure why you can't just call it, and the file name,
AppListShower if there isn't going to be a non-views one. Same for the other new
*Views things you're adding in this CL.)

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/linux/app_list_linux.cc (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/linux/app_list_linux.cc:53: // TODO(mgiuca):
Support other window manager configurations, and multiple
Sorry, you're going to have a minor merge conflict here (but it won't break your
architecture).

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/linux/app_list_linux.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/linux/app_list_linux.h:21: class AppListLinux {
This isn't really a class any more. Can you just take those static methods out
and make them separate functions? (Probably should be in the app_list
namespace.)

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/app_list_win.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_win.h:20: class AppListWin {
Same comment as for AppListLinux.

[tapted, 2014-04-29 04:20:30]

oops - just noticed I had some self-nits - I'll check them off in the next
patchset.

Let's discuss!

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_factory.h (left):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_factory.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2014/04/29 03:52:51, Matt Giuca wrote:
> I'm confused about this, is it Reitveld's fault?
> 
> This file, along with app_list_shower and app_list_shower_unittest, have been
> entirely erased, but Reitveld shows them as M instead of D. Have the files
> actually been deleted, or just emptied?

I blame rietveld. The raw diff has

Index: chrome/browser/ui/app_list/app_list_factory.h
diff --git a/chrome/browser/ui/app_list/app_list_factory.h
b/chrome/browser/ui/app_list/app_list_factory.h
deleted file mode 100644


Uploading to a new CL might convince Rietveld to do the right thing.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views.h:43: //
AppListCreatorDelegate overrides:
oops

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_delegate.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_delegate.h:19: virtual void
OnViewHidden() = 0;
onviewdismissed?

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_views.cc (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.cc:106: // manages it's own
lifetime.
its

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_views.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.h:23: class AppListShower {
On 2014/04/29 03:52:51, Matt Giuca wrote:
> Shouldn't this be called AppListShowerViews?
> 
> (Actually, I'm not sure why you can't just call it, and the file name,
> AppListShower if there isn't going to be a non-views one. Same for the other
new
> *Views things you're adding in this CL.)

app_list_shower.cc is currently compiled on Mac even though it is not used there
at all. Adding _views ensures it's only compiled when toolkit_views=1 (which
will soon "sometimes" be true on Mac).

As for whether the class name should correspond for this use case - there's not
much consistency around Chrome. I think it should be AppListShowerViews and only
left it as `AppListShower`to keep the diff more sane :).

So I'll rename towards the end of review, or in a follow-up CL.

(I had actually already renamed AppListShower to AppListCreator in earlier
iterations since `Shower` always seemed a bit awkward, but all that was
orthogonal and messed up the diff).

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/linux/app_list_linux.cc (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/linux/app_list_linux.cc:53: // TODO(mgiuca):
Support other window manager configurations, and multiple
On 2014/04/29 03:52:51, Matt Giuca wrote:
> Sorry, you're going to have a minor merge conflict here (but it won't break
your
> architecture).

No probs!

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/linux/app_list_linux.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/linux/app_list_linux.h:21: class AppListLinux {
On 2014/04/29 03:52:51, Matt Giuca wrote:
> This isn't really a class any more. Can you just take those static methods out
> and make them separate functions? (Probably should be in the app_list
> namespace.)

Yep! I'd actually propose putting `static void
MoveNearCursor(app_list::AppListView* view);` in app_list_positioner.h, then
defining these functions in app_list_postitioner{_linux,_win,_mac}.cc . Except
that raises a question around whether AppListView should appear (even if just a
forward-dec) in a header used on Cocoa.

The reason for delay was again just to keep the diff slightly saner for the
initial review, so I was planning this for a follow-up CL.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (left):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:18: #include
"base/threading/sequenced_worker_pool.h"
safe?

[Matt Giuca, 2014-04-30 00:41:22]

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views.h:43: //
AppListCreatorDelegate overrides:
The Oops is to change this to AppListControllerDelegate, right?

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_delegate.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_delegate.h:19: virtual void
OnViewHidden() = 0;
SGTM.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_views.cc (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.cc:106: // manages it's own
lifetime.
Oops, I wonder if that's my fault :|

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_views.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.h:23: class AppListShower {
OK. I'd prefer not to use Creator because AFAIK the app list is not created /
destroyed, just hidden / shown. Is that correct?

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.h:38: virtual void
HandleViewBeingDestroyed();
Maybe:
// Virtual functions mocked out in tests.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.h:66:
scoped_ptr<ScopedKeepAlive> keep_alive_;
Confused about why this comment was there before but not the variable... :|

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_views_unittest.cc (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views_unittest.cc:16: class
TestAppListShower : public AppListShower {
Is FakeAppListShower a better name? (This is not a test, it is a fake.)

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/linux/app_list_linux.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/linux/app_list_linux.h:21: class AppListLinux {
OK, I'd rather not move these around in this CL. I agree, too much diff. (Plus
I'm editing this file in another CL and I'd rather a line-level conflict than a
file-level one!)

Can you add // TODO to move the functions outside this class?

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/linux/app_list_service_linux.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/linux/app_list_service_linux.h:25: //
app_list::AppListViewObserver:
"overrides" (consistency with above)

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/app_list_service_win.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.h:33: //
AppListServiceViews:
nit: "overrides"

[tapted, 2014-04-30 03:26:02]

Thanks Matt!

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_views.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_views.h:43: //
AppListCreatorDelegate overrides:
On 2014/04/30 00:41:23, Matt Giuca wrote:
> The Oops is to change this to AppListControllerDelegate, right?

close! AppListShowerDelegate.

Done.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_delegate.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_delegate.h:19: virtual void
OnViewHidden() = 0;
On 2014/04/30 00:41:23, Matt Giuca wrote:
> SGTM.

Done.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_views.cc (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.cc:106: // manages it's own
lifetime.
On 2014/04/30 00:41:23, Matt Giuca wrote:
> Oops, I wonder if that's my fault :|

Done (I blame English - its its fault :p)

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_views.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.h:23: class AppListShower {
On 2014/04/30 00:41:23, Matt Giuca wrote:
> OK. I'd prefer not to use Creator because AFAIK the app list is not created /
> destroyed, just hidden / shown. Is that correct?

It does create, but then it's owned by the native widget (so not destroyed
here). I backed away from 'Creator' after I decided not to move the show/hide
functions into AppListServiceViews (which could easily be done and would remove
another layer of delegation). Leaving show/hide on Shower makes the tests a bit
more compelling, but really there should be more tests on the Service as well.

Probably 'Controller' is the right name... but the ControllerDelegate already
exists, and is not this guy's delegate, so that would be confusing.

A nice follow-up might be to have AppListShower implement
AppListControllerDelegate and merge it with AppListControllerDelegateViews --
that's probably more consistent with typical design patterns, but it might make
testing more convoluted too.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.h:38: virtual void
HandleViewBeingDestroyed();
On 2014/04/30 00:41:23, Matt Giuca wrote:
> Maybe:
> // Virtual functions mocked out in tests.

Done.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views.h:66:
scoped_ptr<ScopedKeepAlive> keep_alive_;
On 2014/04/30 00:41:23, Matt Giuca wrote:
> Confused about why this comment was there before but not the variable... :|

Yeah - it was like this when app_list_shower.h first came into existence in
r225655

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower_views_unittest.cc (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower_views_unittest.cc:16: class
TestAppListShower : public AppListShower {
On 2014/04/30 00:41:23, Matt Giuca wrote:
> Is FakeAppListShower a better name? (This is not a test, it is a fake.)

Done.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/linux/app_list_linux.cc (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/linux/app_list_linux.cc:53: // TODO(mgiuca):
Support other window manager configurations, and multiple
On 2014/04/29 04:20:30, tapted wrote:
> On 2014/04/29 03:52:51, Matt Giuca wrote:
> > Sorry, you're going to have a minor merge conflict here (but it won't break
> your
> > architecture).
> 
> No probs! 

Done.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/linux/app_list_linux.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/linux/app_list_linux.h:21: class AppListLinux {
On 2014/04/30 00:41:23, Matt Giuca wrote:
> OK, I'd rather not move these around in this CL. I agree, too much diff. (Plus
> I'm editing this file in another CL and I'd rather a line-level conflict than
a
> file-level one!)
> 
> Can you add // TODO to move the functions outside this class?

Done.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/linux/app_list_service_linux.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/linux/app_list_service_linux.h:25: //
app_list::AppListViewObserver:
On 2014/04/30 00:41:23, Matt Giuca wrote:
> "overrides" (consistency with above)

Done.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (left):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:18: #include
"base/threading/sequenced_worker_pool.h"
On 2014/04/29 04:20:30, tapted wrote:
> safe?

Done.

(context: this header has a habit of disappearing in official builds after
making through the CQ due to sequence_checker_impl.h -- pretty sure it is still
needed).

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/app_list_service_win.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_service_win.h:33: //
AppListServiceViews:
On 2014/04/30 00:41:23, Matt Giuca wrote:
> nit: "overrides"

Done.

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/app_list/win/app_list_win.h (right):

https://codereview.chromium.org/225053004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/app_list/win/app_list_win.h:20: class AppListWin {
On 2014/04/29 03:52:51, Matt Giuca wrote:
> Same comment as for AppListLinux.

Done.

[Matt Giuca, 2014-04-30 04:45:59]

Groovy! Thanks for doing all this work!

LGTM

[tapted, 2014-04-30 04:48:24]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-04-30 04:53:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/225053004/540001

[commit-bot: I haz the power, 2014-04-30 11:30:52]

Change committed as 267184

[tapted, 2014-04-30 11:57:03]

A revert of this CL has been created in
https://codereview.chromium.org/262643003/ by tapted@chromium.org.

The reason for reverting is: failed compile on clang dbg.

Overriding method must be marked with OVERRIDE.
  virtual app_list::AppListView* MakeViewForCurrentProfile() {
Overriding method must be marked with OVERRIDE.
  virtual AppListControllerDelegate* GetControllerDelegateForCreate() {
.

[tapted, 2014-04-30 11:59:19]

https://codereview.chromium.org/225053004/diff/540001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/225053004/diff/540001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2830: # Unit tests for chrome app list, not run on
any bots, this is for faster
turns out this comment is a lie.. I think it's time to fold these into
unit_tests so they get CQ coverage.

[tapted, 2014-04-30 12:43:26]

added the missing OVERRIDEs that clang wants on the waterfall but not
linux_chromium_clang_dbg in the CQ, nor my local linux setup where I tested it
all :|

https://codereview.chromium.org/225053004/diff/600001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_shower.h (left):

https://codereview.chromium.org/225053004/diff/600001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_shower.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
wow..... some script changed the date from 2013 to 2014 between the revert CL at
https://codereview.chromium.org/262643003/diff/1/chrome/browser/ui/app_list/a...
and the committed revision for it at
https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/app_list/a...

That made a fun mess to untangle...

[tapted, 2014-04-30 12:43:53]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-04-30 12:44:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/225053004/620001

[commit-bot: I haz the power, 2014-04-30 12:46:43]

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

[commit-bot: I haz the power, 2014-04-30 12:46:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[tapted, 2014-04-30 22:04:35]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:05:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/225053004/620001

[commit-bot: I haz the power, 2014-04-30 22:10:51]

Change committed as 267354

[Matt Giuca, 2014-04-30 22:18:41]

:( what a mess. No idea why there's a difference.

[tapted, 2014-05-01 06:53:09]

PTAL. Rebased on crrev/262773002 and updated build/filename_rules.gypi to make
the foo_views rule consistent with all the others.

app_list_shower_views_unittest.cc is the only thing currently affected by this.
Existing foo_views_unittests.cc files are all excluded with a regex on their
enclosing folder (I guess the foo_views.cc files everywhere else just don't have
tests of their own o_O).

That is, adding this rule doesn't cause any existing tests to get skipped.

[Matt Giuca, 2014-05-01 07:29:58]

lgtm

[tapted, 2014-05-03 00:15:23]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-05-03 00:16:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/225053004/640001

[commit-bot: I haz the power, 2014-05-03 00:16:51]

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

[commit-bot: I haz the power, 2014-05-03 00:16:52]

Failed to apply patch for chrome/chrome_browser_ui.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_browser_ui.gypi
  Hunk #3 FAILED at 1444.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/chrome_browser_ui.gypi.rej

Patch:       chrome/chrome_browser_ui.gypi
Index: chrome/chrome_browser_ui.gypi
diff --git a/chrome/chrome_browser_ui.gypi b/chrome/chrome_browser_ui.gypi
index
ca7c696d0f12495c5a6b7a25a0f922fd91c09bbb..0e351ecf1a1055122c070fd42f34b87c2a811197
100644
--- a/chrome/chrome_browser_ui.gypi
+++ b/chrome/chrome_browser_ui.gypi
@@ -135,12 +135,12 @@
         'browser/ui/app_list/app_context_menu.cc',
         'browser/ui/app_list/app_context_menu.h',
         'browser/ui/app_list/app_context_menu_delegate.h',
-        'browser/ui/app_list/app_list.h',
         'browser/ui/app_list/app_list_controller_delegate.cc',
         'browser/ui/app_list/app_list_controller_delegate.h',
         'browser/ui/app_list/app_list_controller_delegate_impl.cc',
         'browser/ui/app_list/app_list_controller_delegate_impl.h',
-        'browser/ui/app_list/app_list_factory.h',
+        'browser/ui/app_list/app_list_controller_delegate_views.cc',
+        'browser/ui/app_list/app_list_controller_delegate_views.h',
         'browser/ui/app_list/app_list_icon_win.cc',
         'browser/ui/app_list/app_list_icon_win.h',
         'browser/ui/app_list/app_list_positioner.cc',
@@ -152,8 +152,11 @@
         'browser/ui/app_list/app_list_service_impl.h',
         'browser/ui/app_list/app_list_service_mac.h',
         'browser/ui/app_list/app_list_service_mac.mm',
-        'browser/ui/app_list/app_list_shower.cc',
-        'browser/ui/app_list/app_list_shower.h',
+        'browser/ui/app_list/app_list_service_views.cc',
+        'browser/ui/app_list/app_list_service_views.h',
+        'browser/ui/app_list/app_list_shower_delegate.h',
+        'browser/ui/app_list/app_list_shower_views.cc',
+        'browser/ui/app_list/app_list_shower_views.h',
         'browser/ui/app_list/app_list_syncable_service.cc',
         'browser/ui/app_list/app_list_syncable_service.h',
         'browser/ui/app_list/app_list_syncable_service_factory.cc',
@@ -1441,8 +1444,6 @@
         'browser/ui/views/accessibility/invert_bubble_view.h',
         'browser/ui/views/accessibility/automation_manager_views.cc',
         'browser/ui/views/accessibility/automation_manager_views.h',
-       
'browser/ui/views/app_list/linux/app_list_controller_delegate_linux.cc',
-        'browser/ui/views/app_list/linux/app_list_controller_delegate_linux.h',
         'browser/ui/views/app_list/linux/app_list_linux.cc',
         'browser/ui/views/app_list/linux/app_list_linux.h',
         'browser/ui/views/app_list/linux/app_list_service_linux.cc',

[tapted, 2014-05-03 02:23:53]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-05-03 02:27:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/225053004/660001

[commit-bot: I haz the power, 2014-05-03 03:55:22]

Change committed as 268035