Improving the process model extension API

Improving the process model extension API

BUG=32302
TEST=Unit tests.


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

[nasko, 2012-04-23 20:43:23]

Charlie, can you review the initial version of the experimental.processes API
revival?

[Charlie Reis, 2012-04-23 22:19:51]

I'm excited!  These will be quite useful.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:134: // processes.onUpdated
event arrive.
nit: Update comment here and below.  We might consider a helper function for
this to avoid bugs if someone changes one but not the other.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:120:
CreateCacheData(cache_stats.images));
nit: Should line up with open paren.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:163:
model_->StartUpdating();
Why is this needed without any listeners?

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:207: bool
ExtensionProcessesEventRouter::HasEventListeners(std::string& event_name) {
There's a fair amount going on here.  Maybe add a comment saying that we have to
look at all extensions in all profiles to make it easier to read?  

Also, is all that work really necessary?  Would it be easier to keep a count of
how many listeners we have per event (in a map)?  I'm concerned about how often
we call this, and how many extensions it might have to loop through, when most
of them will say no.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:217: for
(ExtensionSet::const_iterator it = extensions->begin();
FYI, this may be changing soon, so be aware of potential merge conflicts.  See:
https://chromiumcodereview.appspot.com/10113005/

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:272: // the Task Manager
data.
But we do use the Task Manager's ToBeRemoved for non-renderer processes?  Maybe
add that to the comment.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:286: // Third arg: The exit
code for the process.
Thanks for documenting these.  Is it not possible for them to be non-zero, or is
this just a simplification because we don't have access to the real exit
types/codes?

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:330: // Store each process
indexed by the string version of its id
nit: End with period, here and below.  (Yep, probably my fault.)

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:347:
AddMemoryDetails(it.GetCurrentValue(), model_, it.GetCurrentKey());
Hmm.  I was thinking we could combine the two loops and just put the
"updated_memory" conditional on this line, but I suppose that would make
onUpdated listeners hear memory stats if anyone happened to be listening to
memory...

I still wonder if there's some redundancy here.  For example, it looks like we
don't need to do the processes->Set(...) call if "updated" is true, since we
already did it above.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:360: void
ExtensionProcessesEventRouter::ProcessHangEvent(
Should we skip this if no one is listening, like OnItemsChanged?  Or is that not
necessary here?

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:482: true);
Given our recent experiences, I wonder if we want to add a UMA histogram for
this so that we don't think Chrome is horribly broken if extensions start using
this a lot.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:489: // integer parameters
as input from extensions.
Sounds useful.  Is there any shared place we can put this to avoid duplicating
it?

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/extension_processes_api.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.h:61: // It helps to avoid
collecing data if noone is interested in it.
Nit: noone -> no one (here and below)

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.h:98: // * guards against
killing non-Chrome processes
Nice.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/extension_processes_api_constants.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api_constants.cc:16: const char
kOsProcessId[] = "osProcessId";
Each group should be alphabetical.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/task...
File chrome/browser/task_manager/task_manager.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/task...
chrome/browser/task_manager/task_manager.h:255: // Invoked when a range of items
is to be immediately removed.
Worthwhile to add a comment saying why this is useful vs OnItemsRemoved (i.e.,
the resource at that index can still be queried).

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/task...
chrome/browser/task_manager/task_manager.h:256: virtual void
OnItemsToBeRemoved(int start, int length) = 0;
Should we make this an empty implementation (like OnReadyPeriodicalUpdate below)
rather than pure virtual?  That would save some changes in the other TaskManager
files.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
File chrome/common/extensions/api/experimental.processes.json (left):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/api/experimental.processes.json:26: "description": "The
most recent measurement of the process's CPU usage, between 0 and 100%."
This seems more grammatically correct to me, but I don't feel strongly.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
File chrome/common/extensions/api/experimental.processes.json (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/api/experimental.processes.json:41: "optional": true,
Remind me, which cases don't we report cpu?

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/api/experimental.processes.json:77: "description": "The
most recent measurement of the process frames per section."
frames per second

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/api/experimental.processes.json:195: "description": "",
Description?

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/api/experimental.processes.json:239: "description":
"Fired each time the Task Manager updates its process statistics, providing the
dictionary of updated Process objects, indexed by process ID. Identical to
onUpdate, with the addition of memory usage details included in each Process
object.",
Should we note the implications for CPU overhead?

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
File
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js
(right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js:7:
chrome.experimental.processes.onUpdated.addListener(function(processes) {
Is there anything else we can display here from your additions?

[nasko, 2012-04-24 18:14:29]

I've made changes based on the review. I also realized some of the code was
stashed and not present in the initial version, so it is not added. There is a
set of "if defined", since the task manager seems to have been disabled on
android, so I've extended the initial change to include my new code.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/extension_event_router.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_event_router.cc:134: // processes.onUpdated
event arrive.
On 2012/04/23 22:19:51, creis wrote:
> nit: Update comment here and below.  We might consider a helper function for
> this to avoid bugs if someone changes one but not the other.

I don't think it is worthwhile adding a helper function now for this. If the
extension subsystem allows us easy testing for listeners in the future (bug
124804 filed), then this can even be removed.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:120:
CreateCacheData(cache_stats.images));
On 2012/04/23 22:19:51, creis wrote:
> nit: Should line up with open paren.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:163:
model_->StartUpdating();
On 2012/04/23 22:19:51, creis wrote:
> Why is this needed without any listeners?

The reason is that we can return CPU/Network stats from the  getProcessInfo
function.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:207: bool
ExtensionProcessesEventRouter::HasEventListeners(std::string& event_name) {
On 2012/04/23 22:19:51, creis wrote:
> There's a fair amount going on here.  Maybe add a comment saying that we have
to
> look at all extensions in all profiles to make it easier to read?  
> 
> Also, is all that work really necessary?  Would it be easier to keep a count
of
> how many listeners we have per event (in a map)?  I'm concerned about how
often
> we call this, and how many extensions it might have to loop through, when most
> of them will say no.

Added a comment and filed crbug.com/124804 to get a better API for doing this.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:217: for
(ExtensionSet::const_iterator it = extensions->begin();
On 2012/04/23 22:19:51, creis wrote:
> FYI, this may be changing soon, so be aware of potential merge conflicts. 
See:
> https://chromiumcodereview.appspot.com/10113005/

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:272: // the Task Manager
data.
On 2012/04/23 22:19:51, creis wrote:
> But we do use the Task Manager's ToBeRemoved for non-renderer processes? 
Maybe
> add that to the comment.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:286: // Third arg: The exit
code for the process.
On 2012/04/23 22:19:51, creis wrote:
> Thanks for documenting these.  Is it not possible for them to be non-zero, or
is
> this just a simplification because we don't have access to the real exit
> types/codes?

It is just a simplification, since we don't have the data. I could omit them,
but that would mean that extension developers will might need to explicitly test
for them.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:330: // Store each process
indexed by the string version of its id
On 2012/04/23 22:19:51, creis wrote:
> nit: End with period, here and below.  (Yep, probably my fault.)

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:347:
AddMemoryDetails(it.GetCurrentValue(), model_, it.GetCurrentKey());
On 2012/04/23 22:19:51, creis wrote:
> Hmm.  I was thinking we could combine the two loops and just put the
> "updated_memory" conditional on this line, but I suppose that would make
> onUpdated listeners hear memory stats if anyone happened to be listening to
> memory...
> 
> I still wonder if there's some redundancy here.  For example, it looks like we
> don't need to do the processes->Set(...) call if "updated" is true, since we
> already did it above.

As we discussed verbally, the goal is to keep the external API consistent, so we
won't be providing memory to onUpdated listeners. 

Yes, we can skip the processes->Set() call, so I've added this.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:360: void
ExtensionProcessesEventRouter::ProcessHangEvent(
On 2012/04/23 22:19:51, creis wrote:
> Should we skip this if no one is listening, like OnItemsChanged?  Or is that
not
> necessary here?

Good catch!

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:482: true);
On 2012/04/23 22:19:51, creis wrote:
> Given our recent experiences, I wonder if we want to add a UMA histogram for
> this so that we don't think Chrome is horribly broken if extensions start
using
> this a lot.

This is a good idea. I would suggest we add it in a separate CL though, along
with the actual histogram definition for it.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:489: // integer parameters
as input from extensions.
On 2012/04/23 22:19:51, creis wrote:
> Sounds useful.  Is there any shared place we can put this to avoid duplicating
> it?

Not quite sure. I will look around to see if there is a good common place this
can be moved to.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/extension_processes_api.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.h:61: // It helps to avoid
collecing data if noone is interested in it.
On 2012/04/23 22:19:51, creis wrote:
> Nit: noone -> no one (here and below)

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.h:98: // * guards against
killing non-Chrome processes
On 2012/04/23 22:19:51, creis wrote:
> Nice.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/extension_processes_api_constants.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api_constants.cc:16: const char
kOsProcessId[] = "osProcessId";
On 2012/04/23 22:19:51, creis wrote:
> Each group should be alphabetical.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/task...
File chrome/browser/task_manager/task_manager.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/task...
chrome/browser/task_manager/task_manager.h:255: // Invoked when a range of items
is to be immediately removed.
On 2012/04/23 22:19:51, creis wrote:
> Worthwhile to add a comment saying why this is useful vs OnItemsRemoved (i.e.,
> the resource at that index can still be queried).

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/task...
chrome/browser/task_manager/task_manager.h:256: virtual void
OnItemsToBeRemoved(int start, int length) = 0;
On 2012/04/23 22:19:51, creis wrote:
> Should we make this an empty implementation (like OnReadyPeriodicalUpdate
below)
> rather than pure virtual?  That would save some changes in the other
TaskManager
> files.

This is a good point. I've made it non-virtual with empty implementation.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
File chrome/common/extensions/api/experimental.processes.json (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/api/experimental.processes.json:41: "optional": true,
On 2012/04/23 22:19:51, creis wrote:
> Remind me, which cases don't we report cpu?

This has remained from the earlier time when getProcessInfo was not returning
periodically updated values, since we didn't run the TM update thread all the
time. I will remove the optional bit.

The same is true for the osProcessId, profile, tabs, and network.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/api/experimental.processes.json:77: "description": "The
most recent measurement of the process frames per section."
On 2012/04/23 22:19:51, creis wrote:
> frames per second

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/api/experimental.processes.json:195: "description": "",
On 2012/04/23 22:19:51, creis wrote:
> Description?

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/api/experimental.processes.json:239: "description":
"Fired each time the Task Manager updates its process statistics, providing the
dictionary of updated Process objects, indexed by process ID. Identical to
onUpdate, with the addition of memory usage details included in each Process
object.",
On 2012/04/23 22:19:51, creis wrote:
> Should we note the implications for CPU overhead?

Done.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
File
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js
(right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js:7:
chrome.experimental.processes.onUpdated.addListener(function(processes) {
On 2012/04/23 22:19:51, creis wrote:
> Is there anything else we can display here from your additions?

We could, but considering this is just a demo, I didn't think we need to list
every possible property. If you believe it will add to the sample, let me know.

[nasko, 2012-04-27 00:06:25]

I've changed the API to work differently - the task manager is no longer
updating all the time. I've split the registering of notification listeners from
the actual refresh of data. When the first API function is called, it will call
the task manager to start listening to events. To accomplish this, I've moved
all the API functions to use internally an async model and once the task manager
is done with the starting to listen and enumerating all resources, it sends a
notification which the API listens to.

[Charlie Reis, 2012-04-27 22:01:55]

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:482: true);
On 2012/04/24 18:14:29, nasko wrote:
> On 2012/04/23 22:19:51, creis wrote:
> > Given our recent experiences, I wonder if we want to add a UMA histogram for
> > this so that we don't think Chrome is horribly broken if extensions start
> using
> > this a lot.
> 
> This is a good idea. I would suggest we add it in a separate CL though, along
> with the actual histogram definition for it.

I found out that the timing of the histogram CL doesn't matter, so we might as
well just include it here.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/browser/exte...
chrome/browser/extensions/extension_processes_api.cc:489: // integer parameters
as input from extensions.
On 2012/04/24 18:14:29, nasko wrote:
> On 2012/04/23 22:19:51, creis wrote:
> > Sounds useful.  Is there any shared place we can put this to avoid
duplicating
> > it?
> 
> Not quite sure. I will look around to see if there is a good common place this
> can be moved to.

extension_tab_util.h, perhaps?

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
File
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js
(right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js:7:
chrome.experimental.processes.onUpdated.addListener(function(processes) {
On 2012/04/24 18:14:29, nasko wrote:
> On 2012/04/23 22:19:51, creis wrote:
> > Is there anything else we can display here from your additions?
> 
> We could, but considering this is just a demo, I didn't think we need to list
> every possible property. If you believe it will add to the sample, let me
know.

I think it might be worthwhile, as a demo to extension writers of what's
possible.  Feel free to push back if it gets unwieldy, though.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:18: #include
"chrome/browser/extensions/extension_service.h"
Nit: this goes at line 16.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:117: continue;
nit: 2 space indent

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:133: DictionaryValue*
CreateProcessFromModel(int process_id,
Let's put a comment on this one, since it's kind of subtle.  It returns a
Process object with all non-optional data.  It also includes any optional
onUpdated* stats if there are listeners present, but it never includes memory,
which must be added using AddMemoryDetails.

Side note: I think we might want to pass in a boolean saying whether to add the
onUpdated* stats or not, so that the policy (i.e., looking for any listeners) is
external to this otherwise simple helper function.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:270:
model_->StartUpdating();
These calls to StartUpdating and StopUpdating look odd next to the refcount
here, since it isn't obvious that they have their own internal refcount as well
(which might count other things besides extension listeners).  Maybe mention
that in a comment.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:326: DCHECK(length == 1);
DCHECK_EQ(1, length)

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:341:
model_->GetUniqueChildProcessId(index), model_, index);
Here's a reason to pass in the policy of whether or not to include the
onUpdated* stats as a boolean flag.  It should be false here, or else OnCreated
will return different amounts of information based on whether you also have an
onUpdated listener or not.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:472: process =
CreateProcessFromModel(id, model_, i);
Ditto.  We would want to pass in false here.  (I suppose it's a bit unfortunate
not to have CPU info, but they can listen to onUpdated if they want that.)

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:478: DCHECK(process !=
NULL);
DCHECK(process)

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:549: if
(router->ExtensionHasEventListener(extension_id, event_name))
Why is this part necessary if router->HasEventListener(event_name) returned
true?  We don't seem to need the specific extension, and this just seems to be
verifying what we already know.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:562: AddRef();
What happens if an extension calls this twice in a row, before the TaskManager
has had a chance to start listening?  Should we make it a no-op?

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:611: // Balance the AddRef
in the RunImpl
nit: End with period.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:38: bool
StartTaskManagerListening();
Let's try to make the return value a little more obvious by calling this
DidStartTaskManagerListening.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:40: bool
task_manager_listening();
is_task_manager_listening()

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:41: int listeners();
num_listeners().  These can be implemented inline if they're just one-liners.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:102: private:
Do these need explicit public constructors?  I haven't checked what the other
extension modules do.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api_constants.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api_constants.h:31: extern const
char kCssCache[];
This list still isn't alphabetical.  Is it in some other order?  Doesn't seem to
match the order in the API docs.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/tas...
File chrome/browser/task_manager/task_manager.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/tas...
chrome/browser/task_manager/task_manager.h:410: void StartListening();
These should have comments to explain what they're for and how they're different
from Start/StopUpdating.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/tas...
chrome/browser/task_manager/task_manager.h:446: enum ListenState {
Why not a is_listeneing_ boolean?  With UpdateState, they needed 3 values.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/common/exte...
File chrome/common/extensions/api/experimental.processes.json (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:16: "description":
"Unique ID of the process provided by Chrome"
Nit: end with period.

Note: we'll want to make a pass over this whole file to have it match our
proposed API doc, but that can wait until we're closer to finished.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:108: "id":
"MemoryUsage",
Do we want this in here?  We're not using it yet.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/test/data/e...
File chrome/test/data/extensions/api_test/processes/api/test.js (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/test/data/e...
chrome/test/data/extensions/api_test/processes/api/test.js:64:
assertTrue("network" in process);
What about things like sqliteMemory?  We should be exhaustive here to avoid
regressions.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/test/data/e...
chrome/test/data/extensions/api_test/processes/api/test.js:75:
assertTrue("network" in process);
Could just call validateProcessPropertiesOnUpdate, since this should be a
superset of that.  (We can move the assertFalse(privateMemory) outside of that
one, or use a boolean flag to say whether we want it to be present or not.)

[nasko, 2012-04-30 18:05:19]

Updated with changes based on review feedback.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:38: bool
StartTaskManagerListening();
On 2012/04/27 22:01:56, creis wrote:
> Let's try to make the return value a little more obvious by calling this
> DidStartTaskManagerListening.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:40: bool
task_manager_listening();
On 2012/04/27 22:01:56, creis wrote:
> is_task_manager_listening()

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:41: int listeners();
On 2012/04/27 22:01:56, creis wrote:
> num_listeners().  These can be implemented inline if they're just one-liners.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:102: private:
On 2012/04/27 22:01:56, creis wrote:
> Do these need explicit public constructors?  I haven't checked what the other
> extension modules do.

I've used the tabs extension as a model and they have the same construct. Look
up DetectTabLanguageFunction or CaptureVisibleTabFunction.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api_constants.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api_constants.h:31: extern const
char kCssCache[];
On 2012/04/27 22:01:56, creis wrote:
> This list still isn't alphabetical.  Is it in some other order?  Doesn't seem
to
> match the order in the API docs.

Missed in a stash. Fixed now.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/tas...
File chrome/browser/task_manager/task_manager.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/tas...
chrome/browser/task_manager/task_manager.h:410: void StartListening();
On 2012/04/27 22:01:56, creis wrote:
> These should have comments to explain what they're for and how they're
different
> from Start/StopUpdating.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/tas...
chrome/browser/task_manager/task_manager.h:446: enum ListenState {
On 2012/04/27 22:01:56, creis wrote:
> Why not a is_listeneing_ boolean?  With UpdateState, they needed 3 values.

I initially started with different states. You are right that we can do it with
a boolean. Changed.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/common/exte...
File chrome/common/extensions/api/experimental.processes.json (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:16: "description":
"Unique ID of the process provided by Chrome"
On 2012/04/27 22:01:56, creis wrote:
> Nit: end with period.
> 
> Note: we'll want to make a pass over this whole file to have it match our
> proposed API doc, but that can wait until we're closer to finished.

Yes, I agree and the reason I didn't make a real effort on the descriptions was
waiting for the doc to solidify.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:108: "id":
"MemoryUsage",
On 2012/04/27 22:01:56, creis wrote:
> Do we want this in here?  We're not using it yet.

I've removed it.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/test/data/e...
File chrome/test/data/extensions/api_test/processes/api/test.js (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/test/data/e...
chrome/test/data/extensions/api_test/processes/api/test.js:64:
assertTrue("network" in process);
On 2012/04/27 22:01:56, creis wrote:
> What about things like sqliteMemory?  We should be exhaustive here to avoid
> regressions.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/test/data/e...
chrome/test/data/extensions/api_test/processes/api/test.js:75:
assertTrue("network" in process);
On 2012/04/27 22:01:56, creis wrote:
> Could just call validateProcessPropertiesOnUpdate, since this should be a
> superset of that.  (We can move the assertFalse(privateMemory) outside of that
> one, or use a boolean flag to say whether we want it to be present or not.)

I've rewritten the whole process validation.

[Charlie Reis, 2012-05-01 17:58:24]

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:18: #include
"chrome/browser/extensions/extension_service.h"
On 2012/04/27 22:01:56, creis wrote:
> Nit: this goes at line 16.

Looks like none of the comments in this file have been addressed.  I'll hold off
on further review until you have a new patch up.

https://chromiumcodereview.appspot.com/10175008/diff/67002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/67002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:40: bool
is_task_manager_listening();
Please implement these two inline, since they're just one-liners.

[nasko, 2012-05-01 21:28:40]

Updated with the missed file and moving common functionality to
ExtensionTabUtil.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:18: #include
"chrome/browser/extensions/extension_service.h"
On 2012/04/27 22:01:56, creis wrote:
> Nit: this goes at line 16.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:18: #include
"chrome/browser/extensions/extension_service.h"
On 2012/04/27 22:01:56, creis wrote:
> Nit: this goes at line 16.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:117: continue;
On 2012/04/27 22:01:56, creis wrote:
> nit: 2 space indent

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:133: DictionaryValue*
CreateProcessFromModel(int process_id,
On 2012/04/27 22:01:56, creis wrote:
> Let's put a comment on this one, since it's kind of subtle.  It returns a
> Process object with all non-optional data.  It also includes any optional
> onUpdated* stats if there are listeners present, but it never includes memory,
> which must be added using AddMemoryDetails.
> 
> Side note: I think we might want to pass in a boolean saying whether to add
the
> onUpdated* stats or not, so that the policy (i.e., looking for any listeners)
is
> external to this otherwise simple helper function.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:270:
model_->StartUpdating();
On 2012/04/27 22:01:56, creis wrote:
> These calls to StartUpdating and StopUpdating look odd next to the refcount
> here, since it isn't obvious that they have their own internal refcount as
well
> (which might count other things besides extension listeners).  Maybe mention
> that in a comment.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:326: DCHECK(length == 1);
On 2012/04/27 22:01:56, creis wrote:
> DCHECK_EQ(1, length)

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:341:
model_->GetUniqueChildProcessId(index), model_, index);
On 2012/04/27 22:01:56, creis wrote:
> Here's a reason to pass in the policy of whether or not to include the
> onUpdated* stats as a boolean flag.  It should be false here, or else
OnCreated
> will return different amounts of information based on whether you also have an
> onUpdated listener or not.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:472: process =
CreateProcessFromModel(id, model_, i);
On 2012/04/27 22:01:56, creis wrote:
> Ditto.  We would want to pass in false here.  (I suppose it's a bit
unfortunate
> not to have CPU info, but they can listen to onUpdated if they want that.)

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:478: DCHECK(process !=
NULL);
On 2012/04/27 22:01:56, creis wrote:
> DCHECK(process)

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:549: if
(router->ExtensionHasEventListener(extension_id, event_name))
On 2012/04/27 22:01:56, creis wrote:
> Why is this part necessary if router->HasEventListener(event_name) returned
> true?  We don't seem to need the specific extension, and this just seems to be
> verifying what we already know.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:562: AddRef();
On 2012/04/27 22:01:56, creis wrote:
> What happens if an extension calls this twice in a row, before the TaskManager
> has had a chance to start listening?  Should we make it a no-op?

We can't really be no-op, we need to still call the callback with the results,
so we need to go in either of the branches below. Adding more logic to pick one
vs other is not really worthwhile, especially when the Task Manager is doing a
no-op on StartListening anyway.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:611: // Balance the AddRef
in the RunImpl
On 2012/04/27 22:01:56, creis wrote:
> nit: End with period.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/54001/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:611: // Balance the AddRef
in the RunImpl
On 2012/04/27 22:01:56, creis wrote:
> nit: End with period.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/67002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/67002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:40: bool
is_task_manager_listening();
On 2012/05/01 17:58:25, creis wrote:
> Please implement these two inline, since they're just one-liners.

Done.

[Charlie Reis, 2012-05-02 00:23:13]

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/browser_extension_window_controller.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/browser_extension_window_controller.cc:7: #include
"base/values.h"
Unneeded?

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:210: // Borrowed from the
tabs extension. Allows us to support both list and single
This is in tabs_util.h now, so we should remove this copy.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:260:
model_->StopListening();
Do we also need to set task_manager_listening_ to false?

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:661: killed =
base::KillProcess(model->GetProcess(i),
I still think we should have a histogram for this in this CL, probably using
UMA_HISTOGRAM_COUNTS(..., 1);

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:91: // done once for the
lifetime of this object.
I'm a little confused.  It seems like we can stop listening, so we'll need to be
able to turn this off and back on again (unlike the comment says).

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api_constants.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api_constants.cc:52: const char
kExtensionNotSupported[] = "The extension API is not supported.";
Should this be specific to the processes extension API?

[nasko, 2012-05-02 18:14:02]

Added the histogram and removed the unneeded code. Also, some clarification
comments to your questions.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/browser_extension_window_controller.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/browser_extension_window_controller.cc:7: #include
"base/values.h"
On 2012/05/02 00:23:13, creis wrote:
> Unneeded?

Needed, because of the added function to extension_tab_util.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:210: // Borrowed from the
tabs extension. Allows us to support both list and single
On 2012/05/02 00:23:13, creis wrote:
> This is in tabs_util.h now, so we should remove this copy.

Doh! Removed.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:260:
model_->StopListening();
On 2012/05/02 00:23:13, creis wrote:
> Do we also need to set task_manager_listening_ to false?

This is the destructor, right? The object will go away and if created again the
constructor will set it to false. Also, this is a singleton with lifetime bound
to the browser process.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:661: killed =
base::KillProcess(model->GetProcess(i),
On 2012/05/02 00:23:13, creis wrote:
> I still think we should have a histogram for this in this CL, probably using
> UMA_HISTOGRAM_COUNTS(..., 1);

Done.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:91: // done once for the
lifetime of this object.
On 2012/05/02 00:23:13, creis wrote:
> I'm a little confused.  It seems like we can stop listening, so we'll need to
be
> able to turn this off and back on again (unlike the comment says).

No, we can't stop listening, unless we know no extension is loaded that will
call this APIs methods. As such, we need to be listening all the time after the
first method call.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api_constants.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api_constants.cc:52: const char
kExtensionNotSupported[] = "The extension API is not supported.";
On 2012/05/02 00:23:13, creis wrote:
> Should this be specific to the processes extension API?

It will be returned when someone calls a processes.* method, so it should be
clear, but I can be more explicit if you believe this helps.

[Charlie Reis, 2012-05-03 18:47:50]

I think we're almost there.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
File
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js
(right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js:7:
chrome.experimental.processes.onUpdated.addListener(function(processes) {
On 2012/04/27 22:01:55, creis wrote:
> On 2012/04/24 18:14:29, nasko wrote:
> > On 2012/04/23 22:19:51, creis wrote:
> > > Is there anything else we can display here from your additions?
> > 
> > We could, but considering this is just a demo, I didn't think we need to
list
> > every possible property. If you believe it will add to the sample, let me
> know.
> 
> I think it might be worthwhile, as a demo to extension writers of what's
> possible.  Feel free to push back if it gets unwieldy, though.

I do think we should add more here, because it's a useful guide for developers. 
These get published on the extension developer docs site as an example of how to
use the API.

We should at least demo things like ID vs OS ID, Profile, FPS, and anything else
that isn't immediately obvious.  I'd also love to see a kill button in each row
if possible.  We probably don't need to show every API call (like
onUnresponsive), though.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/browser_extension_window_controller.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/browser_extension_window_controller.cc:7: #include
"base/values.h"
On 2012/05/02 18:14:02, nasko wrote:
> On 2012/05/02 00:23:13, creis wrote:
> > Unneeded?
> 
> Needed, because of the added function to extension_tab_util.

Sounds like this line belongs in extension_tab_util.cc instead.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.h (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.h:91: // done once for the
lifetime of this object.
On 2012/05/02 18:14:02, nasko wrote:
> On 2012/05/02 00:23:13, creis wrote:
> > I'm a little confused.  It seems like we can stop listening, so we'll need
to
> be
> > able to turn this off and back on again (unlike the comment says).
> 
> No, we can't stop listening, unless we know no extension is loaded that will
> call this APIs methods. As such, we need to be listening all the time after
the
> first method call.

Ok.  Ignore this comment and my other one about task_manager_listening_.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api_constants.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api_constants.cc:52: const char
kExtensionNotSupported[] = "The extension API is not supported.";
On 2012/05/02 18:14:02, nasko wrote:
> On 2012/05/02 00:23:13, creis wrote:
> > Should this be specific to the processes extension API?
> 
> It will be returned when someone calls a processes.* method, so it should be
> clear, but I can be more explicit if you believe this helps.

Yes, more explicit messages are useful to developers.  I might even say
something like "here" or "on this platform," since we're talking about cases
where the entire Task Manager hasn't been compiled into Chrome.

[nasko, 2012-05-03 23:28:04]

The example process monitor is now much closer to the real task manager :).
Fixed the other issues.

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
File
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js
(right):

https://chromiumcodereview.appspot.com/10175008/diff/3001/chrome/common/exten...
chrome/common/extensions/docs/examples/api/processes/process_monitor/popup.js:7:
chrome.experimental.processes.onUpdated.addListener(function(processes) {
On 2012/05/03 18:47:51, creis wrote:
> On 2012/04/27 22:01:55, creis wrote:
> > On 2012/04/24 18:14:29, nasko wrote:
> > > On 2012/04/23 22:19:51, creis wrote:
> > > > Is there anything else we can display here from your additions?
> > > 
> > > We could, but considering this is just a demo, I didn't think we need to
> list
> > > every possible property. If you believe it will add to the sample, let me
> > know.
> > 
> > I think it might be worthwhile, as a demo to extension writers of what's
> > possible.  Feel free to push back if it gets unwieldy, though.
> 
> I do think we should add more here, because it's a useful guide for
developers. 
> These get published on the extension developer docs site as an example of how
to
> use the API.
> 
> We should at least demo things like ID vs OS ID, Profile, FPS, and anything
else
> that isn't immediately obvious.  I'd also love to see a kill button in each
row
> if possible.  We probably don't need to show every API call (like
> onUnresponsive), though.

Changed to more closely resemble the actual task manager :)

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/browser_extension_window_controller.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/browser_extension_window_controller.cc:7: #include
"base/values.h"
On 2012/05/03 18:47:51, creis wrote:
> On 2012/05/02 18:14:02, nasko wrote:
> > On 2012/05/02 00:23:13, creis wrote:
> > > Unneeded?
> > 
> > Needed, because of the added function to extension_tab_util.
> 
> Sounds like this line belongs in extension_tab_util.cc instead.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api_constants.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/77002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api_constants.cc:52: const char
kExtensionNotSupported[] = "The extension API is not supported.";
On 2012/05/03 18:47:51, creis wrote:
> On 2012/05/02 18:14:02, nasko wrote:
> > On 2012/05/02 00:23:13, creis wrote:
> > > Should this be specific to the processes extension API?
> > 
> > It will be returned when someone calls a processes.* method, so it should be
> > clear, but I can be more explicit if you believe this helps.
> 
> Yes, more explicit messages are useful to developers.  I might even say
> something like "here" or "on this platform," since we're talking about cases
> where the entire Task Manager hasn't been compiled into Chrome.

Done.

[Charlie Reis, 2012-05-04 20:41:29]

The new sample extension is great.

LGTM, with the following small changes and doc cleanup.  We should find the
right owners to review.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:135: // using these APIs.
For memory details are not added by this function, the
nit: "For memory details *which* are not added..."

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:276: #endif  //
defined(ENABLE_TASK_MANAGER)
This won't compile without a return if the Task Manager is disabled.  Maybe move
the #endif above the return false?

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/tas...
File chrome/browser/task_manager/task_manager.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/tas...
chrome/browser/task_manager/task_manager.cc:664: CHECK_EQ(0, listen_requests_);
Is there a reason this one is a CHECK and the rest are DCHECKs?  Let's make it a
DCHECK if not, or else add a comment saying why.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
File chrome/common/extensions/api/experimental.processes.json (right):

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:31: "tabs": {
The API doc says this is optional, but we appear to be returning an empty list
for non-renderers instead.  Empty list sounds fine, so let's update the API doc.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:38: "description": "The
most recent measurement of the process CPU usage, between 0 and 100%."
I think we do want to include the clauses about when these are available (from
the API doc).  This file gets machine translated into the docs that users will
be referencing.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:50: "memoryUsage": {
This is still here.  Please remove.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:95: "description": "The
Cache object contains information about the size and utilization of a cache used
by Chromium.",
We should be consistent about Chrome vs Chromium in this file.  (I'm guessing
Chrome, but please check the other API docs.)

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:170: "type": "boolean"
Please include descriptions of all the parameters for each function.

[nasko, 2012-05-07 17:08:47]

I've synced the schema file with the API doc and fixed the remaining issues. 

Adding reviewers for the proper areas:
mpcomplete@chromium.org -> chrome/*/extensions/
yoshiki@chromium.org -> chrome/browser/task_manager/
thakis@chromium.org -> chrome/common/

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/ext...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:135: // using these APIs.
For memory details are not added by this function, the
On 2012/05/04 20:41:30, creis wrote:
> nit: "For memory details *which* are not added..."

Done.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/ext...
chrome/browser/extensions/extension_processes_api.cc:276: #endif  //
defined(ENABLE_TASK_MANAGER)
On 2012/05/04 20:41:30, creis wrote:
> This won't compile without a return if the Task Manager is disabled.  Maybe
move
> the #endif above the return false?

Done.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/tas...
File chrome/browser/task_manager/task_manager.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/browser/tas...
chrome/browser/task_manager/task_manager.cc:664: CHECK_EQ(0, listen_requests_);
On 2012/05/04 20:41:30, creis wrote:
> Is there a reason this one is a CHECK and the rest are DCHECKs?  Let's make it
a
> DCHECK if not, or else add a comment saying why.

No real reason. Probably an editing mistake. Thanks for catching it!

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
File chrome/common/extensions/api/experimental.processes.json (right):

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:31: "tabs": {
On 2012/05/04 20:41:30, creis wrote:
> The API doc says this is optional, but we appear to be returning an empty list
> for non-renderers instead.  Empty list sounds fine, so let's update the API
doc.

Doc updated.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:38: "description": "The
most recent measurement of the process CPU usage, between 0 and 100%."
On 2012/05/04 20:41:30, creis wrote:
> I think we do want to include the clauses about when these are available (from
> the API doc).  This file gets machine translated into the docs that users will
> be referencing.

I've synced all the others with the clause from the API doc.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:50: "memoryUsage": {
On 2012/05/04 20:41:30, creis wrote:
> This is still here.  Please remove.

Done.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:95: "description": "The
Cache object contains information about the size and utilization of a cache used
by Chromium.",
On 2012/05/04 20:41:30, creis wrote:
> We should be consistent about Chrome vs Chromium in this file.  (I'm guessing
> Chrome, but please check the other API docs.)

Replaced by "the browser" to be agnostic to the naming of it. Tabs docs seem to
use that approach.

https://chromiumcodereview.appspot.com/10175008/diff/90002/chrome/common/exte...
chrome/common/extensions/api/experimental.processes.json:170: "type": "boolean"
On 2012/05/04 20:41:30, creis wrote:
> Please include descriptions of all the parameters for each function.

Done.

[Matt Perry, 2012-05-07 20:33:34]

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
chrome/browser/extensions/extension_processes_api.cc:56: // Determine process
type
nit: end with .

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
chrome/browser/extensions/extension_processes_api.cc:268: bool
ExtensionProcessesEventRouter::DidStartTaskManagerListening() {
DidFoo sounds like you're notifying someone that Foo just happened. You should
just drop the "Did".

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
chrome/browser/extensions/extension_processes_api.cc:546:
DidStartTaskManagerListening();
If this returns false, the AddRef will never be balanced.

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
chrome/browser/extensions/extension_tab_util.cc:227: bool
ExtensionTabUtil::ReadOneOrMoreIntegers(base::Value* value,
This is now a generic util function, and not related to tabs. Could you move it
to a new file, say extension_function_util, and rename the variable to not
mention tabs?

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/test/data/...
File chrome/test/data/extensions/api_test/processes/api/test.js (right):

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/test/data/...
chrome/test/data/extensions/api_test/processes/api/test.js:279:
chrome.experimental.processes.getProcessInfo([0, 2], false,
are 0 and 2 supposed to be process IDs here? If so, that seems fragile. You
should use getProcessId to get a real ID.

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/test/data/...
chrome/test/data/extensions/api_test/processes/api/test.js:295:
assertTrue(Object.keys(processes).length >= 1);
why not merge this with the next test?

[nasko, 2012-05-08 00:34:55]

Fixed issues brought up by Matt.

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
chrome/browser/extensions/extension_processes_api.cc:56: // Determine process
type
On 2012/05/07 20:33:34, Matt Perry wrote:
> nit: end with .

Done.

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
chrome/browser/extensions/extension_processes_api.cc:268: bool
ExtensionProcessesEventRouter::DidStartTaskManagerListening() {
On 2012/05/07 20:33:34, Matt Perry wrote:
> DidFoo sounds like you're notifying someone that Foo just happened. You should
> just drop the "Did".

Done.

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
chrome/browser/extensions/extension_processes_api.cc:546:
DidStartTaskManagerListening();
On 2012/05/07 20:33:34, Matt Perry wrote:
> If this returns false, the AddRef will never be balanced.

Removed the return value of StartTaskManagerListening, as it is no longer used
(it was in a previous revision). Does address your concern?

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
chrome/browser/extensions/extension_tab_util.cc:227: bool
ExtensionTabUtil::ReadOneOrMoreIntegers(base::Value* value,
On 2012/05/07 20:33:34, Matt Perry wrote:
> This is now a generic util function, and not related to tabs. Could you move
it
> to a new file, say extension_function_util, and rename the variable to not
> mention tabs?

Done.

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/test/data/...
File chrome/test/data/extensions/api_test/processes/api/test.js (right):

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/test/data/...
chrome/test/data/extensions/api_test/processes/api/test.js:279:
chrome.experimental.processes.getProcessInfo([0, 2], false,
On 2012/05/07 20:33:34, Matt Perry wrote:
> are 0 and 2 supposed to be process IDs here? If so, that seems fragile. You
> should use getProcessId to get a real ID.

ID 0 is guaranteed to be there, since it is the ID for the browser process. I
did however change it to avoid possible flakiness in the future.

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/test/data/...
chrome/test/data/extensions/api_test/processes/api/test.js:295:
assertTrue(Object.keys(processes).length >= 1);
On 2012/05/07 20:33:34, Matt Perry wrote:
> why not merge this with the next test?

Done.

[Matt Perry, 2012-05-08 01:00:34]

lgtm

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
File chrome/browser/extensions/extension_processes_api.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/101002/chrome/browser/ex...
chrome/browser/extensions/extension_processes_api.cc:546:
DidStartTaskManagerListening();
On 2012/05/08 00:34:56, nasko wrote:
> On 2012/05/07 20:33:34, Matt Perry wrote:
> > If this returns false, the AddRef will never be balanced.
> 
> Removed the return value of StartTaskManagerListening, as it is no longer used
> (it was in a previous revision). Does address your concern?

Actually I was more worried about what would happen if ENABLE_TASK_MANAGER were
not defined, but I guess the function wouldn't get called in that case.

https://chromiumcodereview.appspot.com/10175008/diff/96025/chrome/browser/ext...
File chrome/browser/extensions/extension_function_util.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/96025/chrome/browser/ext...
chrome/browser/extensions/extension_function_util.cc:7: bool
ExtensionFuncUtil::ReadOneOrMoreIntegers(base::Value* value,
nit: just make the function global. classes of static methods are discouraged by
the style guide.

[nasko, 2012-05-08 16:39:50]

The utility function is now in the extensions namespace instead of a static
member of class.

https://chromiumcodereview.appspot.com/10175008/diff/96025/chrome/browser/ext...
File chrome/browser/extensions/extension_function_util.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/96025/chrome/browser/ext...
chrome/browser/extensions/extension_function_util.cc:7: bool
ExtensionFuncUtil::ReadOneOrMoreIntegers(base::Value* value,
On 2012/05/08 01:00:34, Matt Perry wrote:
> nit: just make the function global. classes of static methods are discouraged
by
> the style guide.

Done.

[Matt Perry, 2012-05-08 17:40:31]

still LGTM

[Nico, 2012-05-08 18:15:35]

lgtm

[yoshiki, 2012-05-08 19:31:06]

Looking at chrome/browser/task_manager/*.

https://chromiumcodereview.appspot.com/10175008/diff/109002/chrome/browser/ta...
File chrome/browser/task_manager/task_manager.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/109002/chrome/browser/ta...
chrome/browser/task_manager/task_manager.cc:980: if (update_state_ !=
TASK_PENDING || !is_listening_) {
We can use "listen_requests_ == 0" instead of "!is_listening_", right? If so,
the property |is_listening_| can be removed from the class.

[nasko, 2012-05-09 00:21:54]

Removed the unneeded variable in task manager.

https://chromiumcodereview.appspot.com/10175008/diff/109002/chrome/browser/ta...
File chrome/browser/task_manager/task_manager.cc (right):

https://chromiumcodereview.appspot.com/10175008/diff/109002/chrome/browser/ta...
chrome/browser/task_manager/task_manager.cc:980: if (update_state_ !=
TASK_PENDING || !is_listening_) {
On 2012/05/08 19:31:07, yoshiki wrote:
> We can use "listen_requests_ == 0" instead of "!is_listening_", right? If so,
> the property |is_listening_| can be removed from the class.

Done.

[yoshiki, 2012-05-09 02:40:26]

LGTM

[commit-bot: I haz the power, 2012-05-17 15:40:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10175008/94030

[commit-bot: I haz the power, 2012-05-17 15:41:03]

Can't apply patch for file
chrome/common/extensions/api/experimental.processes.json.
While running patch -p1 --forward --force;
patching file chrome/common/extensions/api/experimental.processes.json
Hunk #1 FAILED at 5.
Hunk #2 FAILED at 14.
Hunk #3 FAILED at 21.
Hunk #4 FAILED at 49.
Hunk #5 FAILED at 63.
Hunk #6 FAILED at 74.
6 out of 6 hunks FAILED -- saving rejects to file
chrome/common/extensions/api/experimental.processes.json.rej

[commit-bot: I haz the power, 2012-05-17 16:29:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10175008/103005

[commit-bot: I haz the power, 2012-05-17 18:18:39]

Change committed as 137690