Expands the chrome.experimental.processes extension API. ...

Expands the chrome.experimental.processes extension API. 
Adds an onUpdated event that reports process metrics from the TaskManager, 
and modifies the TaskManager to support multiple independent observers. 

BUG=32302
TEST=ExtensionApiTest.Processes browsertest 
TEST=process_monitor sample extension 


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

[Charlie Reis, 2010-10-15 02:51:59]

Ok, I'm getting ready to land this again-- it was reverted because of a test
failure on the Vista Tests bot, but it turns out that's a deeper issue affecting
anything that opens a task manager (see http://crbug.com/44991).  I'd like to
have this CL ready to land as soon as that issue is resolved.

Patch Set 1 is identical to what I landed in
http://codereview.chromium.org/3597016/show (except for a weird patch issue with
two of the process_monitor files, which I fixed in Patch Set 3).

Patch Set 2 fixes the show_tabs sample extension to use the new
getProcessIdForTab API, since the previous CL accidentally left it broken. 
Patch Set 2 also fixes the cast issues discussed in
http://codereview.chromium.org/3762004/show.

Since there's a few tweaks here since the version I committed earlier, I'll wait
for Erik's LGTM before proceeding.

Thanks all!

[Erik does not do reviews, 2010-10-15 18:40:34]

I didn't have time to look at the apitest js code yet, but hopefully this is
enough to start on.

http://codereview.chromium.org/3801008/diff/13032/23002
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23002#newcode83
chrome/browser/extensions/extension_processes_api.cc:83: }
how about an else for unknown types?  I suspect that this misses some like
utility processes, the gpu process, etc.

http://codereview.chromium.org/3801008/diff/13032/23002#newcode87
chrome/browser/extensions/extension_processes_api.cc:87: // TODO(creis): Network
is actually reported per-resource (tab),
nit: newline above comment

