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=62458

[Charlie Reis, 2010-10-07 21:05:09]

Here's a first cut at expanding the API.  There's a few things I still need to
fix, but I wanted to get feedback at this point.

Current issues:
 - Doesn't get updates if TaskManager isn't visible.
 - ExtensionApiTest.Processes hasn't been updated yet.
 - There should probably be a cleaner way to get the relevant Profile(s) in
OnItemsChanged, since the TaskManager affects all active Profiles.

I just sent an email to chromium-dev for discussing the proposed API changes.

[rafaelw, 2010-10-08 01:24:31]

FYI, I'm on vacation tomorrow. I can get to this (and the related thread) on
Monday. If this is time critical, I'm sure someone else on the extensions team
can take it. If not, I'll get to it Monday.

[Charlie Reis, 2010-10-08 01:31:41]

On 2010/10/08 01:24:31, rafaelw wrote:
> FYI, I'm on vacation tomorrow. I can get to this (and the related thread) on
> Monday. If this is time critical, I'm sure someone else on the extensions team
> can take it. If not, I'll get to it Monday.

Thanks for the heads up.  I've got enough feedback from the thread to make
progress for now, but I'll add another reviewer if I'm ready before Monday. 
Have fun!

[Charlie Reis, 2010-10-12 17:30:06]

Ok, this is ready for review now.  It gets the TaskManager to start and stop
updating whether an extension or the real task manager window triggers it.

I'll see if I can add another test for the new TaskManager behavior, and I'll
upload the updated extension doc files once we're ready.  I put a few comments
inline with questions and explanations.

http://codereview.chromium.org/3597016/diff/23001/24001
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24001#newcode196
chrome/browser/extensions/extension_message_service.cc:196: // TODO(creis):
Shouldn't we remove the listeners even if rph is gone?
I think these lines should be removed.  I can understand having them in
AddEventListener, but wouldn't we want to remove the listeners from our map if
the process is gone?

http://codereview.chromium.org/3597016/diff/23001/24007
File chrome/browser/gtk/task_manager_gtk.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24007#newcode490
chrome/browser/gtk/task_manager_gtk.cc:490: // observing events.
This fixes a crash if you open the task manager after an extension has already
called StartUpdating.  This GTK class needs to know that there are already
resources.

I haven't tested on Mac or Windows yet, so we might need something similar
there.  I'll see if there's a way to test it.

http://codereview.chromium.org/3597016/diff/23001/24008
File chrome/browser/task_manager/task_manager.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24008#newcode553
chrome/browser/task_manager/task_manager.cc:553: Clear();
Seems better to do this here than in OnWindowClosed (below), since it's always
necessary.  I ran into problems with extensions not clearing the model after
removing a listener.

http://codereview.chromium.org/3597016/diff/23001/24009
File chrome/browser/task_manager/task_manager.h (right):

http://codereview.chromium.org/3597016/diff/23001/24009#newcode208
chrome/browser/task_manager/task_manager.h:208: size_t
GetResourceRawV8MemoryAllocatedSize(int index) const;
I'll remove these last 5-- they're not being used yet, and they'll probably end
up looking more like the functions below (returning bool).

http://codereview.chromium.org/3597016/diff/23001/24009#newcode224
chrome/browser/task_manager/task_manager.h:224: 
These are just moved up from below, to make them public.

[rafaelw, 2010-10-12 21:44:05]

mostly looks good.

note that I didn't review task_manager* code (I'm assuming that's why Jay is on
this review).

Added mpcomplete. Matt, can you answer Charlie's question in
ExtensionMessageService. Also, is this going to interact properly with
incognito?

