Initial stubs for App Launcher on Linux Aura.

Initial stubs for App Launcher on Linux Aura.

Adds AppListServiceLinux which can be successfully created, but won't
show anything as all of the methods are stubs. Affects only the Aura
build for Linux.

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

[Matt Giuca, 2013-09-26 04:10:39]

https://codereview.chromium.org/24710002/diff/1/chrome/browser/ui/app_list/ap...
File chrome/browser/ui/app_list/app_list_service_linux.cc (right):

https://codereview.chromium.org/24710002/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_service_linux.cc:14:
LeakySingletonTraits<AppListServiceLinux> >::get();
Can anybody explain why LeakySingletonTraits is used here? (It's in
AppListController (win) and AppListServiceMac.)

[Matt Giuca, 2013-09-26 04:36:41]

Note: This doesn't affect the default build on Linux, because
app_list_service_linux.cc is not compiled. So it just changes the build with
enable_app_list=1 (which is not the default on Linux).

[tapted, 2013-09-26 04:39:56]

I think you should change build/common.gypi too, otherwise all this added code
is dead to the CQ..

https://codereview.chromium.org/24710002/diff/1/chrome/browser/ui/app_list/ap...
File chrome/browser/ui/app_list/app_list_service_linux.cc (right):

https://codereview.chromium.org/24710002/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_service_linux.cc:14:
LeakySingletonTraits<AppListServiceLinux> >::get();
On 2013/09/26 04:10:40, Matt Giuca wrote:
> Can anybody explain why LeakySingletonTraits is used here? (It's in
> AppListController (win) and AppListServiceMac.)

It prevents a call to the AppListServiceLinux destructor being made during the
atexit handlers. Since it's virtual, it "can't" be elided, but it's also really
boring, so would just make Chrome take a few microseconds longer to exit.

https://codereview.chromium.org/24710002/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_service_linux.cc:18: NOTIMPLEMENTED();
You can probably just `HandleCommandLineFlags(initial_profile);` here - there
may be nothing else you need.

[koz (OOO until 15th September), 2013-09-26 04:42:33]

https://codereview.chromium.org/24710002/diff/1/chrome/browser/ui/app_list/ap...
File chrome/browser/ui/app_list/app_list_service_linux.h (right):

https://codereview.chromium.org/24710002/diff/1/chrome/browser/ui/app_list/ap...
chrome/browser/ui/app_list/app_list_service_linux.h:13: // operate, and controls
when the app list is opened and closed.
It would be more accurate to say that it opens and closes the app list at the
request of its clients, ie: it doesn't control when the app list is opened.

[Matt Giuca, 2013-09-26 05:05:45]

On 2013/09/26 04:39:56, tapted wrote:
> I think you should change build/common.gypi too, otherwise all this added code
> is dead to the CQ..

Do you mean to turn on enable_app_list=1 for Linux? I specifically don't want to
do this yet, because
a) there are NOTIMPLEMENTEDs that will fire on normal Chrome startup,
b) I'm not sure what this will do on the GTK build, and
c) it will increase the size of the Chrome binary for M32 (and it's not clear we
want it just yet).

Do we need it in the CQ just yet?

[tapted, 2013-09-26 05:12:07]

On 2013/09/26 05:05:45, Matt Giuca wrote:
> On 2013/09/26 04:39:56, tapted wrote:
> > I think you should change build/common.gypi too, otherwise all this added
code
> > is dead to the CQ..
> 
> Do you mean to turn on enable_app_list=1 for Linux? I specifically don't want
to
> do this yet, because
> a) there are NOTIMPLEMENTEDs that will fire on normal Chrome startup,

You shouldn't get this if you add HandleCommandLineFlags to the Init().

> b) I'm not sure what this will do on the GTK build, and

You should only enable it on linux if use_aura is true - GTK is lost.

> c) it will increase the size of the Chrome binary for M32 (and it's not clear
we
> want it just yet).

It should only affect the linux_aura builds, so nothing we release yet :) (but
also the linker in release builds should be able to scrap the unused symbols
until you start referencing stuff in /ui/app_list)

> 
> Do we need it in the CQ just yet?

[Matt Giuca, 2013-09-26 09:01:14]

> > a) there are NOTIMPLEMENTEDs that will fire on normal Chrome startup,
> 
> You shouldn't get this if you add HandleCommandLineFlags to the Init().

Well, it doesn't really require adding HandleCommandLineFlags, just removing the
NOTIMPLEMENTED() will suffice.

HandleCommandLineFlags doesn't seem to do much (it just checks whether the app
list is being enabled and if so calls CreateShortcut). But I guess putting it in
there justifies removing the NOTIMPLEMENTED, so may as well.

> > b) I'm not sure what this will do on the GTK build, and
> 
> You should only enable it on linux if use_aura is true - GTK is lost.

Done.

