Skeleton for app_list on OSX, and refactoring for enable_app_list=1 on OS=="mac".

Skeleton for app_list on OSX, and refactoring for enable_app_list=1 on OS=="mac".

Moves ui/app_list code specific to toolkit-views to ui/app_list/views.
Update gyp[i], include guards, #includes for moved files.
Seed chrome/browser/ui/cocoa/app_list and ui/app_list/cocoa with a
skeleton Cocoa UI.

BUG=138633
TEST=Mostly build changes. Functionality of AppLauncher on ChromeOS
     and Windows should be unchanged.
     On OSX, without chrome running, `Chromium.app/Contents/MacOS/Chromium --show-app-list`
     should show a borderless gray window for 1 second (note: this is for
     testing only -- OSX needs a different approach for IPC).

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

[tapted, 2013-01-08 05:28:55]

@xiyuan for ui/app_list
@sky for
 - ash/
 - c/b/ui/views
 - c/chrome_browser_ui.gypi
 - (general file layout)
@sail for
 - c/b/ui/cocoa
 - +other Cocoa (see below)

@sail are you comfortable receiving the bulk of reviews for the Cocoa parts of
this new feature (or can you recommend a backup)?

And (sky/xiyuan/sail) assuming the layout of files here is correct, shall we add
(in a follow-up CL) an OWNERS file at /ui/app_list/cocoa/OWNERS ?

I'm still somewhat new on Chrome (and Cocoa, TBH). After discussing with our
team here, we propose it contain sail as owner, with myself in a comment until
I'm a full committer.

[sail, 2013-01-08 07:25:26]

Hi tapted, your cocoa changes look great. This looks like a good start.

We'll also need a unit test or a browser test (which ever is easier) for the app
list controller and the app list view.