http://codereview.chromium.org/3597016/diff/23001/24010
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3597016/diff/23001/24010#newcode2579
chrome/common/extensions/api/extension_api.json:2579: "type": {
re: the discussion of some of these being unavailable on some platforms. What
about marking the ones that aren't available on all platforms as "optional", and
noting in the doc that it will only appears on x,y platforms?

http://codereview.chromium.org/3597016/diff/23001/24010#newcode2592
chrome/common/extensions/api/extension_api.json:2592: "networkString": {
this foo/fooString business feels a tad gross to me.  is the string part really
necessary? how hard is it to format these yourself?

[Jay Civelli, 2010-10-12 22:41:01]

LGTM for the task_manager files.

[Matt Perry, 2010-10-12 22:49:58]

http://codereview.chromium.org/3597016/diff/23001/24001
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24001#newcode196
chrome/browser/extensions/extension_message_service.cc:196: // TODO(creis):
Shouldn't we remove the listeners even if rph is gone?
On 2010/10/12 17:30:06, creis wrote:
> I think these lines should be removed.  I can understand having them in
> AddEventListener, but wouldn't we want to remove the listeners from our map if
> the process is gone?

The good news: it won't matter soon. I have a CL that removes them.

Is it causing a problem? It's only there because of the DCHECK. A dead RPH will
have already removed its listeners via a Notification below.

http://codereview.chromium.org/3597016/diff/23001/24002
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24002#newcode131
chrome/browser/extensions/extension_processes_api.cc:131: event_name, json_args,
profile, GURL());
This part is subtle. The profile parameter should be the profile that the event
is coming from. For example, if a new incognito process is created, we'd use the
incognito profile here - that prevents us from sharing that information with an
extension that isn't enabled in incognito.

OTOH, if these are "global" events (there's nothing specific to a profile and an
extension should see it regardless of whether it is incognito-enabled), pass
NULL for the profile.

http://codereview.chromium.org/3597016/diff/23001/24003
File chrome/browser/extensions/extension_processes_api.h (right):

http://codereview.chromium.org/3597016/diff/23001/24003#newcode60
chrome/browser/extensions/extension_processes_api.h:60: ProfileMap profiles_;
Is there a reason to use a map here instead of a set?

[Charlie Reis, 2010-10-13 01:16:54]

New CL uploaded.  The profile issue is the main remaining question.

(I also added a small change to api_test/permissions/enabled/background.html to
make it work with the new getProcessIdForTab API.)

Thanks,
Charlie

http://codereview.chromium.org/3597016/diff/23001/24001
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24001#newcode196
chrome/browser/extensions/extension_message_service.cc:196: // TODO(creis):
Shouldn't we remove the listeners even if rph is gone?
On 2010/10/12 22:49:58, Matt Perry wrote:
> On 2010/10/12 17:30:06, creis wrote:
> > I think these lines should be removed.  I can understand having them in
> > AddEventListener, but wouldn't we want to remove the listeners from our map
if
> > the process is gone?
> 
> The good news: it won't matter soon. I have a CL that removes them.
> 
> Is it causing a problem? It's only there because of the DCHECK. A dead RPH
will
> have already removed its listeners via a Notification below.

It was only a problem when I put my ListenerRemoved call below this code, since
the listeners wouldn't get removed when the process went away.  It works now
whether this code is there or not, so I'll leave it in place and wait for your
CL to remove it.

http://codereview.chromium.org/3597016/diff/23001/24002
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24002#newcode131
chrome/browser/extensions/extension_processes_api.cc:131: event_name, json_args,
profile, GURL());
On 2010/10/12 22:49:58, Matt Perry wrote:
> This part is subtle. The profile parameter should be the profile that the
event
> is coming from. For example, if a new incognito process is created, we'd use
the
> incognito profile here - that prevents us from sharing that information with
an
> extension that isn't enabled in incognito.
> 
> OTOH, if these are "global" events (there's nothing specific to a profile and
an
> extension should see it regardless of whether it is incognito-enabled), pass
> NULL for the profile.

I'm not sure which way to go on this.  It would seem strange to exclude
processes from a custom task manager extension just because they're in incognito
tabs.  On the flip side, the process objects returned in the onUpdated events
will eventually have tab IDs for each tab in the process-- how much are we
trying to hide the existence of incognito tabs from non-incognito extensions? 
(If they look up an incognito tab by ID, will they find it?  Is it harmful to
know the ID and the corresponding CPU/memory metrics?)

http://codereview.chromium.org/3597016/diff/23001/24003
File chrome/browser/extensions/extension_processes_api.h (right):

http://codereview.chromium.org/3597016/diff/23001/24003#newcode60
chrome/browser/extensions/extension_processes_api.h:60: ProfileMap profiles_;
On 2010/10/12 22:49:58, Matt Perry wrote:
> Is there a reason to use a map here instead of a set?

I picked it up from extension_history_api.h and then wasn't sure how to handle
the Profiles in this case.  I can switch it to a set, but I'm guessing there's
more work to be done on how this handles profiles.

http://codereview.chromium.org/3597016/diff/23001/24009
File chrome/browser/task_manager/task_manager.h (right):

http://codereview.chromium.org/3597016/diff/23001/24009#newcode208
chrome/browser/task_manager/task_manager.h:208: size_t
GetResourceRawV8MemoryAllocatedSize(int index) const;
On 2010/10/12 17:30:06, creis wrote:
> I'll remove these last 5-- they're not being used yet, and they'll probably
end
> up looking more like the functions below (returning bool).

Done.