> > c) it will increase the size of the Chrome binary for M32 (and it's not
clear
> we
> > want it just yet).
> 
> It should only affect the linux_aura builds, so nothing we release yet :) (but
> also the linker in release builds should be able to scrap the unused symbols
> until you start referencing stuff in /ui/app_list)

Well I don't think the latter part is true, because as soon as you turn the flag
on, the Get() symbol is going to be used and that will drag in the entire app
list.

But it's okay because it's now disabled in the non-Aura build.

[tfarina, 2013-09-26 13:25:15]

There is no bug associated with this work?

[benwells, 2013-09-26 22:59:59]

On 2013/09/26 13:25:15, tfarina wrote:
> There is no bug associated with this work?

There is now: https://code.google.com/p/chromium/issues/detail?id=299250

[koz (OOO until 15th September), 2013-09-26 23:22:13]

lgtm!

This is the start of a beautiful friendship :-)

Don't forget to add BUG=

[tapted, 2013-09-26 23:51:18]

(as discussed) I think you're now exposed to app_list_controller_browsertest.cc
- I think the right thing to do is to wrap the tests that don't yet pass (or are
missing test harness pieces) #if !defined(OS_LINUX) with a TODO like ~expose
these tests on Linux when implemented

https://codereview.chromium.org/24710002/diff/13001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/24710002/diff/13001/build/common.gypi#newcode165
build/common.gypi:165: ['use_ash==1 or OS=="win" or OS=="mac" or ((OS=="linux"
or OS=="freebsd" or OS=="openbsd" or OS=="solaris") and use_aura==1)', {
The minimal change for this might be simply to change 'use_ash' into 'use_aura'
- I don't think that will break stuff..

[Matt Giuca, 2013-09-27 05:57:40]

I've added the minimal test class so it compiles, and disabled the failing tests
on Linux. Let's see ... I suspect it will now have compile issues on non-Aura
Linux.

https://codereview.chromium.org/24710002/diff/13001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/24710002/diff/13001/build/common.gypi#newcode165
build/common.gypi:165: ['use_ash==1 or OS=="win" or OS=="mac" or ((OS=="linux"
or OS=="freebsd" or OS=="openbsd" or OS=="solaris") and use_aura==1)', {
This doesn't seem quite right, as discussed: Aura doesn't necessarily imply App
List, even though it "just happens" to be the case right now.

Instead, I refactored desktop_linux as you suggested so I don't have to
enumerate all the Unix-like OSes.

[tapted, 2013-09-27 06:25:56]

nice! lgtm if there are just mechanical fixes.. It looks like linux_chromeos
still has a complaint

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

https://codereview.chromium.org/24710002/diff/30001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:89: #endif
nit: `  // !defined(OS_LINUX)`

https://codereview.chromium.org/24710002/diff/30001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:201: #endif  //
!defined(OS_CHROMEOS)
this should be updated too

https://codereview.chromium.org/24710002/diff/30001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:280:
IN_PROC_BROWSER_TEST_F(AppListControllerSearchResultsBrowserTest,
This guy might need to be hidden too

https://codereview.chromium.org/24710002/diff/30001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/24710002/diff/30001/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:1642: 'sources!': [
you might need to add app_list_service_test_api_linux.cc here if
filename_rules.gypi refuses to help you

[Matt Giuca, 2013-09-27 10:29:41]

Thanks for the tips, Trent!

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

https://codereview.chromium.org/24710002/diff/30001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:89: #endif
On 2013/09/27 06:25:56, tapted wrote:
> nit: `  // !defined(OS_LINUX)`

Done.

https://codereview.chromium.org/24710002/diff/30001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:201: #endif  //
!defined(OS_CHROMEOS)
On 2013/09/27 06:25:56, tapted wrote:
> this should be updated too

Done.

https://codereview.chromium.org/24710002/diff/30001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:280:
IN_PROC_BROWSER_TEST_F(AppListControllerSearchResultsBrowserTest,
On 2013/09/27 06:25:56, tapted wrote:
> This guy might need to be hidden too

Done.

https://codereview.chromium.org/24710002/diff/30001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

https://codereview.chromium.org/24710002/diff/30001/chrome/chrome_tests.gypi#...
chrome/chrome_tests.gypi:1642: 'sources!': [
On 2013/09/27 06:25:56, tapted wrote:
> you might need to add app_list_service_test_api_linux.cc here if
> filename_rules.gypi refuses to help you

Done.

(Oh damn, I just spent a good while figuring this out, before I read this
comment...)

[commit-bot: I haz the power, 2013-09-27 14:44:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/24710002/54001

[commit-bot: I haz the power, 2013-09-27 16:34:53]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-27 23:59:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/24710002/54001

[commit-bot: I haz the power, 2013-09-28 02:06:30]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-30 03:51:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/24710002/54001

[commit-bot: I haz the power, 2013-09-30 08:36:14]

Change committed as 225936