It might be easier to manage if you split out the chrome/browser/ui/cocoa/*
changes into a separate CL. Up to you.

> @sail are you comfortable receiving the bulk of reviews for the Cocoa parts of
this new feature (or can you recommend a backup)?

Yea, you can definitely send me cocoa app list CLs.

> And (sky/xiyuan/sail) assuming the layout of files here is correct, shall we
add
(in a follow-up CL) an OWNERS file at /ui/app_list/cocoa/OWNERS ?

> I'm still somewhat new on Chrome (and Cocoa, TBH). After discussing with our
team here, we propose it contain sail as owner, with myself in a comment until
I'm a full committer.

Hm.. it'd be weird for an OWNER to be someone who didn't actually write the
code. How about not having an OWNERS file until you're a  full committer. This
way the parent OWNERS file can be used in the meantime.

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm (right):

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:38:
AppListController() : current_window_(NULL) {}
for Objective-C classes you should use nil instead of NULL. (Though this can go
away entirely if you use scoped_nsobject()).

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:45: NSWindow*
current_window_;
should be scoped_nsobject

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:77:
current_window_ = [[AppListView alloc] initAsBubble];
Should also call setReleasedWhenClosed:NO
This allows you to be clear about when the reference to the window is valid.

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:89:
[current_window_ makeKeyAndOrderFront:NSApp];
you can just use nil instead of NSApp

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_view.h (right):

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_view.h:7: @interface AppListView : NSWindow {
Would AppListWindow or AppListViewWindow be a better name?

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_view.h:8: @private
remove?

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_view.mm (right):

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_view.mm:9: const int kWindowWidth = 200;
move above the @implementation line?
or maybe inside the init method?

[sail, 2013-01-08 07:41:12]

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_view.h (right):

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_view.h:7: @interface AppListView : NSWindow {
On 2013/01/08 07:25:27, sail wrote:
> Would AppListWindow or AppListViewWindow be a better name?

Also, where is this bubble being shown from (a browser toolbar, web page, OS X
dock, etc...)? If it's from the browser window you'll probably want a bunch of
standard stuff that the other browser window bubbles already have. To get this
you'll either have to refactor the bubble code from chrome/browser/ui/cocoa or
move this class.

[xiyuan, 2013-01-08 17:09:37]

ui/app_list LGTM with nits

I am all for to have a /ui/app_list/cocoa/OWNERS file and have sail@ sit there.

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm (right):

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:97:
[current_window_ close];
Reset |current_window_| to nil after closing?

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/app_list.gyp
File ui/app_list/app_list.gyp (right):

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/app_list.gyp#n...
ui/app_list/app_list.gyp:69: 'views/search_result_view.cc',
sort the files alphabetically.

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/app_list.gyp#n...
ui/app_list/app_list.gyp:113: 'views/test/apps_grid_view_test_api.h',
sort the file. Header file should precede its cc file.

[xiyuan, 2013-01-08 17:10:37]

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/app_list.gyp
File ui/app_list/app_list.gyp (right):

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/app_list.gyp#n...
ui/app_list/app_list.gyp:113: 'views/test/apps_grid_view_test_api.h',
On 2013/01/08 17:09:37, xiyuan wrote:
> sort the file. Header file should precede its cc file.

Disregard this one. The current order is correct.

[sky, 2013-01-08 19:08:18]

LGTM

[tapted, 2013-01-09 04:29:49]

On 2013/01/08 07:25:26, sail wrote:
> We'll also need a unit test or a browser test (which ever is easier) for the
app
> list controller and the app list view.

Done - I've added a browser_test for the AppListController (all supported
platforms):
https://codereview.chromium.org/11784034/diff/32060/chrome/browser/ui/app_lis...
and an additional case for the existing app_list_unittests target:
https://codereview.chromium.org/11784034/diff/32060/ui/app_list/cocoa/app_lis...

PTAL.

However, I don't think any default builders are including app_list_unittests in
their default deps. Is it time to add
'../ui/app_list/app_list.gyp:app_list_unittests' to some targets in all.gyp?
(perhaps in an isolated CL?)

In the meantime, I've fired off a few bots with an explicit target.

> It might be easier to manage if you split out the chrome/browser/ui/cocoa/*
> changes into a separate CL. Up to you.

There would likely be some tricky/temporary gyp changes, or #ifdefs, if I did
that while switching enable_app_list on OSX in common.gypi. However, with the
unit test harness in /ui/app_list a lot of changes in future may get a goot
split this way.

> > And (sky/xiyuan/sail) assuming the layout of files here is correct, shall we
> add
> (in a follow-up CL) an OWNERS file at /ui/app_list/cocoa/OWNERS ?
> 
> > I'm still somewhat new on Chrome (and Cocoa, TBH). After discussing with our
> team here, we propose it contain sail as owner, with myself in a comment until
> I'm a full committer.
> 
> Hm.. it'd be weird for an OWNER to be someone who didn't actually write the
> code. How about not having an OWNERS file until you're a  full committer. This
> way the parent OWNERS file can be used in the meantime.

I'm OK with that, if the OWNERS are.. more feedback is always welcome :). The
rationale behind suggesting it was that there are not too many parent OWNERS at
/ui/app_list/cocoa. Possibilities are xiyuan (app_list), and sky,ben,thakis
(ui), (then root owners, +darin). I don't want to burden people for owners
stamps, if it doesn't make sense to, or if Cocoa is less interesting for them
;-).

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm (right):

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:38:
AppListController() : current_window_(NULL) {}
On 2013/01/08 07:25:27, sail wrote:
> for Objective-C classes you should use nil instead of NULL. (Though this can
go
> away entirely if you use scoped_nsobject()).

Good advice! Done (scoped_nsobject below).

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:45: NSWindow*
current_window_;
On 2013/01/08 07:25:27, sail wrote:
> should be scoped_nsobject

Done.

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:77:
current_window_ = [[AppListView alloc] initAsBubble];
On 2013/01/08 07:25:27, sail wrote:
> Should also call setReleasedWhenClosed:NO
> This allows you to be clear about when the reference to the window is valid.

Done (in initAsBubble).

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:89:
[current_window_ makeKeyAndOrderFront:NSApp];
On 2013/01/08 07:25:27, sail wrote:
> you can just use nil instead of NSApp

Done.

https://codereview.chromium.org/11784034/diff/8003/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:97:
[current_window_ close];
On 2013/01/08 17:09:37, xiyuan wrote:
> Reset |current_window_| to nil after closing?

I think with setReleasedWhenClosed:NO it can stick around so we can re-show it
like we do in app_list_controller_win.cc (but you're right that before I had
that change I should have been setting it to nil here). I've discovered that
"close" on Cocoa can be both "hide" and "close" in views-terminology.

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/app_list.gyp
File ui/app_list/app_list.gyp (right):

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/app_list.gyp#n...
ui/app_list/app_list.gyp:69: 'views/search_result_view.cc',
On 2013/01/08 17:09:37, xiyuan wrote:
> sort the files alphabetically.

Done. (how did I miss that.. I've now fed it /usr/bin/sort)

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_view.h (right):

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_view.h:7: @interface AppListView : NSWindow {
On 2013/01/08 07:41:12, sail wrote:
> On 2013/01/08 07:25:27, sail wrote:
> > Would AppListWindow or AppListViewWindow be a better name?
> 
> Also, where is this bubble being shown from (a browser toolbar, web page, OS X
> dock, etc...)? If it's from the browser window you'll probably want a bunch of
> standard stuff that the other browser window bubbles already have. To get this
> you'll either have to refactor the bubble code from chrome/browser/ui/cocoa or
> move this class.

It will be a bubble shown from the OSX Dock, possibly without any browser
windows visible. (And, on Windows, there is a flow where the user does not have
Chrome Browser visibly installed at all, but that might not be a priority for
the OSX app_list port). I did have an initial look at the various cocoa bubbles
under chrome/browser -- I will certainly be mining them for ideas, and mapping
out refactoring opportunities where I can. Although, there is not currently a
/ui/cocoa (like there is a /ui/views), so there are some logistics to tackle
there.

There's a tricky balance between how closely to mirror the toolkit-views
/ui/app_list and whether to lean on cocoa parts in /chrome/browser/ui. The
current plan is to keep the new app_list things out of /chrome/browser as much
as possible, to minimise coupling.

The rationale here for the name, "AppListView" was more on the "mirror
toolkit-views" side of the balance. At a high level, I think the existing
approach there is to populate the view and wrap it in a native window once we
know how big the view will be, and use the view as the main handle in the
controller. For the initial skeleton I wanted something basic, so it's just the
NSWindow with no views yet. However, you are right that it is confusing.

I've renamed this to AppListViewWindow (and updated filenames/gyp/etc.) so that
it's clearer that it's still the NSWindow at this point.

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_view.h:8: @private
On 2013/01/08 07:25:27, sail wrote:
> remove?

Done.

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
File ui/app_list/cocoa/app_list_view.mm (right):

https://codereview.chromium.org/11784034/diff/8003/ui/app_list/cocoa/app_list...
ui/app_list/cocoa/app_list_view.mm:9: const int kWindowWidth = 200;
On 2013/01/08 07:25:27, sail wrote:
> move above the @implementation line?
> or maybe inside the init method?

Done. (moved to init method)

[tapted, 2013-01-09 08:15:58]

On 2013/01/09 04:29:49, tapted wrote: 
> In the meantime, I've fired off a few bots with an explicit target.

Actually (for app_list_unittests) that doesn't work :(. Looks like there are a
few steps to adding things to the tryserver (e.g., a recent, new optional
target: http://crrev.com/11299263 http://crrev.com/11316278 ).

I don't think there's a better alternative to adding a new tryserver target to
include expose those tests to trybots (e.g. by piggy-backing off ui_unittests or
similar), without sacrificing the decoupling of /ui/../app_list.gyp. @sail: Are
you OK with the "offline" unit test for now, while I investigate getting
app_list_unittests into the builders in parallel?


The browser test I added, however, does get some coverage of the code I added
anyway. And works on OSX and ChromeOS. However, it does not work on win_rel,
which is unfortunate. There is http://crbug.com/168955 which may relate, but the
stack dump in the trybot output isn't very informative.

Patch set 5 disables the browser test on Windows, with a follow up to continue
in http://crbug.com/168955 . @sail How do you think we are looking in patch set
5?

[xiyuan, 2013-01-09 17:33:45]

On 2013/01/09 08:15:58, tapted wrote:
> On 2013/01/09 04:29:49, tapted wrote: 
> > In the meantime, I've fired off a few bots with an explicit target.
> 
> Actually (for app_list_unittests) that doesn't work :(. Looks like there are a
> few steps to adding things to the tryserver (e.g., a recent, new optional
> target: http://crrev.com/11299263 http://crrev.com/11316278 ).
> 
> I don't think there's a better alternative to adding a new tryserver target to
> include expose those tests to trybots (e.g. by piggy-backing off ui_unittests
or
> similar), without sacrificing the decoupling of /ui/../app_list.gyp. @sail:
Are
> you OK with the "offline" unit test for now, while I investigate getting
> app_list_unittests into the builders in parallel?
> 

fyi, mukai's https://codereview.chromium.org/11637051 added app_list_unittests
for aura_builder. The bots should built it now but probably not run it as a test
step.

[sail, 2013-01-09 22:09:53]

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:27:
app_list_controller::ShowAppList();
I think this should be a platform specific test. Then you could write something
like this:

AppListController controller;
controller.ShowAppList();
EXPECT_TRUE([controller.current_window() isVisible]);
controller.DismissView();
EXPECT_FALSE([controller.current_window() isVisible]);

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm (right):

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:16: class
AppListControllerDelegateCocoa : public AppListControllerDelegate {
is this class used anywhere?

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:101: void
CreateAppList() {
is this function used anywhere?

https://codereview.chromium.org/11784034/diff/28004/ui/app_list/cocoa/app_lis...
File ui/app_list/cocoa/app_list_view_window_unittest.mm (right):

https://codereview.chromium.org/11784034/diff/28004/ui/app_list/cocoa/app_lis...
ui/app_list/cocoa/app_list_view_window_unittest.mm:30: window_.reset(nil);
don't need nil here

[benwells, 2013-01-09 23:35:46]

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:27:
app_list_controller::ShowAppList();
On 2013/01/09 22:09:53, sail wrote:
> I think this should be a platform specific test. Then you could write
something
> like this:
> 
> AppListController controller;
> controller.ShowAppList();
> EXPECT_TRUE([controller.current_window() isVisible]);
> controller.DismissView();
> EXPECT_FALSE([controller.current_window() isVisible]);

It would be good to test that the window is visible, but we should be able to do
that in a platform independent way and get coverage on Windows as well.

Tests for the app list on windows are non-existent, and that is my fault. I've
logged http://crbug.com/169114 to track this. If it's OK with you I will expose
the necessary plumbing to test visibility and other basic stuff across platforms
after this patch is landed.

[sail, 2013-01-09 23:38:38]

On 2013/01/09 23:35:46, benwells wrote:
>
https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/app_lis...
> File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):
> 
>
https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/app_lis...
> chrome/browser/ui/app_list/app_list_controller_browsertest.cc:27:
> app_list_controller::ShowAppList();
> On 2013/01/09 22:09:53, sail wrote:
> > I think this should be a platform specific test. Then you could write
> something
> > like this:
> > 
> > AppListController controller;
> > controller.ShowAppList();
> > EXPECT_TRUE([controller.current_window() isVisible]);
> > controller.DismissView();
> > EXPECT_FALSE([controller.current_window() isVisible]);
> 
> It would be good to test that the window is visible, but we should be able to
do
> that in a platform independent way and get coverage on Windows as well.
> 
> Tests for the app list on windows are non-existent, and that is my fault. I've
> logged http://crbug.com/169114 to track this. If it's OK with you I will
expose
> the necessary plumbing to test visibility and other basic stuff across
platforms
> after this patch is landed.

Sounds good to me. I'm not convinced that we'll be able to do all testing in
cross platform code but we can address that later.

[tapted, 2013-01-10 00:07:35]

I'll need to wait for http://crrev.com/11834002 to land and hit lkgr (currently
in the CQ), and do a rebase.

I should just need to change a #include and the namespace (app_list_controller::
--> chrome::), but let me know if I should ping here beforehand.

On 2013/01/09 23:38:38, sail wrote:
> Sounds good to me. I'm not convinced that we'll be able to do all testing in
> cross platform code but we can address that later.

I'll keep in mind that an additional, platform-specific browser_test may be on
the cards. E.g. I think the AppListControllerDelegateCocoa I just took out will
be better in its own header to facilitate this.

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm (right):

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:16: class
AppListControllerDelegateCocoa : public AppListControllerDelegate {
On 2013/01/09 22:09:53, sail wrote:
> is this class used anywhere?

No, not yet (I have this stub plumbed through in a local branch). I'll delete it
from this here, moving it to the next CL. Also, that plumbing with conflict with
http://crrev.com/11573026 in review, so it would be good to delay a bit.

https://codereview.chromium.org/11784034/diff/28004/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:101: void
CreateAppList() {
On 2013/01/09 22:09:53, sail wrote:
> is this function used anywhere?

Not yet - I'll delete it. And good point - this may be something that I can
refactor to share between platforms. (context: on Windows it's called from a
PostDelayedTask in InitAppList to pre-populate the views for a faster show).

https://codereview.chromium.org/11784034/diff/28004/ui/app_list/cocoa/app_lis...
File ui/app_list/cocoa/app_list_view_window_unittest.mm (right):

https://codereview.chromium.org/11784034/diff/28004/ui/app_list/cocoa/app_lis...
ui/app_list/cocoa/app_list_view_window_unittest.mm:30: window_.reset(nil);
On 2013/01/09 22:09:53, sail wrote:
> don't need nil here

Done.

[sail, 2013-01-10 00:12:49]

LGTM!

https://codereview.chromium.org/11784034/diff/39004/ui/app_list/cocoa/app_lis...
File ui/app_list/cocoa/app_list_view_window_unittest.mm (right):

https://codereview.chromium.org/11784034/diff/39004/ui/app_list/cocoa/app_lis...
ui/app_list/cocoa/app_list_view_window_unittest.mm:18:
scoped_nsobject<AppListViewWindow> window_;
should be protected

[commit-bot: I haz the power, 2013-01-10 04:56:05]

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

[tapted, 2013-01-10 04:56:13]

Thanks!

(lkgr has arrived.)

https://chromiumcodereview.appspot.com/11784034/diff/39004/ui/app_list/cocoa/...
File ui/app_list/cocoa/app_list_view_window_unittest.mm (right):

https://chromiumcodereview.appspot.com/11784034/diff/39004/ui/app_list/cocoa/...
ui/app_list/cocoa/app_list_view_window_unittest.mm:18:
scoped_nsobject<AppListViewWindow> window_;
On 2013/01/10 00:12:49, sail wrote:
> should be protected

Done.

[commit-bot: I haz the power, 2013-01-10 07:33:14]

Change committed as 176042