Lazy initialization for ProcessesEventRouter.

Lazy initialization for ProcessesEventRouter.

Make it profile-scoped, not a singleton.
Construct EventRouter at ExtensionSystem creation time.
Implement EventRouter::Observer interface for lazy event router initialization, and use it for processes API.

BUG=156715, 159265
TBR=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166813

[Yoyo Zhou, 2012-11-07 03:25:19]

https://codereview.chromium.org/11359081/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_system.cc (right):

https://codereview.chromium.org/11359081/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_system.cc:120:
ProcessesAPIService::Get(profile_);
This is the unfortunate part.

The observers are registered with EventRouter (for logically they belong there).
But EventRouter isn't initialized until relatively late in profile startup,
along with most of the extensions stuff. Meanwhile, PKSF only can change the
order of PKS construction, so the ProcessesAPIService can either be created on
demand (here) or right after ExtensionSystem is constructed.

The most obvious solution is to register observers with ExtensionSystem instead,
but I'm not sure if the observer map should live there, or if the registrations
should merely be stashed until EventRouter comes alive.

[Aaron Boodman, 2012-11-07 04:08:19]

cool

https://codereview.chromium.org/11359081/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_system.cc (right):

https://codereview.chromium.org/11359081/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_system.cc:120:
ProcessesAPIService::Get(profile_);
On 2012/11/07 03:25:19, Yoyo Zhou wrote:
> This is the unfortunate part.
> 
> The observers are registered with EventRouter (for logically they belong
there).
> But EventRouter isn't initialized until relatively late in profile startup,
> along with most of the extensions stuff. Meanwhile, PKSF only can change the
> order of PKS construction, so the ProcessesAPIService can either be created on
> demand (here) or right after ExtensionSystem is constructed.
> 
> The most obvious solution is to register observers with ExtensionSystem
instead,
> but I'm not sure if the observer map should live there, or if the
registrations
> should merely be stashed until EventRouter comes alive.

