Refactor app_list_util.h into AppListService abstract base.

Refactor app_list_util.h into AppListService abstract base.

AppList usage is often guarded by an #ifdef ENABLE_APP_LIST, which adds 
fragility and reduces clarity. AppList is now enabled by default on 
chromeos, win, and mac. We also want to support multiple desktop types, 
e.g. for win+ash, where access to two app list controllers is required, 
depending on the current desktop type.

This change adds an AppListService abstract base, and refactors much of 
the current clutter around AppList calls.

BUG=138632, 138633, 174950
TEST=app_list_controller_browsertest.cc .

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

[tapted, 2013-02-11 04:04:20]

Added some notes and removed CCs since this is just for brainstorming.

CCs:

chromium-reviews@chromium.org, dbeam+watch-options@chromium.org, sadrul,
ben+watch@chromium.org, tfarina, sail+watch@chromium.org, Aaron Boodman,
pam+watch@chromium.org, chromium-apps-reviews@chromium.org

https://codereview.chromium.org/12207104/diff/20/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_install_ui_default.cc (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_install_ui_default.cc:111: // TODO(tapted):
Remove this ifdef.
actually, looking at the function name (OnAppLauncherEnabledCompleted), maybe it
doesn't matter that the notification isn't guarded.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:43:
ASSERT_FALSE(chrome::IsAppListVisible());
these all need to become, e.g.,

AppListService::Get()->IsAppListVisible()

