Change NotifyAppList*() functions into observers on a ProfileKeyedService.

Change NotifyAppList*() functions into observers on a ProfileKeyedService.

This causes the events to also be delivered to the app list on ChromeOS, which would otherwise require separate plumbing.


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

[koz (OOO until 15th September), 2013-02-18 07:40:44]

Hi Xiyuan,

I'm trying to unify the ChromeOS and windows app lists so that they both get
"install progress" app items (currently only windows does). To that end I was
thinking that using notifications would let us reach arbitrarily many app lists
(eg: in the win/ash case where we have an ash app list and a windows one) and
cut down on boilerplate code too.

However to get ChromeOS behaving the way I want is not as simple as just
listening for the right events because on that platform the view (and the model)
are created / destroyed whenever the app list is shown / hidden. This is
problematic because it means that any time the app list is hidden all the
"install progress" app items will get destroyed.

It sounds like the thing to do would be to make the ChromeOS app list behave in
the same way that the Win one does, which is to keep the view created, but
occasionally off-screen, and then it can listen to events and keep itself
updated, same as in Windows. What do you think?

[benwells, 2013-02-18 07:56:35]

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:30:
#include "chrome/browser/ui/app_list/app_list_util.h"
Is this include still needed?

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:477: #if
defined(ENABLE_APP_LIST)
Should we kill the #if? This is nice and generic now.

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:516: #if
defined(ENABLE_APP_LIST)
Kill the #if, kill the #if, kill the #if.

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:538: #if
defined(ENABLE_APP_LIST)
Die, #if, die!

https://codereview.chromium.org/12298015/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/12298015/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension.h:969: struct ExtensionInstallingDetails {
I think this should go somewhere else. Not sure where, but extension.h is on the
path to lean and meanness.

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

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:30:
#include "chrome/browser/ui/app_list/app_list_util.h"
On 2013/02/18 07:56:36, benwells wrote:
> Is this include still needed?

Nope, good catch.

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:477: #if
defined(ENABLE_APP_LIST)
On 2013/02/18 07:56:36, benwells wrote:
> Should we kill the #if? This is nice and generic now.

Done.

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:516: #if
defined(ENABLE_APP_LIST)
On 2013/02/18 07:56:36, benwells wrote:
> Kill the #if, kill the #if, kill the #if.

Done.

https://codereview.chromium.org/12298015/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:538: #if
defined(ENABLE_APP_LIST)
On 2013/02/18 07:56:36, benwells wrote:
> Die, #if, die!

Done.

https://codereview.chromium.org/12298015/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/12298015/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension.h:969: struct ExtensionInstallingDetails {
On 2013/02/18 07:56:36, benwells wrote:
> I think this should go somewhere else. Not sure where, but extension.h is on
the
> path to lean and meanness.

As discussed, renamed to ExtensionInstallingInfo to conform to the other
detail-ey structs in here.

[benwells, 2013-02-19 03:37:12]

lgtm

[xiyuan, 2013-02-19 17:20:42]

LGTM

In general, I don't like keep things around when they are not visible and not
providing any functionality. For supporting install progress on ChromeOS, we are
missing a piece to track/remember the install progress. I would prefer to keep
this piece independent of app list.

I am thinking of creating an ExtensionInstallTracker (or something similar) that
is either part of ExtensionSystem (or ExtensionService) or a PKS (profile keyed
service). This class would listen to the new notifications and track in-progress
installations. And when app list is invoked, we query it in AppsModelBuilder.
How does this sound?

https://codereview.chromium.org/12298015/diff/4001/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/12298015/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.h:972: gfx::ImageSkia icon,
nit: const gfx::ImageSkia&

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

I'd be fine with that approach. One thing to consider about it is that it
doesn't scale up to handle any other kinds of transient state. That said, it's
nicer to depend on asking something for data, rather than needing to be around
to catch it as it floats down the stream.

Another approach that doesn't keep things around unnecessarily might be to keep
the only the model around and construct a view for it as needed. The model would
be serving the purpose of tracking state even if the view isn't there.

https://codereview.chromium.org/12298015/diff/4001/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/12298015/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.h:972: gfx::ImageSkia icon,
On 2013/02/19 17:20:42, xiyuan wrote:
> nit: const gfx::ImageSkia&

Done.

[koz (OOO until 15th September), 2013-02-20 00:13:57]

+OWNERS

thakis: chrome/browser/ui/cocoa
sky: chrome/common

[xiyuan, 2013-02-20 00:25:06]