Move EventRouter construction into ExtensionSystem construction?

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/processes/processes_api_service.h (right):

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/processes/processes_api_service.h:16: namespace
extensions {
Nit: In the end, I don't think we want these in the extensions namespace (that
should just be the core). Not sure about this intermediate state tho.

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/event_router.h (right):

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/event_router.h:89: void RegisterObserver(Observer*
observer,
Do we not need an UnregisterObserver? And if so, I wonder if we need something
like base/observer_list.h but in map form so that we can remove during
iteration.

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_dependency_manager.cc:19: #include
"chrome/browser/extensions/api/processes/processes_api_service_factory.h"
Can you clean up this factory boilerplate now?

[Yoyo Zhou, 2012-11-07 22:53:36]

https://codereview.chromium.org/11359081/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_system.cc (right):

https://codereview.chromium.org/11359081/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_system.cc:120:
ProcessesAPIService::Get(profile_);
On 2012/11/07 04:08:20, Aaron Boodman wrote:
> On 2012/11/07 03:25:19, Yoyo Zhou wrote:
> > This is the unfortunate part.
> > 
> > The observers are registered with EventRouter (for logically they belong
> there).
> > But EventRouter isn't initialized until relatively late in profile startup,
> > along with most of the extensions stuff. Meanwhile, PKSF only can change the
> > order of PKS construction, so the ProcessesAPIService can either be created
on
> > demand (here) or right after ExtensionSystem is constructed.
> > 
> > The most obvious solution is to register observers with ExtensionSystem
> instead,
> > but I'm not sure if the observer map should live there, or if the
> registrations
> > should merely be stashed until EventRouter comes alive.
> 
> Move EventRouter construction into ExtensionSystem construction?

This turned out to be the best solution. I had to clean up some other detritus
in EventRouter to do so, but it was worth it.

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/processes/processes_api_service.h (right):

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/processes/processes_api_service.h:16: namespace
extensions {
On 2012/11/07 04:08:20, Aaron Boodman wrote:
> Nit: In the end, I don't think we want these in the extensions namespace (that
> should just be the core). Not sure about this intermediate state tho.

Should they be in a namespace like processes_api? extensions_api_processes?
Whatever it is, I'd want to do it in another CL.

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/event_router.h (right):

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/event_router.h:89: void RegisterObserver(Observer*
observer,
On 2012/11/07 04:08:20, Aaron Boodman wrote:
> Do we not need an UnregisterObserver? And if so, I wonder if we need something
> like base/observer_list.h but in map form so that we can remove during
> iteration.

After thinking about this, I do think UnregisterObserver is a good idea. I'll
make it of the form UnregisterObserver(observer) to unregister from all events.

Actually, I originally wanted to only support the use case of starting up event
routers lazily, so that the observers would get removed once OnListenerAdded was
fired the first time, but that turns out not to be enough. But for that case,
the goop class should unregister itself.

And yeah, ObserverList doesn't fit because it's not a map.

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_dependency_manager.cc:19: #include
"chrome/browser/extensions/api/processes/processes_api_service_factory.h"
On 2012/11/07 04:08:20, Aaron Boodman wrote:
> Can you clean up this factory boilerplate now?

No, it must be here if we want it to be created when the profile is created
(which is now yes).

[Yoyo Zhou, 2012-11-07 22:54:17]

+ben for OWNERS

[Aaron Boodman, 2012-11-07 23:51:28]

If you decide to change how InitFromPrefs() is done, then I'll take another look
at that. Otherwise LGTM w/ nits.

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/processes/processes_api_service.h (right):

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/processes/processes_api_service.h:16: namespace
extensions {
On 2012/11/07 22:53:36, Yoyo Zhou wrote:
> On 2012/11/07 04:08:20, Aaron Boodman wrote:
> > Nit: In the end, I don't think we want these in the extensions namespace
(that
> > should just be the core). Not sure about this intermediate state tho.
> 
> Should they be in a namespace like processes_api? extensions_api_processes?
> Whatever it is, I'd want to do it in another CL.

I think it depends on whether they end up being their own module or not. It
seems like the pattern in Chrome is that all the modules except the top-level
code have namespaces.

So if they're just dumped into chrome/browser/extension_apis or something, then
no, they'd have no namespace. I sorta think this is the way it should be, just
because I can't think of any reason for extension API impls for Chrome to be
compilable separately from Chrome.

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://codereview.chromium.org/11359081/diff/2001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_dependency_manager.cc:19: #include
"chrome/browser/extensions/api/processes/processes_api_service_factory.h"
On 2012/11/07 22:53:36, Yoyo Zhou wrote:
> On 2012/11/07 04:08:20, Aaron Boodman wrote:
> > Can you clean up this factory boilerplate now?
> 
> No, it must be here if we want it to be created when the profile is created
> (which is now yes).

I mean can we introduce a factory-in-a-box utility class that eliminates having
to implement one of these for every PKS.

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
File chrome/browser/extensions/event_router.cc (right):

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/event_router.cc:190: ExtensionDevToolsManager*
extension_devtools_manager =
Add a TODO to get rid of this?

Also, out of curiosity, how do you plan to do it, since this client wants all
events, not just particular ones?

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
File chrome/browser/extensions/event_router.h (right):

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/event_router.h:88: // Registers an observer,
interested in listeners that receive the event
Nit: comment a little unclear. How about just "Registers an observer to be
notified when an event listener is added for |event_name|."

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:901: //           
crbug.com/159265
Nit: kill the fancy indenting.

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
File chrome/browser/extensions/extension_system.cc (right):

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/extension_system.cc:74: event_router_.reset(new
EventRouter(profile_));
Why not make it part of the initializer list?

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/extension_system.cc:90:
event_router_->InitWithPrefs(extension_prefs_.get());
Shared::InitPrefs() is called in the constructor. Isn't it possible to pass the
prefs into the ctor and get rid of this method?

[Yoyo Zhou, 2012-11-08 00:18:16]

You might need to look again then. (But now the EventRouter constructor is
unchanged from the way it started out.)

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
File chrome/browser/extensions/event_router.cc (right):

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/event_router.cc:190: ExtensionDevToolsManager*
extension_devtools_manager =
On 2012/11/07 23:51:28, Aaron Boodman wrote:
> Add a TODO to get rid of this?

Done.

> Also, out of curiosity, how do you plan to do it, since this client wants all
> events, not just particular ones?

Probably I'd just have another class of observer.

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
File chrome/browser/extensions/event_router.h (right):

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/event_router.h:88: // Registers an observer,
interested in listeners that receive the event
On 2012/11/07 23:51:28, Aaron Boodman wrote:
> Nit: comment a little unclear. How about just "Registers an observer to be
> notified when an event listener is added for |event_name|."

Done.

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:901: //           
crbug.com/159265
On 2012/11/07 23:51:28, Aaron Boodman wrote:
> Nit: kill the fancy indenting.

Done.

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
File chrome/browser/extensions/extension_system.cc (right):

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/extension_system.cc:74: event_router_.reset(new
EventRouter(profile_));
On 2012/11/07 23:51:28, Aaron Boodman wrote:
> Why not make it part of the initializer list?

See below.

https://codereview.chromium.org/11359081/diff/1015/chrome/browser/extensions/...
chrome/browser/extensions/extension_system.cc:90:
event_router_->InitWithPrefs(extension_prefs_.get());
On 2012/11/07 23:51:28, Aaron Boodman wrote:
> Shared::InitPrefs() is called in the constructor. Isn't it possible to pass
the
> prefs into the ctor and get rid of this method?

Yes, I was misinterpreting what was going on. I'll go back to how it was before
(except initializing it here).

[Aaron Boodman, 2012-11-08 00:21:27]

lgtm

[Aaron Boodman, 2012-11-08 00:21:59]

(you didn't answer the question about the factory-in-a-box, but I'll assume
that's a separate cl, if it's even possible)

[Yoyo Zhou, 2012-11-08 00:35:43]

On Wed, Nov 7, 2012 at 4:21 PM,  <aa@chromium.org> wrote:
> (you didn't answer the question about the factory-in-a-box, but I'll assume
> that's a separate cl, if it's even possible)

I wasn't sure where to begin with that one. Maybe it's possible to
have a factory template, if the factories all turn out to be alike.
Seems more appealing than a factory factory.

BTW, I changed the factory to be NULL for TestingProfiles so that
tests don't crash.

[Aaron Boodman, 2012-11-08 00:52:30]

On Wed, Nov 7, 2012 at 4:35 PM, Yoyo Zhou <yoz@chromium.org> wrote:
> On Wed, Nov 7, 2012 at 4:21 PM,  <aa@chromium.org> wrote:
>> (you didn't answer the question about the factory-in-a-box, but I'll assume
>> that's a separate cl, if it's even possible)
>
> I wasn't sure where to begin with that one. Maybe it's possible to
> have a factory template, if the factories all turn out to be alike.
> Seems more appealing than a factory factory.
>
> BTW, I changed the factory to be NULL for TestingProfiles so that
> tests don't crash.

All the factories I have seen are just a static Create(Profile*)
method with a lot of virtual methods overridden that return constant
bools. It seems you could have:

class PKSFactory {
  PKSFactory(PKS*(create_function)(Profile*));
  void set_is_redirected_in_incognito(bool);
  void set_is_created_with_profile(bool);
  void set_is_null_while_testing(bool);
};

PKSFactory* factory = new PKSFactory(&ProcessAPI::CreateFromProfile);
factory->set_is_redirected_in_incognito(true);
... etc ...

Right?

- a

[Aaron Boodman, 2012-11-08 00:52:56]

BTW, for naming. What about instead of FooBarAPIService, just FooBarAPI?

- a

On Wed, Nov 7, 2012 at 4:51 PM, Aaron Boodman <aa@chromium.org> wrote:
> On Wed, Nov 7, 2012 at 4:35 PM, Yoyo Zhou <yoz@chromium.org> wrote:
>> On Wed, Nov 7, 2012 at 4:21 PM,  <aa@chromium.org> wrote:
>>> (you didn't answer the question about the factory-in-a-box, but I'll assume
>>> that's a separate cl, if it's even possible)
>>
>> I wasn't sure where to begin with that one. Maybe it's possible to
>> have a factory template, if the factories all turn out to be alike.
>> Seems more appealing than a factory factory.
>>
>> BTW, I changed the factory to be NULL for TestingProfiles so that
>> tests don't crash.
>
> All the factories I have seen are just a static Create(Profile*)
> method with a lot of virtual methods overridden that return constant
> bools. It seems you could have:
>
> class PKSFactory {
>   PKSFactory(PKS*(create_function)(Profile*));
>   void set_is_redirected_in_incognito(bool);
>   void set_is_created_with_profile(bool);
>   void set_is_null_while_testing(bool);
> };
>
> PKSFactory* factory = new PKSFactory(&ProcessAPI::CreateFromProfile);
> factory->set_is_redirected_in_incognito(true);
> ... etc ...
>
> Right?
>
> - a

[Yoyo Zhou, 2012-11-08 01:01:02]

On Wed, Nov 7, 2012 at 4:52 PM, Aaron Boodman <aa@chromium.org> wrote:
> BTW, for naming. What about instead of FooBarAPIService, just FooBarAPI?

 Sounds good to me. It runs the risk of confusion with ExtensionAPI,
if you don't think that's a problem. But I'll have to co-opt the
*_api.cc file for it.

> On Wed, Nov 7, 2012 at 4:51 PM, Aaron Boodman <aa@chromium.org> wrote:
>> On Wed, Nov 7, 2012 at 4:35 PM, Yoyo Zhou <yoz@chromium.org> wrote:
>>> On Wed, Nov 7, 2012 at 4:21 PM,  <aa@chromium.org> wrote:
>>>> (you didn't answer the question about the factory-in-a-box, but I'll
assume
>>>> that's a separate cl, if it's even possible)
>>>
>>> I wasn't sure where to begin with that one. Maybe it's possible to
>>> have a factory template, if the factories all turn out to be alike.
>>> Seems more appealing than a factory factory.
>>>
>>> BTW, I changed the factory to be NULL for TestingProfiles so that
>>> tests don't crash.
>>
>> All the factories I have seen are just a static Create(Profile*)
>> method with a lot of virtual methods overridden that return constant
>> bools. It seems you could have:
>>
>> class PKSFactory {
>>   PKSFactory(PKS*(create_function)(Profile*));
>>   void set_is_redirected_in_incognito(bool);
>>   void set_is_created_with_profile(bool);
>>   void set_is_null_while_testing(bool);
>> };
>>
>> PKSFactory* factory = new PKSFactory(&ProcessAPI::CreateFromProfile);
>> factory->set_is_redirected_in_incognito(true);
>> ... etc ...
>>
>> Right?

 I don't understand how you get an instance of the factory here. The
basic pattern to get an instance of the service is
QuaggaFactory::GetInstance()->GetServiceForProfile(profile, ...).

[Aaron Boodman, 2012-11-08 01:14:41]

On Wed, Nov 7, 2012 at 5:01 PM, Yoyo Zhou <yoz@chromium.org> wrote:
> On Wed, Nov 7, 2012 at 4:52 PM, Aaron Boodman <aa@chromium.org> wrote:
>> BTW, for naming. What about instead of FooBarAPIService, just FooBarAPI?
>
>  Sounds good to me. It runs the risk of confusion with ExtensionAPI,
> if you don't think that's a problem. But I'll have to co-opt the
> *_api.cc file for it.
>
>> On Wed, Nov 7, 2012 at 4:51 PM, Aaron Boodman <aa@chromium.org> wrote:
>>> On Wed, Nov 7, 2012 at 4:35 PM, Yoyo Zhou <yoz@chromium.org> wrote:
>>>> On Wed, Nov 7, 2012 at 4:21 PM,  <aa@chromium.org> wrote:
>>>>> (you didn't answer the question about the factory-in-a-box, but I'll
assume
>>>>> that's a separate cl, if it's even possible)
>>>>
>>>> I wasn't sure where to begin with that one. Maybe it's possible to
>>>> have a factory template, if the factories all turn out to be alike.
>>>> Seems more appealing than a factory factory.
>>>>
>>>> BTW, I changed the factory to be NULL for TestingProfiles so that
>>>> tests don't crash.
>>>
>>> All the factories I have seen are just a static Create(Profile*)
>>> method with a lot of virtual methods overridden that return constant
>>> bools. It seems you could have:
>>>
>>> class PKSFactory {
>>>   PKSFactory(PKS*(create_function)(Profile*));
>>>   void set_is_redirected_in_incognito(bool);
>>>   void set_is_created_with_profile(bool);
>>>   void set_is_null_while_testing(bool);
>>> };
>>>
>>> PKSFactory* factory = new PKSFactory(&ProcessAPI::CreateFromProfile);
>>> factory->set_is_redirected_in_incognito(true);
>>> ... etc ...
>>>
>>> Right?
>
>  I don't understand how you get an instance of the factory here. The
> basic pattern to get an instance of the service is
> QuaggaFactory::GetInstance()->GetServiceForProfile(profile, ...).

I was thinking that you create instances of QuaggaFactory using its
constructor and pass them around that way. But I see that will be
problematic because of DependsOn().

What about using templates?

template<typename CreatedClass, typename DependedOnClass,
    bool redirectedInIncognito = false, bool createdWithProfile = false,
    bool isNullWhileTesting> class PKSFactoryTemplate {
  ...
};

[Yoyo Zhou, 2012-11-08 01:31:02]

Passing factories around is somewhat at odds with how they're
currently used (as singletons).

Well, for the cases of goop classes like ProcessesAPI, it'd probably
be easier to templatize them if they just DependOn ExtensionSystem. If
they also depend on their own system's PKS, that could be another
story, maybe a template-instance-specific override.

On Wed, Nov 7, 2012 at 5:14 PM, Aaron Boodman <aa@chromium.org> wrote:
> On Wed, Nov 7, 2012 at 5:01 PM, Yoyo Zhou <yoz@chromium.org> wrote:
>> On Wed, Nov 7, 2012 at 4:52 PM, Aaron Boodman <aa@chromium.org> wrote:
>>> BTW, for naming. What about instead of FooBarAPIService, just FooBarAPI?
>>
>>  Sounds good to me. It runs the risk of confusion with ExtensionAPI,
>> if you don't think that's a problem. But I'll have to co-opt the
>> *_api.cc file for it.
>>
>>> On Wed, Nov 7, 2012 at 4:51 PM, Aaron Boodman <aa@chromium.org> wrote:
>>>> On Wed, Nov 7, 2012 at 4:35 PM, Yoyo Zhou <yoz@chromium.org> wrote:
>>>>> On Wed, Nov 7, 2012 at 4:21 PM,  <aa@chromium.org> wrote:
>>>>>> (you didn't answer the question about the factory-in-a-box, but I'll
assume
>>>>>> that's a separate cl, if it's even possible)
>>>>>
>>>>> I wasn't sure where to begin with that one. Maybe it's possible to
>>>>> have a factory template, if the factories all turn out to be alike.
>>>>> Seems more appealing than a factory factory.
>>>>>
>>>>> BTW, I changed the factory to be NULL for TestingProfiles so that
>>>>> tests don't crash.
>>>>
>>>> All the factories I have seen are just a static Create(Profile*)
>>>> method with a lot of virtual methods overridden that return constant
>>>> bools. It seems you could have:
>>>>
>>>> class PKSFactory {
>>>>   PKSFactory(PKS*(create_function)(Profile*));
>>>>   void set_is_redirected_in_incognito(bool);
>>>>   void set_is_created_with_profile(bool);
>>>>   void set_is_null_while_testing(bool);
>>>> };
>>>>
>>>> PKSFactory* factory = new PKSFactory(&ProcessAPI::CreateFromProfile);
>>>> factory->set_is_redirected_in_incognito(true);
>>>> ... etc ...
>>>>
>>>> Right?
>>
>>  I don't understand how you get an instance of the factory here. The
>> basic pattern to get an instance of the service is
>> QuaggaFactory::GetInstance()->GetServiceForProfile(profile, ...).
>
> I was thinking that you create instances of QuaggaFactory using its
> constructor and pass them around that way. But I see that will be
> problematic because of DependsOn().
>
> What about using templates?
>
> template<typename CreatedClass, typename DependedOnClass,
>     bool redirectedInIncognito = false, bool createdWithProfile = false,
>     bool isNullWhileTesting> class PKSFactoryTemplate {
>   ...
> };

[Yoyo Zhou, 2012-11-08 02:44:29]

ProcessesAPIService->ProcessesAPI as discussed.

[Aaron Boodman, 2012-11-08 02:45:50]

lgtm stands

On Wed, Nov 7, 2012 at 6:44 PM,  <yoz@chromium.org> wrote:
> ProcessesAPIService->ProcessesAPI as discussed.
>
> https://codereview.chromium.org/11359081/

[commit-bot: I haz the power, 2012-11-08 18:07:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/11359081/7007

[commit-bot: I haz the power, 2012-11-08 19:47:45]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-08 21:50:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/11359081/1022

[commit-bot: I haz the power, 2012-11-08 22:09:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/11359081/6014

[commit-bot: I haz the power, 2012-11-09 00:03:32]

Change committed as 166813

[Matt Perry, 2012-11-09 00:23:34]

drive-by nits

https://codereview.chromium.org/11359081/diff/6014/chrome/browser/extensions/...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/11359081/diff/6014/chrome/browser/extensions/...
chrome/browser/extensions/api/processes/processes_api.h:117: // by moving some
state to ProcessesAPI from the event router.
Couldn't you have the accessor lazily create the event router?

https://codereview.chromium.org/11359081/diff/6014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_system.cc (right):

https://codereview.chromium.org/11359081/diff/6014/chrome/browser/extensions/...
chrome/browser/extensions/extension_system.cc:89: event_router_.reset(new
EventRouter(profile_, extension_prefs_.get()));
Weird... these aren't prefs.

[Yoyo Zhou, 2012-11-09 00:35:55]

Since I had to revert this, I can get to one of your comments...

https://codereview.chromium.org/11359081/diff/6014/chrome/browser/extensions/...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/11359081/diff/6014/chrome/browser/extensions/...
chrome/browser/extensions/api/processes/processes_api.h:117: // by moving some
state to ProcessesAPI from the event router.
On 2012/11/09 00:23:34, Matt Perry wrote:
> Couldn't you have the accessor lazily create the event router?

Duh. Thanks.

https://codereview.chromium.org/11359081/diff/6014/chrome/browser/extensions/...
File chrome/browser/extensions/extension_system.cc (right):

https://codereview.chromium.org/11359081/diff/6014/chrome/browser/extensions/...
chrome/browser/extensions/extension_system.cc:89: event_router_.reset(new
EventRouter(profile_, extension_prefs_.get()));
On 2012/11/09 00:23:34, Matt Perry wrote:
> Weird... these aren't prefs.

Yeah, the InitPrefs name is no longer appropriate. Maybe it should just go in
the constructor. I'll do that in a separate change.

[Ben Goodger (Google), 2012-11-12 16:32:02]

lgtm for the gyp