but I didn't try that bit on OSX

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
File chrome/browser/ui/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:38: virtual void ShowAppList(Profile*
profile);
probably just 'Show()' is fine, same for all the other functions --
's/AppList//g'

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/ash/app_li...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/ash/app_li...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:84: #if
!defined(OS_WIN)
actually (is) defined(OS_CHROMEOS) is probably better (assuming this file is
actually part of OS_WIN + USE_ASH)

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/ash/app_li...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/ash/app_li...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:40: class
AppListServiceAsh : public AppListService {
this probably belongs in a separate file, along with the 'ash-on-chromeos'
version of AppListService::Get()

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/cocoa/app_...
File chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/cocoa/app_...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:23: class
AppListControllerCocoa : public AppListService {
should perhaps be in its own header, and use Singleton, for consistency

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (left):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:49: #include
"ui/views/bubble/bubble_border.h"
there might be a few more includes duped from the new header (and only used
there? -- not sure what `iwyu` says about this.)

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_controller_win.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:34: static
AppListControllerWin* GetInstance();
oops - forgot to implement this.

[benwells, 2013-02-11 04:12:32]

Overall approach looks nice. Some initial comments.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
File chrome/browser/ui/app_list_service.cc (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.cc:32: AppListService*
AppListService::GetDisabled() {
Oh, OK I see now. So this means calling code won't need to check
APP_LIST_ENABLED? This should be private, ne? Alternatively we could just bring
in the disabled app list via gyp, or totally via #ifdefs, instead of using a
GetDisabled function.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
File chrome/browser/ui/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:26: static AppListService* Get();
So this would become GetForLastActiveDesktop at some point? And Get take a
desktop as well?

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:27: static AppListService* GetDisabled();
What is GetDisabled for? Unless it's obvious and I'm just being obtuse, it
probably should be commented.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:44: virtual void DismissAppList();
Can we move the test only stuff into private, and let the tests get to it via
friending?

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_controller_win.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:39: bool
app_list_is_showing() const { return app_list_is_showing_; }
I don't think this should all be in the .h. It does not need to be visible
outside the .cc, does it?

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:161: int
pending_profile_loads_;
I think the profile loading stuff should move up to the base class - it will be
the same for all platforms / shells. Maybe.

[koz (OOO until 15th September), 2013-02-11 04:33:37]

Cool change!

So, just so we're straight, we have this idea that there is only ever one
AppListService, which is an interface provided globally. It represents the thing
that orchestrates the display and behaviour of app lists, so clients simply make
requests like "show the app list for this profile" and depending on what the
platform is it will do the right thing. It will also maintain invariants such as
"there should be at most one app list visible at any time".

For the ash+win_aura situation we can have a
AppListService::GetForDesktopType(DesktopType) which puts the burden on the
callers to implement any inter-AppListService behaviour (eg: hide the windows
app list if the ash one is displayed). Or alternatively we could just return a
"smart" AppListService which is aware of this duality and does the right thing.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_install_ui_default.cc (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_install_ui_default.cc:111: // TODO(tapted):
Remove this ifdef.
On 2013/02/11 04:04:20, tapted wrote:
> actually, looking at the function name (OnAppLauncherEnabledCompleted), maybe
it
> doesn't matter that the notification isn't guarded.

Sounds good to me.

[tapted, 2013-02-11 05:35:11]

On 2013/02/11 04:33:37, koz wrote:
> So, just so we're straight, we have this idea that there is only ever one
> AppListService, which is an interface provided globally. It represents the
thing
> that orchestrates the display and behaviour of app lists, so clients simply
make
> requests like "show the app list for this profile" and depending on what the
> platform is it will do the right thing. It will also maintain invariants such
as
> "there should be at most one app list visible at any time".
> 
> For the ash+win_aura situation we can have a
> AppListService::GetForDesktopType(DesktopType) which puts the burden on the
> callers to implement any inter-AppListService behaviour (eg: hide the windows
> app list if the ash one is displayed). Or alternatively we could just return a
> "smart" AppListService which is aware of this duality and does the right
thing.

Yeah - this is the missing piece. I think there might need to be one
AppListService per desktop type 'compiled in' (possibly created lazily).

This would leave some global state in app_list_controller_win.cc -- which would
become responsible for deactivating the "other" service (if we wanted that) when
switching between launchers on different desktop types.

This puts the burden on the platform (i.e. win/mac/chromeos), and each platform
can have an algorithm for deciding what to do. I'm worried that a new/smart
AppListService sub-type for win+aura is a possibly bad fit, since it suggests we
have a new type for each combination we might need to support, which
(theoretically at least) doesn't scale as well as one-per-platform and some
global state.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
File chrome/browser/ui/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:26: static AppListService* Get();
On 2013/02/11 04:12:32, benwells wrote:
> So this would become GetForLastActiveDesktop at some point? And Get take a
> desktop as well?

yep!

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:27: static AppListService* GetDisabled();
On 2013/02/11 04:12:32, benwells wrote:
> What is GetDisabled for? Unless it's obvious and I'm just being obtuse, it
> probably should be commented.

It's the Singleton for AppListService(). i.e. the "dummy" AppListService, which
does nothing.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:44: virtual void DismissAppList();
On 2013/02/11 04:12:32, benwells wrote:
> Can we move the test only stuff into private, and let the tests get to it via
> friending?

"maybe"? Maintaing friends could get fiddly/fragile if we reach a situation
where we need lots of test fixtures. Maybe a public interface exposed here, and
a app_list_service_internal that exposes the interface that actually gets
implemented (which includes tests?)?

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_controller_win.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:39: bool
app_list_is_showing() const { return app_list_is_showing_; }
On 2013/02/11 04:12:32, benwells wrote:
> I don't think this should all be in the .h. It does not need to be visible
> outside the .cc, does it?

the definition or the declaration?

Leaving the definition here (i.e. inline) was largely just me being lazy ;-).
The other motivation was organisation -- (i.e. app_list_controller_win.cc is
large, let's organise it). Downside: more symbols for linking.

We could cut this down the exposed interface to a single function -- just
'...Win::GetInstance()' as far as the AppListService::Get() function is
concerned.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:161: int
pending_profile_loads_;
On 2013/02/11 04:12:32, benwells wrote:
> I think the profile loading stuff should move up to the base class - it will
be
> the same for all platforms / shells. Maybe.

Yeah, OSX will need bits of it too.

I've also toyed with the idea of an 'AppListServiceDesktop' that sits in between
the base and the non-ChromeOS versions.

[benwells, 2013-02-11 05:52:36]

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
File chrome/browser/ui/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:27: static AppListService* GetDisabled();
On 2013/02/11 05:35:11, tapted wrote:
> On 2013/02/11 04:12:32, benwells wrote:
> > What is GetDisabled for? Unless it's obvious and I'm just being obtuse, it
> > probably should be commented.
> 
> It's the Singleton for AppListService(). i.e. the "dummy" AppListService,
which
> does nothing.

I think we should organize the code so it isn't on the public interface.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:38: virtual void ShowAppList(Profile*
profile);
On 2013/02/11 04:04:20, tapted wrote:
> probably just 'Show()' is fine, same for all the other functions --
> 's/AppList//g'

sgtm

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:44: virtual void DismissAppList();
On 2013/02/11 05:35:11, tapted wrote:
> On 2013/02/11 04:12:32, benwells wrote:
> > Can we move the test only stuff into private, and let the tests get to it
via
> > friending?
> 
> "maybe"? Maintaing friends could get fiddly/fragile if we reach a situation
> where we need lots of test fixtures. Maybe a public interface exposed here,
and
> a app_list_service_internal that exposes the interface that actually gets
> implemented (which includes tests?)?

Maybe if we just keep all the testing only functions together, with a comment
that they are just for testing. I'd just like to see it made clear somehow.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (left):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:49: #include
"ui/views/bubble/bubble_border.h"
On 2013/02/11 04:04:20, tapted wrote:
> there might be a few more includes duped from the new header (and only used
> there? -- not sure what `iwyu` says about this.)

Any includes in x.h can be left out of x.cc.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_controller_win.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:39: bool
app_list_is_showing() const { return app_list_is_showing_; }
On 2013/02/11 05:35:11, tapted wrote:
> On 2013/02/11 04:12:32, benwells wrote:
> > I don't think this should all be in the .h. It does not need to be visible
> > outside the .cc, does it?
> 
> the definition or the declaration?
> 
> Leaving the definition here (i.e. inline) was largely just me being lazy ;-).
> The other motivation was organisation -- (i.e. app_list_controller_win.cc is
> large, let's organise it). Downside: more symbols for linking.
> 
> We could cut this down the exposed interface to a single function -- just
> '...Win::GetInstance()' as far as the AppListService::Get() function is
> concerned.

Sorry, I meant the whole interface. Unless I'm mistaken this header is not
included by anything except the matching .cc file.

I don't think splitting part of the .cc into a .h gains anything. If you want to
organize the code a bit less monolithically, we should split out the controller
delegate into a separate file (which might require this .h to be split out as
well, not sure).

[tapted, 2013-02-18 07:05:39]

Windows is in!

still on the agenda: fix tests, chromeos (and cocoa needs a revisit too)

https://codereview.chromium.org/12207104/diff/20/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_install_ui_default.cc (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_install_ui_default.cc:111: // TODO(tapted):
Remove this ifdef.
On 2013/02/11 04:33:38, koz wrote:
> On 2013/02/11 04:04:20, tapted wrote:
> > actually, looking at the function name (OnAppLauncherEnabledCompleted),
maybe
> it
> > doesn't matter that the notification isn't guarded.
> 
> Sounds good to me.

Done.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
File chrome/browser/ui/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:27: static AppListService* GetDisabled();
On 2013/02/11 05:52:36, benwells wrote:
> On 2013/02/11 05:35:11, tapted wrote:
> > On 2013/02/11 04:12:32, benwells wrote:
> > > What is GetDisabled for? Unless it's obvious and I'm just being obtuse, it
> > > probably should be commented.
> > 
> > It's the Singleton for AppListService(). i.e. the "dummy" AppListService,
> which
> > does nothing.
> 
> I think we should organize the code so it isn't on the public interface.

Done.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/ash/app_li...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/ash/app_li...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:84: #if
!defined(OS_WIN)
On 2013/02/11 04:04:20, tapted wrote:
> actually (is) defined(OS_CHROMEOS) is probably better (assuming this file is
> actually part of OS_WIN + USE_ASH)

(deprecated comment)

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_controller_win.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:34: static
AppListControllerWin* GetInstance();
On 2013/02/11 04:04:20, tapted wrote:
> oops - forgot to implement this.

Done.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:39: bool
app_list_is_showing() const { return app_list_is_showing_; }
On 2013/02/11 05:52:36, benwells wrote:
> On 2013/02/11 05:35:11, tapted wrote:
> > On 2013/02/11 04:12:32, benwells wrote:
> > > I don't think this should all be in the .h. It does not need to be visible
> > > outside the .cc, does it?
> > 
> > the definition or the declaration?
> > 
> > Leaving the definition here (i.e. inline) was largely just me being lazy
;-).
> > The other motivation was organisation -- (i.e. app_list_controller_win.cc is
> > large, let's organise it). Downside: more symbols for linking.
> > 
> > We could cut this down the exposed interface to a single function -- just
> > '...Win::GetInstance()' as far as the AppListService::Get() function is
> > concerned.
> 
> Sorry, I meant the whole interface. Unless I'm mistaken this header is not
> included by anything except the matching .cc file.
> 
> I don't think splitting part of the .cc into a .h gains anything. If you want
to
> organize the code a bit less monolithically, we should split out the
controller
> delegate into a separate file (which might require this .h to be split out as
> well, not sure).

Done.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:161: int
pending_profile_loads_;
On 2013/02/11 05:35:11, tapted wrote:
> On 2013/02/11 04:12:32, benwells wrote:
> > I think the profile loading stuff should move up to the base class - it will
> be
> > the same for all platforms / shells. Maybe.
> 
> Yeah, OSX will need bits of it too.
> 
> I've also toyed with the idea of an 'AppListServiceDesktop' that sits in
between
> the base and the non-ChromeOS versions.

(partly done)

https://codereview.chromium.org/12207104/diff/25002/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list_service.cc (right):

https://codereview.chromium.org/12207104/diff/25002/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list_service.cc:26: #include
"chrome/browser/ui/views/app_list/app_list_controller_win.h"
This #include breaks DEPS! Onoz. (uploaded with --bypass-hooks)

[tapted, 2013-02-18 07:52:43]

Cocoa is back. Not sure what to do with that DEPS problem on c/b/ui/views --
maybe we need to move it all to src/apps/app_list first.

There's also two flavours in the CL currently, partly to minimise diffs while we
pick the design. But also because I'm not sure which is best.

The cocoa flavour uses an 'AppListServiceCocoa', but its GetInstance is being
implemented in app_list_controller_cocoa.cc, since it uses the anonymous
namespace there.

The windows flavour uses 'AppListControllerWin' (inheriting from
AppListService').

Maybe going down this route, we should rename the
app_list_controller_{cocoa,win}.cc to app_list_service_foo.cc, then introduce a
new app_list_service_ash.cc (since app_list_controller_ash.h is doing
double-duty right now).


On 2013/02/18 07:05:39, tapted wrote:
>
https://codereview.chromium.org/12207104/diff/25002/chrome/browser/ui/app_lis...
> File chrome/browser/ui/app_list_service.cc (right):
> 
>
https://codereview.chromium.org/12207104/diff/25002/chrome/browser/ui/app_lis...
> chrome/browser/ui/app_list_service.cc:26: #include
> "chrome/browser/ui/views/app_list/app_list_controller_win.h"
> This #include breaks DEPS! Onoz. (uploaded with --bypass-hooks)

[benwells, 2013-02-18 08:31:30]

Let's talk tomorrow about how not to break DEPS. I have a plan :D

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list_service.h:5: #ifndef
CHROME_BROWSER_UI_APP_LIST_SERVICE_H_
Why is this not in c/b/ui/app_list?

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list_service.h:48: virtual Profile*
GetCurrentAppListProfile();
Can we move things just for testing so they are together and have a comment
indicating they are just for testing?

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.cc (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_controller_ash.cc:75: // TODO(ananta):
Handle profile changes correctly when !defined(OS_CHROMEOS).
Might as well kill this TODO (if it's easy): we should just ignore the Show if
the profile does not match.

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:13: template <typename
T> struct DefaultSingletonTraits;
Why is this here?

[tapted, 2013-02-18 10:27:55]

On 2013/02/18 08:31:30, benwells wrote:
> Let's talk tomorrow about how not to break DEPS. I have a plan :D

Oh man - leaving us in suspense.. :P

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list_service.h:5: #ifndef
CHROME_BROWSER_UI_APP_LIST_SERVICE_H_
On 2013/02/18 08:31:30, benwells wrote:
> Why is this not in c/b/ui/app_list?

The app_list folder is bound to ENABLE_APP_LIST - there might be some ways to
make an exception for this file, so we can use it when ENABLE_APP_LIST is false.

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list_service.h:48: virtual Profile*
GetCurrentAppListProfile();
On 2013/02/18 08:31:30, benwells wrote:
> Can we move things just for testing so they are together and have a comment
> indicating they are just for testing?

Shouldn't be an issue.

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:13: template <typename
T> struct DefaultSingletonTraits;
On 2013/02/18 08:31:30, benwells wrote:
> Why is this here?

It's needed for the friend declaration in AppListServiceAsh (boilerplate taken
from singleton.h).

[benwells, 2013-02-18 10:36:17]

Just too lazy to type it all up...

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:13: template <typename
T> struct DefaultSingletonTraits;
On 2013/02/18 10:27:55, tapted wrote:
> On 2013/02/18 08:31:30, benwells wrote:
> > Why is this here?
> 
> It's needed for the friend declaration in AppListServiceAsh (boilerplate taken
> from singleton.h).

Oh, of course :)

[koz (OOO until 15th September), 2013-02-19 00:08:09]

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list_service.cc (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list_service.cc:58: return
AppListControllerAsh::GetInstance();
The ash branch only returns if the conditional is hit. If the conditional isn't
hit it needs to return AppListControllerWin::GetInstance() - the aforementioned
switcheroo?

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/extension_install_ui_default.cc (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_install_ui_default.cc:233:
chrome::ShowAppList(browser->profile());
I think you'll need to update this one, too.

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:160: 
nit: no newline

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.h (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.h:10: class
AppListControllerWin : public AppListService {
So this is called Controller rather than Service to ease the renaming burden?
Add a TODO?

[tapted, 2013-02-19 03:15:32]

This is the version with the "selector" and some gypdeffery (which I still need
to check works.. trybots running now).

Maybe time to upload as a new CL, too. The impacted files are nearing a final
list, so let's spam some watchers.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:43:
ASSERT_FALSE(chrome::IsAppListVisible());
On 2013/02/11 04:04:20, tapted wrote:
> these all need to become, e.g.,
> 
> AppListService::Get()->IsAppListVisible()
> 
> but I didn't try that bit on OSX

Done.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
File chrome/browser/ui/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:38: virtual void ShowAppList(Profile*
profile);
On 2013/02/11 05:52:36, benwells wrote:
> On 2013/02/11 04:04:20, tapted wrote:
> > probably just 'Show()' is fine, same for all the other functions --
> > 's/AppList//g'
> 
> sgtm

I've changed my mind about this... since the usage sounds like we're showing
"the service" which doesn't make sense. Some of the others might make sense to
change though, like the static functions, WDYT?

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/app_list_s...
chrome/browser/ui/app_list_service.h:44: virtual void DismissAppList();
On 2013/02/11 05:52:36, benwells wrote:
> On 2013/02/11 05:35:11, tapted wrote:
> > On 2013/02/11 04:12:32, benwells wrote:
> > > Can we move the test only stuff into private, and let the tests get to it
> via
> > > friending?
> > 
> > "maybe"? Maintaing friends could get fiddly/fragile if we reach a situation
> > where we need lots of test fixtures. Maybe a public interface exposed here,
> and
> > a app_list_service_internal that exposes the interface that actually gets
> > implemented (which includes tests?)?
> 
> Maybe if we just keep all the testing only functions together, with a comment
> that they are just for testing. I'd just like to see it made clear somehow.

IsAppListVisible now finds itself in non-test code for ash. I think the only
test-only function is now GetCurrentAppListProfile, but there is a TODO that
might make it relevant for non-test code..

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/ash/app_li...
File chrome/browser/ui/ash/app_list/app_list_controller_ash.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/ash/app_li...
chrome/browser/ui/ash/app_list/app_list_controller_ash.h:40: class
AppListServiceAsh : public AppListService {
On 2013/02/11 04:04:20, tapted wrote:
> this probably belongs in a separate file, along with the 'ash-on-chromeos'
> version of AppListService::Get()

Done.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/cocoa/app_...
File chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/cocoa/app_...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:23: class
AppListControllerCocoa : public AppListService {
On 2013/02/11 04:04:20, tapted wrote:
> should perhaps be in its own header, and use Singleton, for consistency

Done.

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_controller_win.h (right):

https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_controller_win.h:161: int
pending_profile_loads_;
On 2013/02/18 07:05:39, tapted wrote:
> On 2013/02/11 05:35:11, tapted wrote:
> > On 2013/02/11 04:12:32, benwells wrote:
> > > I think the profile loading stuff should move up to the base class - it
will
> > be
> > > the same for all platforms / shells. Maybe.
> > 
> > Yeah, OSX will need bits of it too.
> > 
> > I've also toyed with the idea of an 'AppListServiceDesktop' that sits in
> between
> > the base and the non-ChromeOS versions.
> 
> (partly done)

I might do the rest in a follow-up once it's clear which "bits" are needed.

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list_service.cc (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list_service.cc:58: return
AppListControllerAsh::GetInstance();
On 2013/02/19 00:08:09, koz wrote:
> The ash branch only returns if the conditional is hit. If the conditional
isn't
> hit it needs to return AppListControllerWin::GetInstance() - the
aforementioned
> switcheroo?

Done.

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list_service.h:5: #ifndef
CHROME_BROWSER_UI_APP_LIST_SERVICE_H_
On 2013/02/18 10:27:55, tapted wrote:
> On 2013/02/18 08:31:30, benwells wrote:
> > Why is this not in c/b/ui/app_list?
> 
> The app_list folder is bound to ENABLE_APP_LIST - there might be some ways to
> make an exception for this file, so we can use it when ENABLE_APP_LIST is
false.

Done (I think... need to launch some builders).

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list_service.h:48: virtual Profile*
GetCurrentAppListProfile();
On 2013/02/18 10:27:55, tapted wrote:
> On 2013/02/18 08:31:30, benwells wrote:
> > Can we move things just for testing so they are together and have a comment
> > indicating they are just for testing?
> 
> Shouldn't be an issue.

\todo

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/extensi...
File chrome/browser/ui/extensions/extension_install_ui_default.cc (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/extensi...
chrome/browser/ui/extensions/extension_install_ui_default.cc:233:
chrome::ShowAppList(browser->profile());
On 2013/02/19 00:08:09, koz wrote:
> I think you'll need to update this one, too.

Done.

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:160: 
On 2013/02/19 00:08:09, koz wrote:
> nit: no newline

Done.

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.h (right):

https://codereview.chromium.org/12207104/diff/16019/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.h:10: class
AppListControllerWin : public AppListService {
On 2013/02/19 00:08:09, koz wrote:
> So this is called Controller rather than Service to ease the renaming burden?
> Add a TODO?

This was to "allow" the implementation of GetInstance to be in
app_list_controller.cc (while still having the class name match the header
filename). I'll see what it looks like after adding the selector units.

[tapted, 2013-02-19 03:35:06]

patch set 10 might fix some build problems.

https://codereview.chromium.org/12207104/diff/20006/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_ash.h (right):

https://codereview.chromium.org/12207104/diff/20006/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:10: class AppListServiceAsh :
public AppListService {
So tempting just to declare these in app_list_selector.cc as single, function
"forward" decs..... i.e.

app_list_service_selector.cc:
namespace chrome {
AppListService* GetAppListServiceAsh();
AppListService* GetAppListServiceCocoa();
AppListService* GetAppListServiceWin();
}

app_list_service_ash.cc:
namespace chrome {
AppListService* GetAppListServiceAsh() {
  return AppListServiceAsh::GetInstance();
}

//etc.

Maybe that's too evil..

https://codereview.chromium.org/12207104/diff/20006/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_selector.cc (right):

https://codereview.chromium.org/12207104/diff/20006/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_selector.cc:8: #if defined(USE_ASH)
so.... should this be split into multiple files?

https://codereview.chromium.org/12207104/diff/20006/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/12207104/diff/20006/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:108:
'browser/ui/app_list/app_list_controller_delegate.cc',
> need to fix sort order.
Done.

https://codereview.chromium.org/12207104/diff/20006/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:2834: ['include',
'^browser/ui/app_list/app_list_service.h'],
not sure if this will work yet..... <go linux, go!>

[benwells, 2013-02-19 03:49:42]

This is looking really good. It will get simpler after
https://codereview.chromium.org/12298015/ lands.

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.cc (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.cc:70: void
AppListService::OnProfileAdded(const base::FilePath& profilePath) {}
A few blank lines in here wouldn't go astray...

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:36: virtual void Init(Profile*
initial_profile);
Hmmm ... should this be static and go to all services?

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_ash.h (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:13: protected:
blank lines here too please, before protected: and private:

https://codereview.chromium.org/12207104/diff/22024/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:107:
'browser/ui/app_list/app_list_service_disabled.cc',
You don't really need the _disabled.cc here, which will mean you don't need to
exclude it later. But maybe that is more confusing.

[koz (OOO until 15th September), 2013-02-19 03:55:14]

https://codereview.chromium.org/12207104/diff/20006/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_selector.cc (right):

https://codereview.chromium.org/12207104/diff/20006/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_selector.cc:8: #if defined(USE_ASH)
On 2013/02/19 03:35:06, tapted wrote:
> so.... should this be split into multiple files?

I think this looks good!

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_win.h (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_win.h:12: static AppListServiceWin*
GetInstance();
nit: newlines before protected: and private:.

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:41: return
ash::Shell::GetInstance()->GetAppListWindow() != NULL;
I think you want ash::Shell::GetInstance()->GetAppListTargetVisibility() here,
which I think should also make the test work for cros (as per your TODO there).

[tapted, 2013-02-19 04:25:21]

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.cc (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.cc:70: void
AppListService::OnProfileAdded(const base::FilePath& profilePath) {}
On 2013/02/19 03:49:42, benwells wrote:
> A few blank lines in here wouldn't go astray...

Done.

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:36: virtual void Init(Profile*
initial_profile);
On 2013/02/19 03:49:42, benwells wrote:
> Hmmm ... should this be static and go to all services?

I'll make it protected and add static function.

'InitAll' ? Done.

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_ash.h (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:13: protected:
On 2013/02/19 03:49:42, benwells wrote:
> blank lines here too please, before protected: and private:

Done. x6

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_win.h (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_win.h:12: static AppListServiceWin*
GetInstance();
On 2013/02/19 03:55:14, koz wrote:
> nit: newlines before protected: and private:.

Done.

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:41: return
ash::Shell::GetInstance()->GetAppListWindow() != NULL;
On 2013/02/19 03:55:14, koz wrote:
> I think you want ash::Shell::GetInstance()->GetAppListTargetVisibility() here,
> which I think should also make the test work for cros (as per your TODO
there).

Oooh - nice. Done.

https://codereview.chromium.org/12207104/diff/22024/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/12207104/diff/22024/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:66: # All .cc, .h, .m, and .mm files under
browser/ui except for:
buut

https://codereview.chromium.org/12207104/diff/22024/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:107:
'browser/ui/app_list/app_list_service_disabled.cc',
On 2013/02/19 03:49:42, benwells wrote:
> You don't really need the _disabled.cc here, which will mean you don't need to
> exclude it later. But maybe that is more confusing.

There's a "buut" on line 66 ;)

[benwells, 2013-02-19 07:34:28]

Comment for AppListServiceAshImpl also applies for win and cocoa.

Also, I noticed we have app_list_service_ash.cc and
app_list_controller_delegate_ash.cc. I guess the latter should be
app_list_shell_delegate_ash.cc, but that can come later with a corresponding
change for win.

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:163:
AppListService::RegisterAppListPrefs(registry);
nit: This function should just become RegisterPrefs.

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

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:40: #if
defined(OS_WIN) || defined(OS_CHROMEOS)
Something to think about ... how do we test ash launcher on windows? Hehehe

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:45: const base::FilePath&
profile_file_path);
nit: fits on previous line.

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:13: class
AppListServiceAshImpl : public AppListServiceAsh {
Why have the impl class? It would be more transparent to have the ShowAppList
etc. on the AppListServiceAsh class in app_list_service_ash.h.

[tfarina, 2013-02-20 03:29:44]

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_cocoa.h (right):

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_cocoa.h:5: #ifndef
CHROME_BROWSER_UI_APP_LIST_APP_LIST_SERVICE_COCOA_H_
what is this file doing here?

Can you move it to c/b/ui/cocoa/app_list/?

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_cocoa.h:15: AppListServiceCocoa() {}
is this protected to disallow manual instantiation? can you make it private?

[tfarina, 2013-02-20 03:33:01]

This is referencing crbug.com/174950. Is it the right bug for this change? It's
already closed/fixed.

[tapted, 2013-02-20 04:15:06]

Still need to poke around on chromeos to get the test working there.

On 2013/02/20 03:33:01, tfarina wrote:
> This is referencing crbug.com/174950. Is it the right bug for this change?
It's
> already closed/fixed.

That was the motivation, and symptom of this piece being missing. crbug/174950
has a workaround fix, and this CL tidies things up a bit. I've added some
feature bugs.

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:163:
AppListService::RegisterAppListPrefs(registry);
On 2013/02/19 07:34:28, benwells wrote:
> nit: This function should just become RegisterPrefs.

Done.

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

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:40: #if
defined(OS_WIN) || defined(OS_CHROMEOS)
On 2013/02/19 07:34:28, benwells wrote:
> Something to think about ... how do we test ash launcher on windows? Hehehe

Indeed... I had started poking around at other tests that might do something
like this. The process seems to be something like, in SetUp:

    command_line->AppendSwitch(switches::kOpenAsh);

Then.. "stuff".

However, this test still isn't happy on chromeos yet. (IsAppListVisible still
behaving differently).  maybe 

  ui::LayerAnimator::set_disable_animations_for_test(true);

is the ticket... Need to do more poking around on ash.

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:45: const base::FilePath&
profile_file_path);
On 2013/02/19 07:34:28, benwells wrote:
> nit: fits on previous line.

Done.

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_cocoa.h (right):

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_cocoa.h:5: #ifndef
CHROME_BROWSER_UI_APP_LIST_APP_LIST_SERVICE_COCOA_H_
On 2013/02/20 03:29:44, tfarina wrote:
> what is this file doing here?
> 
> Can you move it to c/b/ui/cocoa/app_list/?

This one, technically yes. But not so for app_list_service_win.h. All three
{ash,cocoa,views} are included from
c/b/ui/app_list/app_list_service_selector.cc, which can not include things from
c/b/ui/views due to DEPS.

This arrangement is not ideal, but is a link in the chain to a bigger refactor.

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_cocoa.h:15: AppListServiceCocoa() {}
On 2013/02/20 03:29:44, tfarina wrote:
> is this protected to disallow manual instantiation? can you make it private?

AppListControllerCocoa inherits from this, so needs a non-private constructor.

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:9: #include
"chrome/browser/ui/app_list/app_list_service_ash.h"
oops - just noticed this is a dupe - removed.

https://codereview.chromium.org/12207104/diff/17043/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:13: class
AppListServiceAshImpl : public AppListServiceAsh {
On 2013/02/19 07:34:28, benwells wrote:
> Why have the impl class? It would be more transparent to have the ShowAppList
> etc. on the AppListServiceAsh class in app_list_service_ash.h.

I went this route to minimise the cross-directory symbols (and also inspired by
your earlier comment:
https://codereview.chromium.org/12207104/diff/20/chrome/browser/ui/views/app_...
;-).

That is, in this form, there is just a static function with its declaration in
c/b/ui/app_list and the definition in c/b/ui/{ash,cocoa,views}/app_list . I see
merit in at least moving the overrides to the headers though.. Done for ash, but
re:

On 2013/02/19 07:34:28, benwells wrote:
> Comment for AppListServiceAshImpl also applies for win and cocoa.

not done for win & cocoa. For those, it wouldn't be enough just to push the
overrides up into app_list_service_foo.h -- implementing them at that level
wouldn't have access to necessary members in the leaf class. E.g. for cocoa,
we'd need the scoped_nsobject and an objective C @class forward declaration in
the header, requiring app_list_service_selector to be compiled as Objective C++.

[tapted, 2013-02-20 12:03:42]

patchset 14 is an alternative that takes the class static member functions out
into regular functions, since the classes weren't adding much. WDYT?

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

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:84: // static
hey tapted: remove this line. Thanks. - tapted.

[benwells, 2013-02-21 04:34:36]

Am liking it, very close now.

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.cc (right):

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.cc:15: base::FilePath
AppListService::GetAppListProfilePath(
I'm kinda confused by the semantics of this for the windows case, where there is
an ash and a windows app list service. Or just for the ash one actually.

Should this be an instance method, with this being the base implementation and
ash providing something based on the current profile in use?

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_ash.h (right):

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:12: AppListService*
GetAppListServiceAsh();
I think this should be GetAppListServiceChromeOS, in
app_list_service_chromeos.h.

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_cocoa.h (right):

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_cocoa.h:12: AppListService*
GetAppListServiceCocoa();
I think this should be GetAppListServiceMac, in app_list_service_mac.h.

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_disabled.cc (right):

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_disabled.cc:20:
AppListServiceDisabled* GetDisabled() {
For consistency, I think this should be GetAppListServiceDisabled, declared in
app_list_service_disabled.h, with the Get call in app_list_service.cc guarded
with #ifs.

[tfarina, 2013-02-21 22:59:09]

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:14: class PrefRegistrySimple;
sort

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:15: class Profile;
duplicated

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:28: static AppListService* Get();
GetInstance?

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:65: virtual void
OnProfileAdded(const base::FilePath& profilePath) OVERRIDE;
Can you make those private?

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:69: const string16& profile_name)
OVERRIDE;
add compiler_specific.h for OVERRIDE

[tfarina, 2013-02-21 23:04:44]

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_ash.h (right):

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:12: AppListService*
GetAppListServiceAsh();
Lets not do this way.

It's better to have one function in a cross-platform file:

AppListService* GetAppListService();

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:14: }
// namespace chrome

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:9: #include
"chrome/browser/ui/app_list/app_list_service.h"
This gets really confusing, you are inheriting from AppListService, but the main
file is chrome/browser/ui/app_list/app_list_service_ash.h.

There are some examples of how to this in a more cross-platform way, but I don't
recall from head right now :/

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:16: return
Singleton<AppListServiceAsh>.get();
is this what we want? Why we can't make someone own AppListService instead?

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:19: // AppListService
overrides:
can be private?

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:27: friend struct
DefaultSingletonTraits<AppListServiceAsh>;
this comes first, it should be the first thing in the private section.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:57: }
// namespace chrome

[tapted, 2013-02-25 07:09:21]

Addressed comments, and switched so that "Leaky" singletons are used, to match
the old lazy_instance versions.

I'll follow up in a sec with some annotations.

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.cc (right):

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.cc:15: base::FilePath
AppListService::GetAppListProfilePath(
On 2013/02/21 04:34:36, benwells wrote:
> I'm kinda confused by the semantics of this for the windows case, where there
is
> an ash and a windows app list service. Or just for the ash one actually.
> 
> Should this be an instance method, with this being the base implementation and
> ash providing something based on the current profile in use?

Done.

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_ash.h (right):

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:12: AppListService*
GetAppListServiceAsh();
On 2013/02/21 04:34:36, benwells wrote:
> I think this should be GetAppListServiceChromeOS, in
> app_list_service_chromeos.h.

This will be called on Windows as well, to obtain the service that controls the
app list in the ash shell. So I think foo..Ash makes sense?

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_cocoa.h (right):

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_cocoa.h:12: AppListService*
GetAppListServiceCocoa();
On 2013/02/21 04:34:36, benwells wrote:
> I think this should be GetAppListServiceMac, in app_list_service_mac.h.

... This change makes sense though. Done.

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_disabled.cc (right):

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_disabled.cc:20:
AppListServiceDisabled* GetDisabled() {
On 2013/02/21 04:34:36, benwells wrote:
> For consistency, I think this should be GetAppListServiceDisabled, declared in
> app_list_service_disabled.h, with the Get call in app_list_service.cc guarded
> with #ifs.

Done. But have a look - maybe it would be nicer in app_list_service_selector.cc
rather than app_list_service.cc? .. not sure if they gypdeffery will like that
approach though (in which case pretend I never suggested it :p).

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

https://codereview.chromium.org/12207104/diff/32005/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:84: // static
On 2013/02/20 12:03:43, tapted wrote:
> hey tapted: remove this line. Thanks. - tapted.

Done. Thanks, tapted.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:14: class PrefRegistrySimple;
On 2013/02/21 22:59:09, tfarina wrote:
> sort

Done.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:15: class Profile;
On 2013/02/21 22:59:09, tfarina wrote:
> duplicated

Done.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:28: static AppListService* Get();
On 2013/02/21 22:59:09, tfarina wrote:
> GetInstance?

AppListService isn't a singleton at this level. And, e.g., we may find cause to
add a 'GetForDeskoptType' static function as well, which takes an argument to
select a particular desktop type.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:65: virtual void
OnProfileAdded(const base::FilePath& profilePath) OVERRIDE;
On 2013/02/21 22:59:09, tfarina wrote:
> Can you make those private?

These could potentially still be overridden by a leaf class.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service.h:69: const string16& profile_name)
OVERRIDE;
On 2013/02/21 22:59:09, tfarina wrote:
> add compiler_specific.h for OVERRIDE

Done.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_ash.h (right):

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:12: AppListService*
GetAppListServiceAsh();
On 2013/02/21 23:04:44, tfarina wrote:
> Lets not do this way.
> 
> It's better to have one function in a cross-platform file:
> 
> AppListService* GetAppListService();
> 

There's not necessarily a 1:1 mapping from a platform to a service. On windows
with aura/ash, we need to provide two -- one "win" and one "ash" service, so
they must have different names.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:14: }
On 2013/02/21 23:04:44, tfarina wrote:
> // namespace chrome

Done.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:9: #include
"chrome/browser/ui/app_list/app_list_service.h"
On 2013/02/21 23:04:44, tfarina wrote:
> This gets really confusing, you are inheriting from AppListService, but the
main
> file is chrome/browser/ui/app_list/app_list_service_ash.h.
> 
> There are some examples of how to this in a more cross-platform way, but I
don't
> recall from head right now :/

This approach is mainly to keep DEPS happy. I think a follow-up will move a lot
of this to /apps, so the organisation will be a bit neater.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:16: return
Singleton<AppListServiceAsh>.get();
On 2013/02/21 23:04:44, tfarina wrote:
> is this what we want? Why we can't make someone own AppListService instead?

I don't think it makes sense for AppListService to have an owner ("yet").

It is a service like PrefService, MetricsService, etc. Those are owned by
g_browser_process, but we don't want to couple ourselves to the browser. We also
can't attach it to a profile, since it needs ways to switch profiles. (unlike,
e.g., ExtensionService+System, which is a per-profile).

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:19: // AppListService
overrides:
On 2013/02/21 23:04:44, tfarina wrote:
> can be private?

Done.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:27: friend struct
DefaultSingletonTraits<AppListServiceAsh>;
On 2013/02/21 23:04:44, tfarina wrote:
> this comes first, it should be the first thing in the private section.

Hmm - the guideline is pretty ambiguous about these, and the pattern above is a
better match for the boilerplate in base/memory/singleton.h. But I'm not fussed
- I think 'friends first' wins out in a git-grep-fight. Done.

https://codereview.chromium.org/12207104/diff/45050/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:57: }
On 2013/02/21 23:04:44, tfarina wrote:
> // namespace chrome

Done.

[tapted, 2013-02-25 07:18:05]

(some notes for patchset 17)

https://codereview.chromium.org/12207104/diff/37055/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12207104/diff/37055/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:346: return
AppListService::Get()->GetAppListProfilePath(user_data_dir);
note: now non-static

https://codereview.chromium.org/12207104/diff/37055/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_ash.h (right):

https://codereview.chromium.org/12207104/diff/37055/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_ash.h:12: AppListService*
GetAppListServiceAsh();
note: still "Ash" (see prev. publish msg)

https://codereview.chromium.org/12207104/diff/37055/chrome/browser/ui/ash/app...
File chrome/browser/ui/ash/app_list/app_list_service_ash.cc (right):

https://codereview.chromium.org/12207104/diff/37055/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:40: return
ChromeLauncherController::instance()->profile()->GetPath();
this is new!  I think it's right. It's used, e.g., in app_list_controller_ash.cc
for, e.g., 

ChromeLauncherController::instance()->ActivateApp

https://codereview.chromium.org/12207104/diff/37055/chrome/browser/ui/ash/app...
chrome/browser/ui/ash/app_list/app_list_service_ash.cc:46: // TODO(ananta):
Handle profile changes correctly when !defined(OS_CHROMEOS).
After encountering chrome_launcher_controller.h, I think this will be a big
change..

[tapted, 2013-02-26 00:26:56]

Pretty sure this will conflict with http://crrev.com/12298015 so I predict
another rebase.

On 2013/02/25 07:09:21, tapted wrote:
> On 2013/02/21 04:34:36, benwells wrote:
> > For consistency, I think this should be GetAppListServiceDisabled, declared
in
> > app_list_service_disabled.h, with the Get call in app_list_service.cc
guarded
> > with #ifs.
> 
> Done. But have a look - maybe it would be nicer in
app_list_service_selector.cc
> rather than app_list_service.cc? .. not sure if they gypdeffery will like that
> approach though (in which case pretend I never suggested it :p).

Moved to app_list_service_selector.cc in patchset 19 per our conversation.

[benwells, 2013-02-26 01:18:25]

lgtm with nits and the disabled selector thing fixed.

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_selector.cc (right):

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_selector.cc:25: AppListService*
AppListService::Get() {
As discussed disabled should be returned if !defined(ENABLE_APP_LIST)

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

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:26: class
AppListControllerCocoa : public AppListService {
Nit: Can you add a TODO to rename this class to AppListServiceCocoa, or just
rename it?

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:136: class
AppListController : public AppListService {
Can you also add a TODO here to rename this to AppListServiceWin, or just do it
now?

[tapted, 2013-02-26 03:04:34]

+sky for OWNERS, please take a look (mostly removing #ifdefs)
 - c/b/{chrome_browser_main.cc, startup/*}
 - c/b/prefs
 - c/b/ui/ash
 - c/b/ui for pedestrian change to manage_profile_handler.cc
 - c/chrome_browser_ui.gypi

+sail for cocoa OWNERS:
 - c/b/ui/cocoa/app_list

(this is a refactor partly motivated by win+aura/ash which broke some old
assumptions)

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_service_selector.cc (right):

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_service_selector.cc:25: AppListService*
AppListService::Get() {
On 2013/02/26 01:18:25, benwells wrote:
> As discussed disabled should be returned if !defined(ENABLE_APP_LIST)

Done. There was a nested-#ifdef version in patch set 20, but it felt too
horrible, so I've un-nested in patch set 21.

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

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:26: class
AppListControllerCocoa : public AppListService {
On 2013/02/26 01:18:25, benwells wrote:
> Nit: Can you add a TODO to rename this class to AppListServiceCocoa, or just
> rename it?

Done. I think a file-move and rename is on the cards too, which will blow out
merge conflicts, so perhaps this is better in a follow-up. Added:

// TODO(tapted): Move entire file to /c/b/ui/app_list/app_list_service_mac.cc
// and rename this class to AppListServiceMac.

I think that's what we want to do, based on the discussions in
http://crrev.com/11973025

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12207104/diff/36063/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:136: class
AppListController : public AppListService {
On 2013/02/26 01:18:25, benwells wrote:
> Can you also add a TODO here to rename this to AppListServiceWin, or just do
it
> now?

Done. i.e.

// TODO(tapted): Move entire file to /c/b/ui/app_list/app_list_service_win.cc
// and rename this class to AppListServiceWin after removing c/b/ui/views
// dependency.

[sail, 2013-02-26 03:08:36]

cocoa/* LGTM

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

https://codereview.chromium.org/12207104/diff/38008/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:26: //
TODO(tapted): Move entire file to /c/b/ui/app_list/app_list_service_mac.cc
nit, normall we don't use abbreviations

[sky, 2013-02-26 15:46:53]

LGTM

[tapted, 2013-02-27 04:53:59]

Rebase looks happy, trybots run against head (lkgr is still ~25 hours old, but
CQ always uses head). Let's go!

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

https://codereview.chromium.org/12207104/diff/38008/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/app_list/app_list_controller_cocoa.mm:26: //
TODO(tapted): Move entire file to /c/b/ui/app_list/app_list_service_mac.cc
On 2013/02/26 03:08:36, sail wrote:
> nit, normall we don't use abbreviations

Done.

[commit-bot: I haz the power, 2013-02-27 04:55:09]

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

[commit-bot: I haz the power, 2013-02-27 06:03:00]

Change committed as 184894