On 2013/02/20 00:12:08, koz wrote:
> I'd be fine with that approach. One thing to consider about it is that it
> doesn't scale up to handle any other kinds of transient state. That said, it's
> nicer to depend on asking something for data, rather than needing to be around
> to catch it as it floats down the stream.
> 
> Another approach that doesn't keep things around unnecessarily might be to
keep
> the only the model around and construct a view for it as needed. The model
would
> be serving the purpose of tracking state even if the view isn't there.

I am fine with keeping the model (or AppsModelBuilder) around. We just need to
find a new owner for it.

[sky, 2013-02-20 00:42:54]

chrome/common LGTM

[Nico, 2013-02-20 09:46:51]

Isn't this backwards? I think we prefer dedicated, narrower interfaces over
notification soup.

[tfarina, 2013-02-20 12:57:45]

No, if this is using content::Notification API, then don't do this!

[tfarina, 2013-02-20 12:59:55]

There is one bug filed for not using Notifications and I've been slowly
converting some of them in the chrome/ side.

crbug.com/170921, basically John, Joi and Ben think they are evil, and in my
experience when converting them is that they are really EVIL!

[Jói, 2013-02-20 13:09:48]

I think switching to NotificationService instead of a dedicated callback
interface is definitely backwards, as Nico and Thiago have mentioned.

The motivation to unify the way ChromeOS and Windows app lists are handled seems
good, but I don't think NotificationService should be part of the solution.

Cheers,
Jói

[benwells, 2013-02-20 21:13:50]

+yoz

I understand; this is something we discussed quite a bit before making this
change. When removing these notifications we'll need some sort of notification /
observer like system for extension events, where the LOADED, UNLOADED etc.
notifications would go. These new notifications would also go there.

yoz: are there current plans to replace NotificationObserver for extension
events? I'd like to either wait for that or push it forward.

[Yoyo Zhou, 2013-02-20 21:17:42]

On 2013/02/20 21:13:50, benwells wrote:
> +yoz
> 
> I understand; this is something we discussed quite a bit before making this
> change. When removing these notifications we'll need some sort of notification
/
> observer like system for extension events, where the LOADED, UNLOADED etc.
> notifications would go. These new notifications would also go there.
> 
> yoz: are there current plans to replace NotificationObserver for extension
> events? I'd like to either wait for that or push it forward.

It's in the plans but is not likely to happen soon. I don't expect it to
necessarily look vaguely uniform the way NotificationObservers do, so you should
not worry about consistency with interfaces that don't exist yet. That can be
done later.

[koz (OOO until 15th September), 2013-02-20 23:21:37]

On 2013/02/20 21:17:42, Yoyo Zhou wrote:
> On 2013/02/20 21:13:50, benwells wrote:
> > +yoz
> > 
> > I understand; this is something we discussed quite a bit before making this
> > change. When removing these notifications we'll need some sort of
notification
> /
> > observer like system for extension events, where the LOADED, UNLOADED etc.
> > notifications would go. These new notifications would also go there.
> > 
> > yoz: are there current plans to replace NotificationObserver for extension
> > events? I'd like to either wait for that or push it forward.
> 
> It's in the plans but is not likely to happen soon. I don't expect it to
> necessarily look vaguely uniform the way NotificationObservers do, so you
should
> not worry about consistency with interfaces that don't exist yet. That can be
> done later.

Right - if there are plans to migrate all notifications to some new pattern then
it makes sense to land this and then apply the same transformation to this code
in the migration. Not using notifications or something equivalent here means
duplicating the plumbing boilerplate that is currently only present in Win to
ChromeOS, which is really not desirable.

[tfarina, 2013-02-21 00:36:08]

On Wed, Feb 20, 2013 at 8:21 PM,  <koz@chromium.org> wrote:
> Right - if there are plans to migrate all notifications to some new pattern
> then
> it makes sense to land this and then apply the same transformation to this
> code
> in the migration.
There is no new pattern. The alternatives as far as I know are
Delegate/Observer/Listener interfaces or base::Callback/Bind API.

-- 
Thiago

[koz (OOO until 15th September), 2013-02-21 05:19:05]

Ok, cool. I've refactored this to use observers of a PKS rather than
notifications. LGTY?

[benwells, 2013-02-21 06:14:05]

On 2013/02/21 05:19:05, koz wrote:
> Ok, cool. I've refactored this to use observers of a PKS rather than
> notifications. LGTY?

lgtm with some nits.

[benwells, 2013-02-21 06:15:37]