http://codereview.chromium.org/3597016/diff/23001/24010
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/3597016/diff/23001/24010#newcode2579
chrome/common/extensions/api/extension_api.json:2579: "type": {
On 2010/10/12 21:44:05, rafaelw wrote:
> re: the discussion of some of these being unavailable on some platforms. What
> about marking the ones that aren't available on all platforms as "optional",
and
> noting in the doc that it will only appears on x,y platforms?

To be specific, this would apply to Shared and Private Memory (which are
currently only on Windows and Linux) and Memory (which is only available on
Windows and Mac, and will likely stay that way).  The rest are cross-platform,
assuming we don't expose the number of goats teleported.  :)

Since shared and private memory will be available on Mac once
http://crbug.com/25454 is fixed, I don't think we need to make that optional in
the API.  I'm also a little hesitant to expose Memory as an optional property
that works on Windows and Mac-- on one hand, it's another metric that might be
useful to developers, but on the other, it seems likely to lead to extensions
that break on Linux because they weren't tested there.  Seems better to push
people toward the metrics that work cross-platform, but let me know if you feel
otherwise.

http://codereview.chromium.org/3597016/diff/23001/24010#newcode2592
chrome/common/extensions/api/extension_api.json:2592: "networkString": {
On 2010/10/12 21:44:05, rafaelw wrote:
> this foo/fooString business feels a tad gross to me.  is the string part
really
> necessary? how hard is it to format these yourself?

It'd be easy to convert them to the units you want, but a bit trickier to format
them nicely with commas, etc.  That's not a big deal, though, and there are code
snippets out there that will do it for you.  

I've removed the string versions to clean up the API, and we can come up with
something better if there's demand for it.  In the mean time, I've shown how to
do some of the formatting in popup.html.

[Matt Perry, 2010-10-13 01:50:26]

http://codereview.chromium.org/3597016/diff/23001/24001
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24001#newcode196
chrome/browser/extensions/extension_message_service.cc:196: // TODO(creis):
Shouldn't we remove the listeners even if rph is gone?
On 2010/10/13 01:16:54, creis wrote:
> On 2010/10/12 22:49:58, Matt Perry wrote:
> > On 2010/10/12 17:30:06, creis wrote:
> > > I think these lines should be removed.  I can understand having them in
> > > AddEventListener, but wouldn't we want to remove the listeners from our
map
> if
> > > the process is gone?
> > 
> > The good news: it won't matter soon. I have a CL that removes them.
> > 
> > Is it causing a problem? It's only there because of the DCHECK. A dead RPH
> will
> > have already removed its listeners via a Notification below.
> 
> It was only a problem when I put my ListenerRemoved call below this code,
since
> the listeners wouldn't get removed when the process went away.  It works now
> whether this code is there or not, so I'll leave it in place and wait for your
> CL to remove it.

Do you have a test that will fail if this regresses? I'm essentially moving this
check further up the call chain, to BrowserRenderProcessHost::OnMessageReceived.
Not sure if that will still break, since RemoveEventListener is still called by
Observe when a process dies.

http://codereview.chromium.org/3597016/diff/23001/24002
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24002#newcode131
chrome/browser/extensions/extension_processes_api.cc:131: event_name, json_args,
profile, GURL());
On 2010/10/13 01:16:54, creis wrote:
> On 2010/10/12 22:49:58, Matt Perry wrote:
> > This part is subtle. The profile parameter should be the profile that the
> event
> > is coming from. For example, if a new incognito process is created, we'd use
> the
> > incognito profile here - that prevents us from sharing that information with
> an
> > extension that isn't enabled in incognito.
> > 
> > OTOH, if these are "global" events (there's nothing specific to a profile
and
> an
> > extension should see it regardless of whether it is incognito-enabled), pass
> > NULL for the profile.
> 
> I'm not sure which way to go on this.  It would seem strange to exclude
> processes from a custom task manager extension just because they're in
incognito
> tabs.  On the flip side, the process objects returned in the onUpdated events
> will eventually have tab IDs for each tab in the process-- how much are we
> trying to hide the existence of incognito tabs from non-incognito extensions? 
> (If they look up an incognito tab by ID, will they find it?  Is it harmful to
> know the ID and the corresponding CPU/memory metrics?)

Hmm.. my take is that these are global events. I'm only worried about sensitive
data like URLs being accidentally saved to disk, not the mere existence of an
incognito window. However, it would be weird to pass down a tabID that the
extension has no access to (to the extension it looks indistinguishable from a
closed or non-existent tab). Maybe the tabID should only be passed down if the
extension has access? But that's tricky to do...

For now, I would say just pass NULL for the profile. As for the tabIDs, we'll
burn that bridge when we get to it.

http://codereview.chromium.org/3597016/diff/23001/24003
File chrome/browser/extensions/extension_processes_api.h (right):

http://codereview.chromium.org/3597016/diff/23001/24003#newcode60
chrome/browser/extensions/extension_processes_api.h:60: ProfileMap profiles_;
On 2010/10/13 01:16:54, creis wrote:
> On 2010/10/12 22:49:58, Matt Perry wrote:
> > Is there a reason to use a map here instead of a set?
> 
> I picked it up from extension_history_api.h and then wasn't sure how to handle
> the Profiles in this case.  I can switch it to a set, but I'm guessing there's
> more work to be done on how this handles profiles.

I think that's a mistake in that file then, because this is essentially a map of
(Profile* -> Profile*), which doesn't buy you anything over a set.

[Charlie Reis, 2010-10-13 18:01:39]

Now that we're getting close, I've added in the updated extension docs (after
running docs/build/build.py).  

Also, the new ExtensionApiTest.ProcessesVsTaskManager should reveal whether
anything in the Windows and Mac versions of the task manager need updating.

http://codereview.chromium.org/3597016/diff/23001/24001
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24001#newcode196
chrome/browser/extensions/extension_message_service.cc:196: // TODO(creis):
Shouldn't we remove the listeners even if rph is gone?
On 2010/10/13 01:50:27, Matt Perry wrote:
> On 2010/10/13 01:16:54, creis wrote:
> > On 2010/10/12 22:49:58, Matt Perry wrote:
> > > On 2010/10/12 17:30:06, creis wrote:
> > > > I think these lines should be removed.  I can understand having them in
> > > > AddEventListener, but wouldn't we want to remove the listeners from our
> map
> > if
> > > > the process is gone?
> > > 
> > > The good news: it won't matter soon. I have a CL that removes them.
> > > 
> > > Is it causing a problem? It's only there because of the DCHECK. A dead RPH
> > will
> > > have already removed its listeners via a Notification below.
> > 
> > It was only a problem when I put my ListenerRemoved call below this code,
> since
> > the listeners wouldn't get removed when the process went away.  It works now
> > whether this code is there or not, so I'll leave it in place and wait for
your
> > CL to remove it.
> 
> Do you have a test that will fail if this regresses? I'm essentially moving
this
> check further up the call chain, to
BrowserRenderProcessHost::OnMessageReceived.
> Not sure if that will still break, since RemoveEventListener is still called
by
> Observe when a process dies.

As long as this function gets called, we should be fine.  Either way, I've added
a test for it in ExtensionApiTest.ProcessesVsTaskManager.

http://codereview.chromium.org/3597016/diff/23001/24002
File chrome/browser/extensions/extension_processes_api.cc (right):

http://codereview.chromium.org/3597016/diff/23001/24002#newcode131
chrome/browser/extensions/extension_processes_api.cc:131: event_name, json_args,
profile, GURL());
On 2010/10/13 01:50:27, Matt Perry wrote:
> On 2010/10/13 01:16:54, creis wrote:
> > On 2010/10/12 22:49:58, Matt Perry wrote:
> > > This part is subtle. The profile parameter should be the profile that the
> > event
> > > is coming from. For example, if a new incognito process is created, we'd
use
> > the
> > > incognito profile here - that prevents us from sharing that information
with
> > an
> > > extension that isn't enabled in incognito.
> > > 
> > > OTOH, if these are "global" events (there's nothing specific to a profile
> and
> > an
> > > extension should see it regardless of whether it is incognito-enabled),
pass
> > > NULL for the profile.
> > 
> > I'm not sure which way to go on this.  It would seem strange to exclude
> > processes from a custom task manager extension just because they're in
> incognito
> > tabs.  On the flip side, the process objects returned in the onUpdated
events
> > will eventually have tab IDs for each tab in the process-- how much are we
> > trying to hide the existence of incognito tabs from non-incognito
extensions? 
> > (If they look up an incognito tab by ID, will they find it?  Is it harmful
to
> > know the ID and the corresponding CPU/memory metrics?)
> 
> Hmm.. my take is that these are global events. I'm only worried about
sensitive
> data like URLs being accidentally saved to disk, not the mere existence of an
> incognito window. However, it would be weird to pass down a tabID that the
> extension has no access to (to the extension it looks indistinguishable from a
> closed or non-existent tab). Maybe the tabID should only be passed down if the
> extension has access? But that's tricky to do...
> 
> For now, I would say just pass NULL for the profile. As for the tabIDs, we'll
> burn that bridge when we get to it.

Done.

[rafaelw, 2010-10-13 18:09:01]

lgtm

[Matt Perry, 2010-10-13 18:21:30]

LGTM for my parts.