http://codereview.chromium.org/3801008/diff/13032/23002#newcode107
chrome/browser/extensions/extension_processes_api.cc:107: for
(ProfileSet::iterator it = profiles_.begin();
Cross profile stuff definitely brings up some interesting issues... this seems
right in this case, but I wonder if there's going to be other weird stuff as a
result (tab ids?)

http://codereview.chromium.org/3801008/diff/13032/23002#newcode130
chrome/browser/extensions/extension_processes_api.cc:130: if
(!ExtensionTabUtil::GetTabById(tab_id, profile(), include_incognito(),
set error_ here as well

http://codereview.chromium.org/3801008/diff/13032/23006
File chrome/browser/extensions/extension_processes_apitest.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23006#newcode33
chrome/browser/extensions/extension_processes_apitest.cc:33: EXPECT_EQ(1,
model->update_requests_);
this seems like it could be flaky since it depends on the extension process
registering its event before this gets called.  in most cases, it will, but on
the bots it seems less likely.  Search for ExtensionTestMessageListener and
WaitUtilSatisfied.

http://codereview.chromium.org/3801008/diff/13032/23008
File chrome/browser/gtk/task_manager_gtk.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23008#newcode491
chrome/browser/gtk/task_manager_gtk.cc:491: if (model_->ResourceCount() > 0) {
remove {} for one-line blocks

http://codereview.chromium.org/3801008/diff/13032/23009
File chrome/browser/task_manager/task_manager.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23009#newcode124
chrome/browser/task_manager/task_manager.cc:124: DCHECK(index <
ResourceCount());
is this DCHECK enough?  what about index < 0?  or release mode?

http://codereview.chromium.org/3801008/diff/13032/23009#newcode501
chrome/browser/task_manager/task_manager.cc:501: update_requests_++;
what happens if this int wraps?  (say I was being evil in how I used the API)

http://codereview.chromium.org/3801008/diff/13032/23009#newcode533
chrome/browser/task_manager/task_manager.cc:533: update_requests_--;
is it possible for this to go below 0 if I'm evil?

http://codereview.chromium.org/3801008/diff/13032/23011
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3801008/diff/13032/23011#newcode2586
chrome/common/extensions/api/extension_api.json:2586: "type": "number",
do these really need to be numbers rather than integers?  do we care about
fractions?  are the numbers ever going to overflow 32 bits?

http://codereview.chromium.org/3801008/diff/13032/23011#newcode2639
chrome/common/extensions/api/extension_api.json:2639: "additionalProperties": {
"type": "any" }
aren't these just Process objects that you defined above?

[Erik does not do reviews, 2010-10-15 18:52:09]

http://codereview.chromium.org/3801008/diff/13032/23025
File chrome/test/data/extensions/api_test/processes/api/test.js (right):

http://codereview.chromium.org/3801008/diff/13032/23025#newcode97
chrome/test/data/extensions/api_test/processes/api/test.js:97: assertEq(5,
pids.length);
I'm a little worried that this number is a bit fragile given that we may add or
remove component extensions in the future.  How about >= #tabs + 2 (this
extension + browser)?

http://codereview.chromium.org/3801008/diff/13032/23025#newcode99
chrome/test/data/extensions/api_test/processes/api/test.js:99: // Should be able
to look up process object by ID.
It might also be cool to look up the process id for this extension (by name
maybe?).

http://codereview.chromium.org/3801008/diff/13032/23025#newcode134
chrome/test/data/extensions/api_test/processes/api/test.js:134: assertEq(1,
extensionCount);
I'm surprised this works.  At a minimum we also have the bookmark manager
component extension.  Again, maybe this one should be >=

[Charlie Reis, 2010-10-15 21:59:21]

Thanks-- those are useful comments.  Can you take another look?

Jay, can you confirm it's a good idea to change these DCHECKs to CHECKs in
tash_manager.cc?

http://codereview.chromium.org/3801008/diff/13032/23002
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23002#newcode83
chrome/browser/extensions/extension_processes_api.cc:83: }
On 2010/10/15 18:40:34, Erik Kay wrote:
> how about an else for unknown types?  I suspect that this misses some like
> utility processes, the gpu process, etc.

Right now the "else" is plugin (see above).  Do you know any good ways to detect
those types of processes, or should we add some logic in a future CL?  (For now,
I've separated the block and added a TODO comment.)

http://codereview.chromium.org/3801008/diff/13032/23002#newcode87
chrome/browser/extensions/extension_processes_api.cc:87: // TODO(creis): Network
is actually reported per-resource (tab),
On 2010/10/15 18:40:34, Erik Kay wrote:
> nit: newline above comment

Done.

http://codereview.chromium.org/3801008/diff/13032/23002#newcode107
chrome/browser/extensions/extension_processes_api.cc:107: for
(ProfileSet::iterator it = profiles_.begin();
On 2010/10/15 18:40:34, Erik Kay wrote:
> Cross profile stuff definitely brings up some interesting issues... this seems
> right in this case, but I wonder if there's going to be other weird stuff as a
> result (tab ids?)

We may need to address that when we add tab IDs to the process object.  At the
moment there's no way to go from Process to tab ID, so I don't think it's any
worse than before.

It's worth noting that both the process_monitor and show_tabs sample extensions
appear to work correctly in Incognito mode at the moment-- they show the full
contents of the task manager.

http://codereview.chromium.org/3801008/diff/13032/23002#newcode130
chrome/browser/extensions/extension_processes_api.cc:130: if
(!ExtensionTabUtil::GetTabById(tab_id, profile(), include_incognito(),
On 2010/10/15 18:40:34, Erik Kay wrote:
> set error_ here as well

Done.

http://codereview.chromium.org/3801008/diff/13032/23006
File chrome/browser/extensions/extension_processes_apitest.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23006#newcode33
chrome/browser/extensions/extension_processes_apitest.cc:33: EXPECT_EQ(1,
model->update_requests_);
On 2010/10/15 18:40:34, Erik Kay wrote:
> this seems like it could be flaky since it depends on the extension process
> registering its event before this gets called.  in most cases, it will, but on
> the bots it seems less likely.  Search for ExtensionTestMessageListener and
> WaitUtilSatisfied.

Done.

http://codereview.chromium.org/3801008/diff/13032/23008
File chrome/browser/gtk/task_manager_gtk.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23008#newcode491
chrome/browser/gtk/task_manager_gtk.cc:491: if (model_->ResourceCount() > 0) {
On 2010/10/15 18:40:34, Erik Kay wrote:
> remove {} for one-line blocks

Done.

http://codereview.chromium.org/3801008/diff/13032/23009
File chrome/browser/task_manager/task_manager.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23009#newcode124
chrome/browser/task_manager/task_manager.cc:124: DCHECK(index <
ResourceCount());
On 2010/10/15 18:40:34, Erik Kay wrote:
> is this DCHECK enough?  what about index < 0?  or release mode?

That's a good point, actually.  All of the existing GetX(index) methods on
TaskManager should really have a stronger check.  Does a CHECK sound good?

http://codereview.chromium.org/3801008/diff/13032/23009#newcode501
chrome/browser/task_manager/task_manager.cc:501: update_requests_++;
On 2010/10/15 18:40:34, Erik Kay wrote:
> what happens if this int wraps?  (say I was being evil in how I used the API)

It only gets incremented once per extension.  If an extension adds 1000
listeners, it won't increment this 1000 times.  (I just updated the test to
confirm that.)

Assuming extensions can't automatically install other extensions, I don't think
rollover is a concern.

http://codereview.chromium.org/3801008/diff/13032/23009#newcode533
chrome/browser/task_manager/task_manager.cc:533: update_requests_--;
On 2010/10/15 18:40:34, Erik Kay wrote:
> is it possible for this to go below 0 if I'm evil?

I can't think of a scenario where it would happen, but there's no point in being
at risk of it.  I'll leave the DCHECK in place so we catch it in debug builds,
but I've made sure it doesn't go negative after that.

http://codereview.chromium.org/3801008/diff/13032/23011
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3801008/diff/13032/23011#newcode2586
chrome/common/extensions/api/extension_api.json:2586: "type": "number",
On 2010/10/15 18:40:34, Erik Kay wrote:
> do these really need to be numbers rather than integers?  do we care about
> fractions?  are the numbers ever going to overflow 32 bits?

CPU is a number because we report it to the tenth on Mac (to be consistent with
Activity Monitor).  Is there a good reason to round it off for extensions?

Network could be an integer.  It should really be described as bytes per second,
so I've fixed that.

I've changed the two memory metrics to integers as well, since that's what they
are in extension_processes_api.cc.  However, on a machine with 10+ GB of RAM,
it's seems possible that memory could overflow 32 bits...  Do you think we
should make them numbers instead?

http://codereview.chromium.org/3801008/diff/13032/23011#newcode2639
chrome/common/extensions/api/extension_api.json:2639: "additionalProperties": {
"type": "any" }
On 2010/10/15 18:40:34, Erik Kay wrote:
> aren't these just Process objects that you defined above?

Yep.  Fixed.

http://codereview.chromium.org/3801008/diff/13032/23025
File chrome/test/data/extensions/api_test/processes/api/test.js (right):

http://codereview.chromium.org/3801008/diff/13032/23025#newcode97
chrome/test/data/extensions/api_test/processes/api/test.js:97: assertEq(5,
pids.length);
On 2010/10/15 18:52:10, Erik Kay wrote:
> I'm a little worried that this number is a bit fragile given that we may add
or
> remove component extensions in the future.  How about >= #tabs + 2 (this
> extension + browser)?

I made it >= 5.  (It's technically not #tabs + 2, since there are 3 renderer
processes for 5 tabs at this point.)

http://codereview.chromium.org/3801008/diff/13032/23025#newcode99
chrome/test/data/extensions/api_test/processes/api/test.js:99: // Should be able
to look up process object by ID.
On 2010/10/15 18:52:10, Erik Kay wrote:
> It might also be cool to look up the process id for this extension (by name
> maybe?).

Yes, I'm hoping to add an API for looking up process IDs for extensions and
plugins in a future CL.  I agree that it'd be cool for an extension to monitor
its own CPU use.

http://codereview.chromium.org/3801008/diff/13032/23025#newcode134
chrome/test/data/extensions/api_test/processes/api/test.js:134: assertEq(1,
extensionCount);
On 2010/10/15 18:52:10, Erik Kay wrote:
> I'm surprised this works.  At a minimum we also have the bookmark manager
> component extension.  Again, maybe this one should be >=

Interesting-- it didn't give me trouble.  If we expect other types might be
running, I should probably not fail in the default clause either.

I left browserCount as ==, and changed the others to >= to be more flexible.

[Erik does not do reviews, 2010-10-16 04:25:15]

http://codereview.chromium.org/3801008/diff/13032/23002
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23002#newcode83
chrome/browser/extensions/extension_processes_api.cc:83: }
On 2010/10/15 21:59:21, creis wrote:
> On 2010/10/15 18:40:34, Erik Kay wrote:
> > how about an else for unknown types?  I suspect that this misses some like
> > utility processes, the gpu process, etc.
> 
> Right now the "else" is plugin (see above).  Do you know any good ways to
detect
> those types of processes, or should we add some logic in a future CL?  (For
now,
> I've separated the block and added a TODO comment.)

Unfortunately I don't know how to detect this.  You'll need to do a little
digging I guess.

http://codereview.chromium.org/3801008/diff/13032/23009
File chrome/browser/task_manager/task_manager.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23009#newcode501
chrome/browser/task_manager/task_manager.cc:501: update_requests_++;
On 2010/10/15 21:59:21, creis wrote:
> On 2010/10/15 18:40:34, Erik Kay wrote:
> > what happens if this int wraps?  (say I was being evil in how I used the
API)
> 
> It only gets incremented once per extension.  If an extension adds 1000
> listeners, it won't increment this 1000 times.  (I just updated the test to
> confirm that.)
> 
> Assuming extensions can't automatically install other extensions, I don't
think
> rollover is a concern.

sounds good.  thanks.

http://codereview.chromium.org/3801008/diff/13032/23011
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3801008/diff/13032/23011#newcode2586
chrome/common/extensions/api/extension_api.json:2586: "type": "number",
On 2010/10/15 21:59:21, creis wrote:
> On 2010/10/15 18:40:34, Erik Kay wrote:
> > do these really need to be numbers rather than integers?  do we care about
> > fractions?  are the numbers ever going to overflow 32 bits?
> 
> CPU is a number because we report it to the tenth on Mac (to be consistent
with
> Activity Monitor).  Is there a good reason to round it off for extensions?
> 
> Network could be an integer.  It should really be described as bytes per
second,
> so I've fixed that.
> 
> I've changed the two memory metrics to integers as well, since that's what
they
> are in extension_processes_api.cc.  However, on a machine with 10+ GB of RAM,
> it's seems possible that memory could overflow 32 bits...  Do you think we
> should make them numbers instead?

If they can overflow, then they should be numbers.  The other option would be to
change units to kb.

http://codereview.chromium.org/3801008/diff/34001/9009
File chrome/browser/gtk/task_manager_gtk.cc (right):

http://codereview.chromium.org/3801008/diff/34001/9009#newcode489
chrome/browser/gtk/task_manager_gtk.cc:489: // If the model already has
resources, we need to add them before we start
Does the windows code need a similar check?

http://codereview.chromium.org/3801008/diff/34001/9010
File chrome/browser/task_manager/task_manager.cc (right):

http://codereview.chromium.org/3801008/diff/34001/9010#newcode119
chrome/browser/task_manager/task_manager.cc:119: DCHECK(index <
ResourceCount());
same here

http://codereview.chromium.org/3801008/diff/34001/9010#newcode537
chrome/browser/task_manager/task_manager.cc:537: // Just in case it manages to
go negative in release builds, let's reset it.
I'd just change the above into a CHECK.  I agree with your assessment that this
shouldn't be possible.  So it's better to not cover up the error.

http://codereview.chromium.org/3801008/diff/34001/9012
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3801008/diff/34001/9012#newcode2639
chrome/common/extensions/api/extension_api.json:2639: "additionalProperties": {
"$ref": "Process" }
Shouldn't this just replace the whole block rather than being
additionalProperties?

[Charlie Reis, 2010-10-19 00:32:05]

New CL uploaded, which should take care of reporting the process type correctly.

http://codereview.chromium.org/3801008/diff/13032/23002
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3801008/diff/13032/23002#newcode83
chrome/browser/extensions/extension_processes_api.cc:83: }
On 2010/10/16 04:25:15, Erik Kay wrote:
> On 2010/10/15 21:59:21, creis wrote:
> > On 2010/10/15 18:40:34, Erik Kay wrote:
> > > how about an else for unknown types?  I suspect that this misses some like
> > > utility processes, the gpu process, etc.
> > 
> > Right now the "else" is plugin (see above).  Do you know any good ways to
> detect
> > those types of processes, or should we add some logic in a future CL?  (For
> now,
> > I've separated the block and added a TODO comment.)
> 
> Unfortunately I don't know how to detect this.  You'll need to do a little
> digging I guess.

I found an existing type field on ChildProcessInfo, but that only helped with
child processes and not the browser, renderer, extension, and notification
processes.  In this version, I've added a Type field to TaskManager::Resource
which is able to handle all the types.  It's a fairly verbose change (adding
task_manager_resource_provider.{h,cc} to the CL), but hopefully it will be
useful for other purposes as well.

http://codereview.chromium.org/3801008/diff/13032/23011
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3801008/diff/13032/23011#newcode2586
chrome/common/extensions/api/extension_api.json:2586: "type": "number",
On 2010/10/16 04:25:15, Erik Kay wrote:
> On 2010/10/15 21:59:21, creis wrote:
> > On 2010/10/15 18:40:34, Erik Kay wrote:
> > > do these really need to be numbers rather than integers?  do we care about
> > > fractions?  are the numbers ever going to overflow 32 bits?
> > 
> > CPU is a number because we report it to the tenth on Mac (to be consistent
> with
> > Activity Monitor).  Is there a good reason to round it off for extensions?
> > 
> > Network could be an integer.  It should really be described as bytes per
> second,
> > so I've fixed that.
> > 
> > I've changed the two memory metrics to integers as well, since that's what
> they
> > are in extension_processes_api.cc.  However, on a machine with 10+ GB of
RAM,
> > it's seems possible that memory could overflow 32 bits...  Do you think we
> > should make them numbers instead?
> 
> If they can overflow, then they should be numbers.  The other option would be
to
> change units to kb.

I don't see why we'd want to lose precision.  I think it makes sense to make
them numbers to handle upcoming machines with gobs of memory.

On that note, network should really be a number too, since 10 gigabit network
cards are out there.

http://codereview.chromium.org/3801008/diff/34001/9009
File chrome/browser/gtk/task_manager_gtk.cc (right):

http://codereview.chromium.org/3801008/diff/34001/9009#newcode489
chrome/browser/gtk/task_manager_gtk.cc:489: // If the model already has
resources, we need to add them before we start
On 2010/10/16 04:25:16, Erik Kay wrote:
> Does the windows code need a similar check?

Not in my testing.  The ExtensionApiTest.ProcessesVsTaskManager test actually
covers this, by starting the extension first and then opening the Task Manager. 
As a result, we get test coverage for this on all three platforms.  (There may
be other platform-specific assumptions I missed, but I haven't found any yet.)

http://codereview.chromium.org/3801008/diff/34001/9010
File chrome/browser/task_manager/task_manager.cc (right):

http://codereview.chromium.org/3801008/diff/34001/9010#newcode119
chrome/browser/task_manager/task_manager.cc:119: DCHECK(index <
ResourceCount());
On 2010/10/16 04:25:16, Erik Kay wrote:
> same here

Done.

http://codereview.chromium.org/3801008/diff/34001/9010#newcode537
chrome/browser/task_manager/task_manager.cc:537: // Just in case it manages to
go negative in release builds, let's reset it.
On 2010/10/16 04:25:16, Erik Kay wrote:
> I'd just change the above into a CHECK.  I agree with your assessment that
this
> shouldn't be possible.  So it's better to not cover up the error.

Done.

http://codereview.chromium.org/3801008/diff/34001/9012
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3801008/diff/34001/9012#newcode2639
chrome/common/extensions/api/extension_api.json:2639: "additionalProperties": {
"$ref": "Process" }
On 2010/10/16 04:25:16, Erik Kay wrote:
> Shouldn't this just replace the whole block rather than being
> additionalProperties?

I'm not sure what syntax you're proposing.  I need it to convey that we're
returning an object with an unknown number of properties, each of which is a
Process.  It shouldn't say we're returning a single Process object (or an object
with one property).  Is there a better syntax for that?

[Charlie Reis, 2010-10-19 01:16:03]

Just pushed patch set 6 to reconcile conflicts with Matt's CL from Friday.  The
code review tool makes it look like it touched a lot of files (because of a
gclient sync), but I only moved the add/remove of event listeners from
extension_message_service.cc to extension_event_router.cc and updated
extension_processes_api.cc accordingly.  Sound right, Matt?

[Matt Perry, 2010-10-19 01:23:17]

On 2010/10/19 01:16:03, creis wrote:
> Just pushed patch set 6 to reconcile conflicts with Matt's CL from Friday. 
The
> code review tool makes it look like it touched a lot of files (because of a
> gclient sync), but I only moved the add/remove of event listeners from
> extension_message_service.cc to extension_event_router.cc and updated
> extension_processes_api.cc accordingly.  Sound right, Matt?

Yep, that's all you should need to do.

[Charlie Reis, 2010-10-19 01:37:50]

Thanks.  One more minor change to task_manager_unittest.cc uploaded to fix a
compile error in unit_tests.

[Jay Civelli, 2010-10-19 17:24:13]

> Jay, can you confirm it's a good idea to change these DCHECKs to CHECKs in
> tash_manager.cc?
Looks reasonable.

Jay

[Erik does not do reviews, 2010-10-19 18:30:17]

LGTM

http://codereview.chromium.org/3801008/diff/60002/65002
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/3801008/diff/60002/65002#newcode70
chrome/browser/extensions/extension_event_router.cc:70: if
(event_name.compare(extension_processes_api_constants::kOnUpdated) == 0) {
nit: remove {} for one line blocks (and the next one).

http://codereview.chromium.org/3801008/diff/60002/65003
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3801008/diff/60002/65003#newcode80
chrome/browser/extensions/extension_processes_api.cc:80:
TaskManager::Resource::Type resource_type = model_->GetResourceType(i);
this looks great!

http://codereview.chromium.org/3801008/diff/60002/65003#newcode122
chrome/browser/extensions/extension_processes_api.cc:122: // TODO(creis):
Network is actually reported per-resource (tab),
this sounds like a potential incompatible change.  if you're going to leave it
as a TODO, you may need a plan for how you'd roll it out without breaking the
old one.

http://codereview.chromium.org/3801008/diff/60002/65003#newcode159
chrome/browser/extensions/extension_processes_api.cc:159: bool
GetProcessForTabFunction::RunImpl() {
rename to GetProcessId...?

http://codereview.chromium.org/3801008/diff/60002/65004
File chrome/browser/extensions/extension_processes_api.h (right):

http://codereview.chromium.org/3801008/diff/60002/65004#newcode39
chrome/browser/extensions/extension_processes_api.h:39: //
TaskManagerModelObserver::OnModelChanged.
replace these individual comment blocks with a single comment
"TaskManagerModelObserver methods" and remove newlines between methods.

[Charlie Reis, 2010-10-19 19:09:22]

Thanks!  Issues fixed.

http://codereview.chromium.org/3801008/diff/60002/65002
File chrome/browser/extensions/extension_event_router.cc (right):

http://codereview.chromium.org/3801008/diff/60002/65002#newcode70
chrome/browser/extensions/extension_event_router.cc:70: if
(event_name.compare(extension_processes_api_constants::kOnUpdated) == 0) {
On 2010/10/19 18:30:17, Erik Kay wrote:
> nit: remove {} for one line blocks (and the next one).

Done.

http://codereview.chromium.org/3801008/diff/60002/65003
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3801008/diff/60002/65003#newcode122
chrome/browser/extensions/extension_processes_api.cc:122: // TODO(creis):
Network is actually reported per-resource (tab),
On 2010/10/19 18:30:17, Erik Kay wrote:
> this sounds like a potential incompatible change.  if you're going to leave it
> as a TODO, you may need a plan for how you'd roll it out without breaking the
> old one.

Currently, extensions never get any network data for tabs that are not the first
tab of the process.  (No other metrics are reported per-tab rather than
per-process.  Well, ok,  goats teleported is the other one.)

My plan is to loop over the other tabs in the process and add up their network
values, reporting that instead.  There won't be any API change-- it'll just
start reporting a more accurate network value.  (I also plan to add an API for
the array of tabs being rendered by the process, which will be done in the same
CL.)

http://codereview.chromium.org/3801008/diff/60002/65003#newcode159
chrome/browser/extensions/extension_processes_api.cc:159: bool
GetProcessForTabFunction::RunImpl() {
On 2010/10/19 18:30:17, Erik Kay wrote:
> rename to GetProcessId...?

Ah, good catch.  Done.

http://codereview.chromium.org/3801008/diff/60002/65004
File chrome/browser/extensions/extension_processes_api.h (right):

http://codereview.chromium.org/3801008/diff/60002/65004#newcode39
chrome/browser/extensions/extension_processes_api.h:39: //
TaskManagerModelObserver::OnModelChanged.
On 2010/10/19 18:30:17, Erik Kay wrote:
> replace these individual comment blocks with a single comment
> "TaskManagerModelObserver methods" and remove newlines between methods.

Done.

[Erik does not do reviews, 2010-10-19 21:27:50]

LGTM!