oops, here are the nits.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:41:
#include "content/public/browser/notification_service.h"
nit: remove notification_service.h

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.h (right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:27: virtual void OnInstallFailure(
nit: will fit on one line (i think)

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker_factory.h (right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:35: virtual
ProfileKeyedService* BuildServiceInstanceFor(
nit: There are some other base functions (e.g.the incognito one). Could you add
comments to the class noting why you are taking the base behavior?

[xiyuan, 2013-02-21 06:33:17]

Mostly good. And I am picky sometime. :p

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.h (right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:28: const std::string& extension_id)
= 0;
nit: add a protected virtual destructor.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:31: class InstallTracker : public
ProfileKeyedService, public InstallObserver {
InstallTracker probably should not derive from InstallObserver because it does
not serve such purpose. It could provide methods to fire the notifications for
InstallObserver but I don't think it should be derived from it.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:37: virtual void Shutdown() OVERRIDE
{}
Think we can get rid of this since we are not really overriding anything here.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:53: Profile* profile_;
This does not seem used anywhere. Think we could get rid of it.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:54: ObserverList<InstallObserver>
observers_;
DISALLOW_COPY_AND_ASSIGN(...)

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker_factory.cc (right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.cc:43: return true;
Why InstallTracker needs to be created with profile? Shouldn't the default lazy
init work?

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker_factory.h (right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:24: static void
ResetForProfile(Profile* profile);
Is this used anywhere?

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:34: //
ProfileKeyedServiceFactory:
nit: ProfileKeyedServiceFactory overrides:

[Jói, 2013-02-21 10:56:25]

Thanks, this approach seems much better to me.  Removing myself from the
reviewer list.

Cheers,
Jói

[koz (OOO until 15th September), 2013-02-25 02:39:28]

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:41:
#include "content/public/browser/notification_service.h"
On 2013/02/21 06:15:38, benwells wrote:
> nit: remove notification_service.h

Done.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.h (right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:27: virtual void OnInstallFailure(
On 2013/02/21 06:15:38, benwells wrote:
> nit: will fit on one line (i think)

Done.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:28: const std::string& extension_id)
= 0;
On 2013/02/21 06:33:17, xiyuan wrote:
> nit: add a protected virtual destructor.

Done.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:31: class InstallTracker : public
ProfileKeyedService, public InstallObserver {
On 2013/02/21 06:33:17, xiyuan wrote:
> InstallTracker probably should not derive from InstallObserver because it does
> not serve such purpose. It could provide methods to fire the notifications for
> InstallObserver but I don't think it should be derived from it.

Done.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:37: virtual void Shutdown() OVERRIDE
{}
On 2013/02/21 06:33:17, xiyuan wrote:
> Think we can get rid of this since we are not really overriding anything here.

Done.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:53: Profile* profile_;
On 2013/02/21 06:33:17, xiyuan wrote:
> This does not seem used anywhere. Think we could get rid of it.

Done.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:54: ObserverList<InstallObserver>
observers_;
On 2013/02/21 06:33:17, xiyuan wrote:
> DISALLOW_COPY_AND_ASSIGN(...)

Done.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker_factory.cc (right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.cc:43: return true;
On 2013/02/21 06:33:17, xiyuan wrote:
> Why InstallTracker needs to be created with profile? Shouldn't the default
lazy
> init work?

Yes, true. Removed.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker_factory.h (right):

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:24: static void
ResetForProfile(Profile* profile);
On 2013/02/21 06:33:17, xiyuan wrote:
> Is this used anywhere?

Removed.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:34: //
ProfileKeyedServiceFactory:
On 2013/02/21 06:33:17, xiyuan wrote:
> nit: ProfileKeyedServiceFactory overrides:

Done.

https://codereview.chromium.org/12298015/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:35: virtual
ProfileKeyedService* BuildServiceInstanceFor(
On 2013/02/21 06:15:38, benwells wrote:
> nit: There are some other base functions (e.g.the incognito one). Could you
add
> comments to the class noting why you are taking the base behavior?

Ah, or I could override ServiceRedirectedInIncognito() so it returns true and
avoid segfaults in incognito mode!

[xiyuan, 2013-02-25 17:33:23]

LGTM

[Nico, 2013-02-25 18:20:55]

lgtm

Thanks, I like this much better now.

[tfarina, 2013-02-25 19:43:18]

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.h (right):

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:10: #include
"ui/gfx/image/image_skia.h"
forward declare this.

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:14: class InstallObserver {
please, put this into c/b/e/install_observer.h

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:33: explicit InstallTracker();
no explicit

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:36: // ProfileKeyedService
implementation.
I don't think this is an implementation of ProfileKeyedService. PKS has only
Shutdown method.

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:40: void OnBeginExtensionInstall(
why these three methods have the same names of InstallObserver?

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:46: int percent_downloaded);
wrong indent

[koz (OOO until 15th September), 2013-02-25 23:35:18]

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.h (right):

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/02/25 19:43:18, tfarina wrote:
> no (c)

Done.

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:10: #include
"ui/gfx/image/image_skia.h"
On 2013/02/25 19:43:18, tfarina wrote:
> forward declare this.

Done.

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:14: class InstallObserver {
On 2013/02/25 19:43:18, tfarina wrote:
> please, put this into c/b/e/install_observer.h

Done.

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:33: explicit InstallTracker();
On 2013/02/25 19:43:18, tfarina wrote:
> no explicit

Done.

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:36: // ProfileKeyedService
implementation.
On 2013/02/25 19:43:18, tfarina wrote:
> I don't think this is an implementation of ProfileKeyedService. PKS has only
> Shutdown method.

Done.

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:40: void OnBeginExtensionInstall(
On 2013/02/25 19:43:18, tfarina wrote:
> why these three methods have the same names of InstallObserver?

This class acts as a broadcaster - it gets notified, then it notifies everyone
listening to it, so I think it's reasonable to have these functions have the
same name as the listeners.

https://codereview.chromium.org/12298015/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:46: int percent_downloaded);
On 2013/02/25 19:43:18, tfarina wrote:
> wrong indent

Done.

[tfarina, 2013-02-25 23:43:02]

you have my informal LGTM with the following changes.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.cc (right):

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.cc:21:
OnBeginExtensionInstall(extension_id,
If it fits I'd align with InstallObserver. Up to you though.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.cc:36:
OnInstallFailure(extension_id));
If it fits I'd align with InstallObserver. Up to you though.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.cc:39: void
InstallTracker::AddObserver(InstallObserver* observer) {
the order doesn't match with the order declared in the header file, this and
RemoveObserver should be moved above OnBeginExtensionInstall

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.h (right):

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:9: #include
"chrome/browser/extensions/install_observer.h"
ObserverList doesn't allow you to forward declare this instead?

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker_factory.h (right):

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:8: #include
"base/memory/singleton.h"
forward declare this:

template <typename T> struct DefaultSingletonTraits;

and include singleton.h in the source file instead.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:33: };
DISALLOW_COPY_AND_ASSING...

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/apps_model_builder.h (right):

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/apps_model_builder.h:43: // InstallObserver
overrides.
extensions::InstallObserver overrides:

Although my preference has always been by:

// Overridden from extensions::InstallObserver:

or

// extensions::InstallObserver:

[koz (OOO until 15th September), 2013-02-26 00:10:50]

Thanks, Thiago!

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.cc (right):

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.cc:21:
OnBeginExtensionInstall(extension_id,
On 2013/02/25 23:43:02, tfarina wrote:
> If it fits I'd align with InstallObserver. Up to you though.

Done.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.cc:36:
OnInstallFailure(extension_id));
On 2013/02/25 23:43:02, tfarina wrote:
> If it fits I'd align with InstallObserver. Up to you though.

Done.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.cc:39: void
InstallTracker::AddObserver(InstallObserver* observer) {
On 2013/02/25 23:43:02, tfarina wrote:
> the order doesn't match with the order declared in the header file, this and
> RemoveObserver should be moved above OnBeginExtensionInstall

Done.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker.h (right):

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker.h:9: #include
"chrome/browser/extensions/install_observer.h"
On 2013/02/25 23:43:02, tfarina wrote:
> ObserverList doesn't allow you to forward declare this instead?

No, alas :-(

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
File chrome/browser/extensions/install_tracker_factory.h (right):

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/02/25 23:43:02, tfarina wrote:
> no (c)

Done.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:8: #include
"base/memory/singleton.h"
On 2013/02/25 23:43:02, tfarina wrote:
> forward declare this:
> 
> template <typename T> struct DefaultSingletonTraits;
> 
> and include singleton.h in the source file instead.

Done.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/extensions...
chrome/browser/extensions/install_tracker_factory.h:33: };
On 2013/02/25 23:43:02, tfarina wrote:
> DISALLOW_COPY_AND_ASSING...

Done.

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/apps_model_builder.h (right):

https://codereview.chromium.org/12298015/diff/31004/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/apps_model_builder.h:43: // InstallObserver
overrides.
On 2013/02/25 23:43:02, tfarina wrote:
> extensions::InstallObserver overrides:
> 
> Although my preference has always been by:
> 
> // Overridden from extensions::InstallObserver:
> 
> or
> 
> // extensions::InstallObserver:

Done.

[commit-bot: I haz the power, 2013-02-26 04:32:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/12298015/17007

[commit-bot: I haz the power, 2013-02-26 05:30:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-26 05:53:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/12298015/18014

[commit-bot: I haz the power, 2013-02-26 07:55:44]

Change committed as 184604