Migrate ProcessesEventRouter to the new task manager

Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation.

The API of the process info has been fixed, since processes don't have titles, instead we added a list of tasks running in that process, with their titles and optional tab IDs.

BUG=525201, 591581
TEST=browser_tests --gtest_filter=ProcessesApiTest.*

Committed: https://crrev.com/98241839b528f945fabb03f38fb1ea6a3e837f09
Cr-Commit-Position: refs/heads/master@{#380696}

[afakhry, 2016-02-02 02:35:19]

Description was changed from

==========
[WIP] Migrate ProcessesEventRouter to the new task manager

BUG=
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProccessApiTest.*
==========

[afakhry, 2016-02-02 02:35:20]

afakhry@chromium.org changed reviewers:
+ creis@chromium.org, nick@chromium.org

[afakhry, 2016-02-02 02:44:27]

Nick and Charlie,

Please take a look at this CL which migrates ProcessesAPIs to the new task
manager implementation.

Notes:
- Working on this CL was very painful yet enjoyable! :)
- I'm sure that the review of this CL will be even more painful! Apologies in
advance!
- I migrated processes.json to processes.idl to define the APIs.
- I had to change certain things in the task manager as well as content/ to
remove the dependency on the NotificationService. For example to get the
termination status of a renderer and to watch for unresponsive renderers.
- As Charlie suggested in his email, I changed the process info, so that it
doesn't contain a 'title' field, but rather, a list of tasks running in that
process with their titles and optional tab Ids.
- Instead of having #if defined(ENABLED_TASK_MANAGER) everywhere, I modified the
gyp and gn files to only include the processes_api* files
if(enable_task_manager). Not sure if that's a good idea, though!

Thanks!

[ncarter (slow), 2016-02-02 22:16:53]

I'll need a little more time to do an in-depth review here, but I skimmed the CL
and here are some first-round comments.

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:15: class
ProccessApiTest;
Proccess -> Process, here and elsewhere.

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
File
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
(right):

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:119:
task->set_termination_status(web_contents()->GetCrashedStatus());
This crash status is really for the main frame, but it's being applied to all
tasks. Is that really what we want to export, or should we do this for just the
main_frame_site_instance_ task?

Also, subframes processes can crash/be killed too, and this won't update
web_contents()->GetCrashedStatus() when they go away.

It might be more natural for GetCrashedStatus() to be moved to the
RenderProcessHost (with the existing WebContents function just calling through
to the RPH state). Then we could query it for any frame, and we can make it so
that it's correct for a frame's RPH during RenderFrameDeleted() too.

It would be fine to get the subframe crashes working as a follow-on CL, unless
they already worked beforehand.

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.h (right):

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.h:63: int
GetChildProcessUniqueID(TaskId task_id) const override;
"ID" -> "Id"

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.h:77: const TaskIdList
GetIdsOfTasksSharingSameProcess(
no need for const on the return value.

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_management/test_task_manager.cc (right):

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/test_task_manager.cc:110: int* out_error_code)
const {
I'd write the out params here, to avoid nondeterminism.

[afakhry, 2016-02-05 19:51:15]

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:15: class
ProccessApiTest;
On 2016/02/02 22:16:53, ncarter wrote:
> Proccess -> Process, here and elsewhere.

Done.

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
File
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
(right):

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:119:
task->set_termination_status(web_contents()->GetCrashedStatus());
On 2016/02/02 22:16:53, ncarter wrote:
> This crash status is really for the main frame, but it's being applied to all
> tasks. Is that really what we want to export, or should we do this for just
the
> main_frame_site_instance_ task?
> 
> Also, subframes processes can crash/be killed too, and this won't update
> web_contents()->GetCrashedStatus() when they go away.
> 
> It might be more natural for GetCrashedStatus() to be moved to the
> RenderProcessHost (with the existing WebContents function just calling through
> to the RPH state). Then we could query it for any frame, and we can make it so
> that it's correct for a frame's RPH during RenderFrameDeleted() too.
> 
> It would be fine to get the subframe crashes working as a follow-on CL, unless
> they already worked beforehand.

Let's discuss more offline.

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.h (right):

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.h:63: int
GetChildProcessUniqueID(TaskId task_id) const override;
On 2016/02/02 22:16:53, ncarter wrote:
> "ID" -> "Id"

Done.

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.h:77: const TaskIdList
GetIdsOfTasksSharingSameProcess(
On 2016/02/02 22:16:53, ncarter wrote:
> no need for const on the return value.

Done.

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_management/test_task_manager.cc (right):

https://codereview.chromium.org/1584473004/diff/140001/chrome/browser/task_ma...
chrome/browser/task_management/test_task_manager.cc:110: int* out_error_code)
const {
On 2016/02/02 22:16:53, ncarter wrote:
> I'd write the out params here, to avoid nondeterminism.

Done.

[afakhry, 2016-02-05 20:10:20]

Patchset #9 (id:160001) has been deleted

[afakhry, 2016-02-05 20:10:53]

Description was changed from

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProccessApiTest.*
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProccessesApiTest.*
==========

[ncarter (slow), 2016-02-08 22:37:09]

I'm still working through this -- but publishing my comments now since the last
one is asking about the feasibility of breaking this CL in half.

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:71: default:
If you omit the 'default' here and add a default-return after the switch, you
ought to be able to force a compile error if this enum is ever added to, which
seems desirable.

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:90:
api::processes::Process* out_process,
|out_process| ought to be the last parameter.--
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

(or, can you change this function so that it returns a Process object?)

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:199: if (--listeners_
<= 0) {
Why not ==

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:117: int
child_process_host_id_;
You don't have a ctor, so it looks like there's no place for this member to be
initialized, which seems dangerous.

= 0 seems like a good idea.

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:158: std::vector<int>
process_host_ids_;
#include <vector>

https://codereview.chromium.org/1584473004/diff/200001/chrome/common/extensio...
File chrome/common/extensions/api/processes.idl (right):

https://codereview.chromium.org/1584473004/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/processes.idl:7: namespace processes {
It is very hard to see how the API has changed. Would it be possible to split
this CL in half, so that the IDL migration is done without changing any of the
API, and there's a separate CL that actually changes the API and does the
new-TaskManager migration?

Currently, this CL is really too complex to efficiently review, and I expect
you'll be pulling in several other reviewers by the time you're done.

[afakhry, 2016-02-09 03:36:05]

Description was changed from

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProccessesApiTest.*
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201,345411
TEST=browser_tests --gtest_filter=ProccessesApiTest.*
==========

[afakhry, 2016-02-09 04:01:23]

The below table will help you understand the mapping between old and new code in
processes_api.cc:

--------------------------------------------+---------------------------------------------

OLD                                         | New
--------------------------------------------+---------------------------------------------
ProcessesEventRouter::OnItemsAdded()        |
ProcessesEventRouter::OnTaskAdded()
ProcessesEventRouter::OnItemsChanged()      |
ProcessesEventRouter::OnTasksRefreshed()
--------------------------------------------+---------------------------------------------
ProcessesEventRouter::OnItemsToBeRemoved()  |
ProcessesEventRouter::OnTaskToBeRemoved()
ProcessesEventRouter::ProcessClosedEvent()  | 
--------------------------------------------+---------------------------------------------
ProcessesEventRouter::ProcessHangEvent()    |
ProcessesEventRouter::OnTaskUnresponsive()
--------------------------------------------+---------------------------------------------
GetProcessIdForTabFunction                  |
ProcessesGetProcessIdForTabFunction
TerminateFunction                           | ProcessesTerminateFunction
GetProcessInfoFunction                      | ProcessesGetProcessInfoFunction
--------------------------------------------+---------------------------------------------

Basically we got rid of the dependency on the notification service, and replaced
ProcessesEventRouter::ProcessClosedEvent() and
ProcessesEventRouter::ProcessHangEvent() called from
ProcessesEventRouter::Observe() with TaskManagerObserver equivalent events.

The TaskManagerObserver events are for broadcasting the ProcessesAPIs events
onUpdated, onUpdatedWithMemory, onCreated, ... etc.
https://developer.chrome.com/extensions/processes

The classes implementing the extension functions had to be renamed to match the
C++ that IDL generates, for example GetProcessIdForTabFunction -->
ProcessesGetProcessIdForTabFunction.

take a look at the changes in the test file
'chrome/test/data/extensions/api_test/processes/api/test.js'. These will help
you verify that the API is the exact same except for what I mentioned below.

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:71: default:
On 2016/02/08 22:37:09, ncarter wrote:
> If you omit the 'default' here and add a default-return after the switch, you
> ought to be able to force a compile error if this enum is ever added to, which
> seems desirable.

Done.

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:90:
api::processes::Process* out_process,
On 2016/02/08 22:37:09, ncarter wrote:
> |out_process| ought to be the last parameter.--
> https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
> 
> (or, can you change this function so that it returns a Process object?)

Thanks for the pointer. Placed as the last parameter.

Process is non-copy-able. I could have returned scoped_ptr<Process>, but thought
it's too verbose.

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:199: if (--listeners_
<= 0) {
On 2016/02/08 22:37:09, ncarter wrote:
> Why not ==

Done.

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:117: int
child_process_host_id_;
On 2016/02/08 22:37:09, ncarter wrote:
> You don't have a ctor, so it looks like there's no place for this member to be
> initialized, which seems dangerous.
> 
> = 0 seems like a good idea.

Done.

https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:158: std::vector<int>
process_host_ids_;
On 2016/02/08 22:37:09, ncarter wrote:
> #include <vector>

Done.

https://codereview.chromium.org/1584473004/diff/200001/chrome/common/extensio...
File chrome/common/extensions/api/processes.idl (right):

https://codereview.chromium.org/1584473004/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/processes.idl:7: namespace processes {
On 2016/02/08 22:37:09, ncarter wrote:
> It is very hard to see how the API has changed. Would it be possible to split
> this CL in half, so that the IDL migration is done without changing any of the
> API, and there's a separate CL that actually changes the API and does the
> new-TaskManager migration?
> 
> Currently, this CL is really too complex to efficiently review, and I expect
> you'll be pulling in several other reviewers by the time you're done.

All the API remained the same except for the Process structure:
(Compare with the old one here
https://developer.chrome.com/extensions/processes#type-Process)

[1] Removed |title| from process, since it's a task-based value.
[2] Replaced |tabs| (an array of integer tab IDs) by |tasks| (an array of
|TaskInfo|, one entry per task running on the process) containing the |title| we
removed from [1], and an optional |tabId| if the task represents a tab.

The test file 'chrome/test/data/extensions/api_test/processes/api/test.js' will
help you verify that these are only the changes to the API.

[afakhry, 2016-02-09 04:07:10]

On 2016/02/09 04:01:23, afakhry wrote:
> The below table will help you understand the mapping between old and new code
in
> processes_api.cc:
> 
>
--------------------------------------------+---------------------------------------------
> 
> OLD                                         | New
>
--------------------------------------------+---------------------------------------------
> ProcessesEventRouter::OnItemsAdded()        |
> ProcessesEventRouter::OnTaskAdded()
> ProcessesEventRouter::OnItemsChanged()      |
> ProcessesEventRouter::OnTasksRefreshed()
>
--------------------------------------------+---------------------------------------------
> ProcessesEventRouter::OnItemsToBeRemoved()  |
> ProcessesEventRouter::OnTaskToBeRemoved()
> ProcessesEventRouter::ProcessClosedEvent()  | 
>
--------------------------------------------+---------------------------------------------
> ProcessesEventRouter::ProcessHangEvent()    |
> ProcessesEventRouter::OnTaskUnresponsive()
>
--------------------------------------------+---------------------------------------------
> GetProcessIdForTabFunction                  |
> ProcessesGetProcessIdForTabFunction
> TerminateFunction                           | ProcessesTerminateFunction
> GetProcessInfoFunction                      | ProcessesGetProcessInfoFunction
>
--------------------------------------------+---------------------------------------------
> 
> Basically we got rid of the dependency on the notification service, and
replaced
> ProcessesEventRouter::ProcessClosedEvent() and
> ProcessesEventRouter::ProcessHangEvent() called from
> ProcessesEventRouter::Observe() with TaskManagerObserver equivalent events.
> 
> The TaskManagerObserver events are for broadcasting the ProcessesAPIs events
> onUpdated, onUpdatedWithMemory, onCreated, ... etc.
> https://developer.chrome.com/extensions/processes
> 
> The classes implementing the extension functions had to be renamed to match
the
> C++ that IDL generates, for example GetProcessIdForTabFunction -->
> ProcessesGetProcessIdForTabFunction.
> 
> take a look at the changes in the test file
> 'chrome/test/data/extensions/api_test/processes/api/test.js'. These will help
> you verify that the API is the exact same except for what I mentioned below.
> 
>
https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
> File chrome/browser/extensions/api/processes/processes_api.cc (right):
> 
>
https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
> chrome/browser/extensions/api/processes/processes_api.cc:71: default:
> On 2016/02/08 22:37:09, ncarter wrote:
> > If you omit the 'default' here and add a default-return after the switch,
you
> > ought to be able to force a compile error if this enum is ever added to,
which
> > seems desirable.
> 
> Done.
> 
>
https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
> chrome/browser/extensions/api/processes/processes_api.cc:90:
> api::processes::Process* out_process,
> On 2016/02/08 22:37:09, ncarter wrote:
> > |out_process| ought to be the last parameter.--
> >
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
> > 
> > (or, can you change this function so that it returns a Process object?)
> 
> Thanks for the pointer. Placed as the last parameter.
> 
> Process is non-copy-able. I could have returned scoped_ptr<Process>, but
thought
> it's too verbose.
> 
>
https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
> chrome/browser/extensions/api/processes/processes_api.cc:199: if (--listeners_
> <= 0) {
> On 2016/02/08 22:37:09, ncarter wrote:
> > Why not ==
> 
> Done.
> 
>
https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
> File chrome/browser/extensions/api/processes/processes_api.h (right):
> 
>
https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
> chrome/browser/extensions/api/processes/processes_api.h:117: int
> child_process_host_id_;
> On 2016/02/08 22:37:09, ncarter wrote:
> > You don't have a ctor, so it looks like there's no place for this member to
be
> > initialized, which seems dangerous.
> > 
> > = 0 seems like a good idea.
> 
> Done.
> 
>
https://codereview.chromium.org/1584473004/diff/200001/chrome/browser/extensi...
> chrome/browser/extensions/api/processes/processes_api.h:158: std::vector<int>
> process_host_ids_;
> On 2016/02/08 22:37:09, ncarter wrote:
> > #include <vector>
> 
> Done.
> 
>
https://codereview.chromium.org/1584473004/diff/200001/chrome/common/extensio...
> File chrome/common/extensions/api/processes.idl (right):
> 
>
https://codereview.chromium.org/1584473004/diff/200001/chrome/common/extensio...
> chrome/common/extensions/api/processes.idl:7: namespace processes {
> On 2016/02/08 22:37:09, ncarter wrote:
> > It is very hard to see how the API has changed. Would it be possible to
split
> > this CL in half, so that the IDL migration is done without changing any of
the
> > API, and there's a separate CL that actually changes the API and does the
> > new-TaskManager migration?
> > 
> > Currently, this CL is really too complex to efficiently review, and I expect
> > you'll be pulling in several other reviewers by the time you're done.
> 
> All the API remained the same except for the Process structure:
> (Compare with the old one here
> https://developer.chrome.com/extensions/processes#type-Process)
> 
> [1] Removed |title| from process, since it's a task-based value.
> [2] Replaced |tabs| (an array of integer tab IDs) by |tasks| (an array of
> |TaskInfo|, one entry per task running on the process) containing the |title|
we
> removed from [1], and an optional |tabId| if the task represents a tab.
> 
> The test file 'chrome/test/data/extensions/api_test/processes/api/test.js'
will
> help you verify that these are only the changes to the API.

Table was distorted, resending:

---------------------------+----------------------------------------------------

ProcessesEventRouter:: OLD | ProcessesEventRouter:: New
---------------------------+----------------------------------------------------
OnItemsAdded()             | OnTaskAdded()
OnItemsChanged()           | OnTasksRefreshed()
---------------------------+----------------------------------------------------
OnItemsToBeRemoved()       | OnTaskToBeRemoved()
ProcessClosedEvent()       | 
---------------------------+----------------------------------------------------
ProcessHangEvent()         | OnTaskUnresponsive()
---------------------------+----------------------------------------------------
GetProcessIdForTabFunction | ProcessesGetProcessIdForTabFunction
TerminateFunction          | ProcessesTerminateFunction
GetProcessInfoFunction     | ProcessesGetProcessInfoFunction
---------------------------+----------------------------------------------------

[ncarter (slow), 2016-02-11 00:33:55]

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:181: }
Given the need to conditionally unsubscribe in the dtor, maybe ScopedObserver
would be helpful.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:191:
task_management::TaskManagerInterface::GetTaskManager()->AddObserver(this);
indentation is off here.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:272:
observed_task_manager()->GetProcessId(task_id);
Why not de-dup on child_process_host_id?

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:276:
seen_processes.insert(proc_id);
In OnTaskAdded, we filter out tasks if ChildProcessUniqueId <= 0. Should we do
the same here?

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:17: class
ProccessesApiTest;
Proccesses -> Processes

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:41: friend class
::ProccessesApiTest;
Proccesses -> Processes

[ncarter (slow), 2016-02-11 22:16:50]

Overall: I think this is good.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (left):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:698:
EXTENSION_FUNCTION_VALIDATE(ReadOneOrMoreIntegers(processes, &process_ids_));
Looks like this was the only call to this function, and ReadOneOrMoreIntegers is
the only function in its util file, so I think you can delete
extensions/browser/extension_function_util.h/.cc!

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:304: // of processes as
an added bonus.
This looks like a behavior change versus the old code. If so, why make it? It
doesn't seem like an improvement to sometimes randomly populate extra fields on
an event -- it just seems like a source of possible nondeterminism.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:448: if
(child_process_host_id_ == 0) {
You use == 0 here, but elsewhere we use <= 0.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:483: auto* render_host
=
process, process_host, rph, or render_process_host would be more conventional
names for this variable.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:531: 
Shouldn't we add REFRESH_TYPE_MEMORY based on the value of include_memory_ ?

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:542: void
ProcessesGetProcessInfoFunction::OnTasksRefreshed(
What motivates making this function being a TM observer?

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:563: auto itr =
std::find(process_host_ids_.begin(),
std::find needs <algorithm>

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_apitest.cc (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_apitest.cc:15: class
ProccessesApiTest : public ExtensionApiTest {
Proccesses -> Processes

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/task_ma...
File chrome/browser/task_management/providers/task_provider_observer.h (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/task_ma...
chrome/browser/task_management/providers/task_provider_observer.h:29: //
renderer has become unresponsive.
The "in which case" part of the second sentence seems redundant to the first
sentence.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/task_ma...
File
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
(right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:170:

This function should use the |render_widget_host| parameter, just in case it
gets hooked up for subframes in the meantime. A simple version of this might be
just to add a DCHECK

DCHECK_EQ(render_widget_host->GetProcess(),
web_contents()->GetMainFrame()->GetProcess());

[afakhry, 2016-02-16 20:10:37]

Description was changed from

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201,345411
TEST=browser_tests --gtest_filter=ProccessesApiTest.*
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201,345411
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

[afakhry, 2016-02-16 21:07:52]

Description was changed from

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201,345411
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

[afakhry, 2016-02-17 02:27:14]

Nick, I handled your comments, as well as implemented a way to notify observers
when background resource usage calculations are complete for all currently
available processes. I honestly have a mixed feeling about the implementation,
especially the naming of things. I also hate to see what was once a minimum
clean API of a TaskManager become so bloated like that! :( A sad fact of life
that code get to rot after a while. But I'm willing to improve things in the
upcoming patches. Please take a look at the initial implementation and please
suggest any modifications.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (left):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:698:
EXTENSION_FUNCTION_VALIDATE(ReadOneOrMoreIntegers(processes, &process_ids_));
On 2016/02/11 22:16:49, ncarter wrote:
> Looks like this was the only call to this function, and ReadOneOrMoreIntegers
is
> the only function in its util file, so I think you can delete
> extensions/browser/extension_function_util.h/.cc!

Done. Thanks for catching this.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:181: }
On 2016/02/11 00:33:54, ncarter wrote:
> Given the need to conditionally unsubscribe in the dtor, maybe ScopedObserver
> would be helpful.

I think ScopedObserver might be an overkill. It seems to be used when there is a
need to observe many Sources. In our case we only observe a single task manager.
The above condition is there for our sanity just in case something skipped
ListenerRemoved() below.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:191:
task_management::TaskManagerInterface::GetTaskManager()->AddObserver(this);
On 2016/02/11 00:33:55, ncarter wrote:
> indentation is off here.

Done.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:272:
observed_task_manager()->GetProcessId(task_id);
On 2016/02/11 00:33:55, ncarter wrote:
> Why not de-dup on child_process_host_id?

child_process_host_id isn't unique enough unfortunately. All ARC processes will
report a value of content::ChildProcessHost::kInvalidUniqueID (-1) -- 0 prior to
this patch -- so it's safer to dedup with something that is surely unique.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:276:
seen_processes.insert(proc_id);
On 2016/02/11 00:33:55, ncarter wrote:
> In OnTaskAdded, we filter out tasks if ChildProcessUniqueId <= 0. Should we do
> the same here?

We actually want to report the browser process, but ignore ARC processes. 
I got this wrong idea that we don't want to report the browser process from the
test here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...

This is one of the reasons why I'm wondering why can't we just use the OS PIDs
for this API instead of the child_process_host_ids.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:304: // of processes as
an added bonus.
On 2016/02/11 22:16:49, ncarter wrote:
> This looks like a behavior change versus the old code. If so, why make it? It
> doesn't seem like an improvement to sometimes randomly populate extra fields
on
> an event -- it just seems like a source of possible nondeterminism.

You're right. It is a behavior change. I did this intentionally. The reason was
to fill in only a **single** dictionary with the needed data (with memory if
there are listeners to onUpdatedWithMemory(), and without memory if we only have
listeners to onUpdated()) with one single iteration. See the old implementation
to see why I was motivated to do this.

Since listeners know what they listen to, so listeners to onUpdated() only
wouldn't be affected if |process['privateMemory']| is defined or not. But
listeners to onUpdatedWithMemory() are guaranteed to have it defined all the
time.

Let me know if I need to change this, it seems to me a nice improvement without
any effect on clients.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:448: if
(child_process_host_id_ == 0) {
On 2016/02/11 22:16:49, ncarter wrote:
> You use == 0 here, but elsewhere we use <= 0.

Thanks for highlighting the discrepancy. 

I now changed ArcProcessTask::GetChildProcessUniqueID() to return 
content::ChildProcessHost::kInvalidUniqueID (which is -1) instead of 0, to
reserve the value 0 for the browser process. 

Now I want to keep the < 0 case separate and report it as an invalid argument.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:483: auto* render_host
=
On 2016/02/11 22:16:49, ncarter wrote:
> process, process_host, rph, or render_process_host would be more conventional
> names for this variable.

Done.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:531: 
On 2016/02/11 22:16:49, ncarter wrote:
> Shouldn't we add REFRESH_TYPE_MEMORY based on the value of include_memory_ ?

Yep, my bad! Done.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:542: void
ProcessesGetProcessInfoFunction::OnTasksRefreshed(
On 2016/02/11 22:16:49, ncarter wrote:
> What motivates making this function being a TM observer?

As we discussed offline, to make this independent from the presence of listeners
to onUpdated(), onCreated(), ... etc. This way, it's only observing the task
manager only for the lifetime of this function.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:563: auto itr =
std::find(process_host_ids_.begin(),
On 2016/02/11 22:16:49, ncarter wrote:
> std::find needs <algorithm>

Right. Done. By the way, why trybots don't fail to compile on a missing thing
like this?

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:17: class
ProccessesApiTest;
On 2016/02/11 00:33:55, ncarter wrote:
> Proccesses -> Processes

Done.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:41: friend class
::ProccessesApiTest;
On 2016/02/11 00:33:55, ncarter wrote:
> Proccesses -> Processes

Done.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_apitest.cc (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_apitest.cc:15: class
ProccessesApiTest : public ExtensionApiTest {
On 2016/02/11 22:16:49, ncarter wrote:
> Proccesses -> Processes

Done. :)

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/task_ma...
File chrome/browser/task_management/providers/task_provider_observer.h (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/task_ma...
chrome/browser/task_management/providers/task_provider_observer.h:29: //
renderer has become unresponsive.
On 2016/02/11 22:16:49, ncarter wrote:
> The "in which case" part of the second sentence seems redundant to the first
> sentence.

Done.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/task_ma...
File
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
(right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:170:

On 2016/02/11 22:16:49, ncarter wrote:
> This function should use the |render_widget_host| parameter, just in case it
> gets hooked up for subframes in the meantime. A simple version of this might
be
> just to add a DCHECK
> 
> DCHECK_EQ(render_widget_host->GetProcess(),
> web_contents()->GetMainFrame()->GetProcess());

Done.

[ncarter (slow), 2016-02-17 21:21:20]

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:181: }
On 2016/02/17 02:27:13, afakhry wrote:
> On 2016/02/11 00:33:54, ncarter wrote:
> > Given the need to conditionally unsubscribe in the dtor, maybe
ScopedObserver
> > would be helpful.
> 
> I think ScopedObserver might be an overkill. It seems to be used when there is
a
> need to observe many Sources. In our case we only observe a single task
manager.
> The above condition is there for our sanity just in case something skipped
> ListenerRemoved() below.

Acknowledged.

https://codereview.chromium.org/1584473004/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:563: auto itr =
std::find(process_host_ids_.begin(),
On 2016/02/17 02:27:14, afakhry wrote:
> On 2016/02/11 22:16:49, ncarter wrote:
> > std::find needs <algorithm>
> 
> Right. Done. By the way, why trybots don't fail to compile on a missing thing
> like this?

It's almost certainly transitively included by some other header file. C++ is
silly.

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_group.cc (right):

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_group.cc:139:
expected_on_bg_done_flags_ = refresh_flags & kBackgroundRefreshTypesMask;
Is it possible for the next Refresh event to fire, before we hear back from the
blocking pool for the previous refresh? Does anything weird happen if it does?

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_group.cc:267:
current_on_bg_done_flags_ |= finished_refresh_type;
current_on_bg_done_flags_ is never cleared back to 0. Is that intentional?

Since the TaskGroup can persist across extension function invocations, it seems
like subsequent queries might notify too early, when the first (rather than
last) background type reports back "refresh done", and I imagine that the other
types would be stale at this point?

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_group.cc:268: if
(current_on_bg_done_flags_ == expected_on_bg_done_flags_)
What if refresh_flags (at Refresh() time) didn't specify *all* of the
kBackgroundRefreshTypesMask types? In that case it seems like we'll never run
the callback (because e.g. OnIdleWakeupsRefreshDone won't happen if
REFRESH_TYPE_IDLE_WAKEUPS wasn't requested?)

[afakhry, 2016-02-19 17:35:10]

PTAL

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_group.cc (right):

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_group.cc:139:
expected_on_bg_done_flags_ = refresh_flags & kBackgroundRefreshTypesMask;
On 2016/02/17 21:21:19, ncarter wrote:
> Is it possible for the next Refresh event to fire, before we hear back from
the
> blocking pool for the previous refresh? Does anything weird happen if it does?

I think it could, since we are using a sequenced task runner for the background
thread. So, I think if we have a lot a lot of tasks, with a lot of background
refresh types enabled, it could happen that (at a one-second refresh interval)
the periodic Refresh() happens before all background operations finishes.

There shouldn't be anything weird if that happens. We only fire the event
TaskManagerObserver::OnTasksRefreshedWithBackgroundCalculations() when all
TaskGroups have heard back from their background samplers for all the enabled
refresh types.

We clear |current_on_bg_done_flags_| only when all observers have been notified
with OnTasksRefreshedWithBackgroundCalculations() at which point we are ready
for another cycle for this event.

So This event means we have ready data for the background calculations like the
CPU or memory ***from the most recent completed background operation***. That
could be from an operation triggered by the current Refresh() or the previous
one. The thing is once we have a complete snapshot of all background data, we
broadcast this event, reset |current_on_bg_done_flags_|, repeat.

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_group.cc:267:
current_on_bg_done_flags_ |= finished_refresh_type;
On 2016/02/17 21:21:19, ncarter wrote:
> current_on_bg_done_flags_ is never cleared back to 0. Is that intentional?
> 
> Since the TaskGroup can persist across extension function invocations, it
seems
> like subsequent queries might notify too early, when the first (rather than
> last) background type reports back "refresh done", and I imagine that the
other
> types would be stale at this point?

Oh sorry it was clear enough. |current_on_bg_done_flags_| is actually cleared in
the ClearCurrentBackgroundCalculationsFlags() in task_group.h. I moved the
definition to the .cc file.

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_group.cc:268: if
(current_on_bg_done_flags_ == expected_on_bg_done_flags_)
On 2016/02/17 21:21:19, ncarter wrote:
> What if refresh_flags (at Refresh() time) didn't specify *all* of the
> kBackgroundRefreshTypesMask types? In that case it seems like we'll never run
> the callback (because e.g. OnIdleWakeupsRefreshDone won't happen if
> REFRESH_TYPE_IDLE_WAKEUPS wasn't requested?)

Thanks! I added a line to TaskGroup:Refresh() to take care of this case when a
refresh type gets disabled.

[ncarter (slow), 2016-02-19 18:19:03]

lgtm

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_group.cc (right):

https://codereview.chromium.org/1584473004/diff/320001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_group.cc:139:
expected_on_bg_done_flags_ = refresh_flags & kBackgroundRefreshTypesMask;
On 2016/02/19 17:35:09, afakhry wrote:
> On 2016/02/17 21:21:19, ncarter wrote:
> > Is it possible for the next Refresh event to fire, before we hear back from
> the
> > blocking pool for the previous refresh? Does anything weird happen if it
does?
> 
> I think it could, since we are using a sequenced task runner for the
background
> thread. So, I think if we have a lot a lot of tasks, with a lot of background
> refresh types enabled, it could happen that (at a one-second refresh interval)
> the periodic Refresh() happens before all background operations finishes.
> 
> There shouldn't be anything weird if that happens. We only fire the event
> TaskManagerObserver::OnTasksRefreshedWithBackgroundCalculations() when all
> TaskGroups have heard back from their background samplers for all the enabled
> refresh types.
> 
> We clear |current_on_bg_done_flags_| only when all observers have been
notified
> with OnTasksRefreshedWithBackgroundCalculations() at which point we are ready
> for another cycle for this event.
> 
> So This event means we have ready data for the background calculations like
the
> CPU or memory ***from the most recent completed background operation***. That
> could be from an operation triggered by the current Refresh() or the previous
> one. The thing is once we have a complete snapshot of all background data, we
> broadcast this event, reset |current_on_bg_done_flags_|, repeat.

OK.

I do worry a bit about the case where sampling takes longer than 1s on average
(due e.g. to extreme load), and we wind up with a queue of Refresh() tasks that
grows faster than it drains.

https://codereview.chromium.org/1584473004/diff/340001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_manager_impl.cc (right):

https://codereview.chromium.org/1584473004/diff/340001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_manager_impl.cc:441:
groups_itr.second->ClearCurrentBackgroundCalculationsFlags();
Ah okay, this was the part I missed. I think this is correct -- though I agree
with your TODO that it's a little inelegant, I think it's fine.

[Charlie Reis, 2016-02-19 19:07:35]

(I'm happy to defer to Nick's review here.  Thanks!)

[afakhry, 2016-02-19 19:24:56]

afakhry@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[afakhry, 2016-02-19 19:24:58]

rdevlin.cronin@chromium.org: Could you please review the extensions related
code.

[Devlin, 2016-02-19 22:32:52]

On 2016/02/19 19:24:58, afakhry wrote:
> mailto:rdevlin.cronin@chromium.org: Could you please review the extensions
related
> code.

There's a few too many "also"s in your CL description.  Any way we can break
this up into multiple CLs?  In particular, I'd like to have things like the
migration between idl and json be separate (since it should be a non-functional
change).  You'll know better than I if there's anything else like that here we
could move into a separate patch.  (We could keep this as the main CL, with the
changes to use the new task manager.)

[afakhry, 2016-03-03 02:20:21]

Description was changed from

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. It also removes the dependency on the NotificationService. It
also
migrates the APIs definitions from processes.json to processes.idl.

We also modified the API of the process info, since processes don't have titles,
and we added instead a list of tasks running in that process, with their titles
and optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. 

The API of the process info was fixed, since processes don't have titles,
instead we added a list of tasks running in that process, with their titles and
optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

[afakhry, 2016-03-03 02:20:38]

Description was changed from

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. 

The API of the process info was fixed, since processes don't have titles,
instead we added a list of tasks running in that process, with their titles and
optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. 

The API of the process info has been fixed, since processes don't have titles,
instead we added a list of tasks running in that process, with their titles and
optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

[afakhry, 2016-03-03 02:21:53]

Rebase has been done over the newly landed IDL migration CL.

Devlin, please review. Thanks!

[Devlin, 2016-03-03 23:12:43]

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (left):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:61:
scoped_ptr<api::processes::Cache> cache(new api::processes::Cache());
revert this change

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:130: new
api::processes::TaskInfo);
nit: explicit ctor

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:148: int64_t
v8_allocated;
initialize these

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:150: if
(task_manager->GetV8Memory(id, &v8_allocated, & v8_used)) {
nit: no space between & and v8_used (would git cl format catch this?)

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:178:
base::TimeDelta::FromSeconds(1),
If we don't have any listeners for OnUpdated[WithMemory], won't this cause a
good deal of extra work for the task manager?  Could we instead wait until we
have a listener for OnUpdated and set the refresh time then?  (Optionally
setting it for an obscenely long time here)

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:244: // Is this the
last task to be removed from the process, and hence its removal
Common code between this and OnTaskAdded could be pulled out.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:300:
FillProcessData(task_id, observed_task_manager(), true, &process);
doc anonymous bool

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:500: void
ProcessesTerminateFunction::OnProcessHandleUI(base::ProcessHandle handle) {
I don't know much about base::ProcessHandles and BrowserChildProcessHosts - is
it thread safe to pass the handle around like this?

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:508: if
(render_process_host)
Would it be cheaper to try this before we do the thread hop?

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:131: base::ProcessHandle
GetProcessHandleIO(int child_process_host_id) const;
nit: OnIO

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:132: void
OnProcessHandleUI(base::ProcessHandle handle);
nit: ReceivedProcessHandleOnUI

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:165: // of this
function. The only background calculations we want to watch is
Reword this sentence (is the "function." supposed to be "function,"?)

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_apitest.cc (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_apitest.cc:33:
IN_PROC_BROWSER_TEST_F(ProcessesApiTest, ProcessesApiListeners) {
I'm honestly not sure how useful this test is as it's been written... Could we
also:
- Check the task manager's refresh rate?
- Add different event listeners?

https://codereview.chromium.org/1584473004/diff/360001/chrome/common/extensio...
File chrome/common/extensions/api/processes.idl (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/common/extensio...
chrome/common/extensions/api/processes.idl:55: TaskInfo[] tasks;
Breaking changes :(

Is there anything else we should update while we're here?

https://codereview.chromium.org/1584473004/diff/360001/chrome/common/extensio...
chrome/common/extensions/api/processes.idl:149: 
Will this newline affect our doc parsing?

[afakhry, 2016-03-07 22:22:32]

Description was changed from

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. 

The API of the process info has been fixed, since processes don't have titles,
instead we added a list of tasks running in that process, with their titles and
optional tab IDs.

BUG=525201
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. 

The API of the process info has been fixed, since processes don't have titles,
instead we added a list of tasks running in that process, with their titles and
optional tab IDs.

BUG=525201,591581
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

[afakhry, 2016-03-08 01:37:30]

PTAL. Thanks!

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (left):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:61:
scoped_ptr<api::processes::Cache> cache(new api::processes::Cache());
On 2016/03/03 23:12:43, Devlin wrote:
> revert this change

Done.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:130: new
api::processes::TaskInfo);
On 2016/03/03 23:12:43, Devlin wrote:
> nit: explicit ctor

Done.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:148: int64_t
v8_allocated;
On 2016/03/03 23:12:43, Devlin wrote:
> initialize these

Done.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:150: if
(task_manager->GetV8Memory(id, &v8_allocated, & v8_used)) {
On 2016/03/03 23:12:43, Devlin wrote:
> nit: no space between & and v8_used (would git cl format catch this?)

Done. Yes it does catch it.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:178:
base::TimeDelta::FromSeconds(1),
On 2016/03/03 23:12:43, Devlin wrote:
> If we don't have any listeners for OnUpdated[WithMemory], won't this cause a
> good deal of extra work for the task manager?  Could we instead wait until we
> have a listener for OnUpdated and set the refresh time then?  (Optionally
> setting it for an obscenely long time here)

True. This doesn't add an observer to the task manager yet. It just sets up this
observer with its default settings. GetRefreshTypesFlagIncludeOptionalData()
doesn't include the memory refresh flag. ProcessesEventRouter::ListenerAdded()
is where we start observing the task manager and add the memory refresh flag if
a listener is interested in OnUpdatedWithMemory().

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:244: // Is this the
last task to be removed from the process, and hence its removal
On 2016/03/03 23:12:43, Devlin wrote:
> Common code between this and OnTaskAdded could be pulled out.

Done.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:300:
FillProcessData(task_id, observed_task_manager(), true, &process);
On 2016/03/03 23:12:43, Devlin wrote:
> doc anonymous bool

Done here and in all.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:500: void
ProcessesTerminateFunction::OnProcessHandleUI(base::ProcessHandle handle) {
On 2016/03/03 23:12:43, Devlin wrote:
> I don't know much about base::ProcessHandles and BrowserChildProcessHosts - is
> it thread safe to pass the handle around like this?

I'm not sure, but the task manager is already using this pattern and I'm not
aware of any problems.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:508: if
(render_process_host)
On 2016/03/03 23:12:43, Devlin wrote:
> Would it be cheaper to try this before we do the thread hop?

Great idea! Done.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:131: base::ProcessHandle
GetProcessHandleIO(int child_process_host_id) const;
On 2016/03/03 23:12:43, Devlin wrote:
> nit: OnIO

Done.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:132: void
OnProcessHandleUI(base::ProcessHandle handle);
On 2016/03/03 23:12:43, Devlin wrote:
> nit: ReceivedProcessHandleOnUI

Done.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:165: // of this
function. The only background calculations we want to watch is
On 2016/03/03 23:12:43, Devlin wrote:
> Reword this sentence (is the "function." supposed to be "function,"?)

Yes, done.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_apitest.cc (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_apitest.cc:33:
IN_PROC_BROWSER_TEST_F(ProcessesApiTest, ProcessesApiListeners) {
On 2016/03/03 23:12:43, Devlin wrote:
> I'm honestly not sure how useful this test is as it's been written... Could we
> also:
> - Check the task manager's refresh rate?
> - Add different event listeners?

I added another listener for onUpdatedWithMemory and made sure that the memory
refresh type flag is enabled, and the listeners count is updated.

As for the task manager's refresh rate, This could make the test unpredictable.
The task manager will run at the lowest requested refresh rate by all of its
clients. It's true that we currently cut off at 1 Hz, but this could change.
There are many tests in the task manager itself to make sure the refresh rate is
updated correctly. We could do EXPECT_TRUE(current_tm_refresh_time <=
requested_refresh_time); But I don't see an added benefit.

https://codereview.chromium.org/1584473004/diff/360001/chrome/common/extensio...
File chrome/common/extensions/api/processes.idl (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/common/extensio...
chrome/common/extensions/api/processes.idl:55: TaskInfo[] tasks;
On 2016/03/03 23:12:43, Devlin wrote:
> Breaking changes :(
> 
> Is there anything else we should update while we're here?

No this is just it.

https://codereview.chromium.org/1584473004/diff/360001/chrome/common/extensio...
chrome/common/extensions/api/processes.idl:149: 
On 2016/03/03 23:12:43, Devlin wrote:
> Will this newline affect our doc parsing?

Reverted this line.

[afakhry, 2016-03-08 02:44:50]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 02:45:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1584473004/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1584473004/380001

[commit-bot: I haz the power, 2016-03-08 02:53:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-08 02:53:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2016-03-08 18:39:57]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 18:40:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1584473004/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1584473004/380001

[commit-bot: I haz the power, 2016-03-08 18:46:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-08 18:46:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-03-08 21:27:23]

Getting close!

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:178:
base::TimeDelta::FromSeconds(1),
On 2016/03/08 01:37:29, afakhry wrote:
> On 2016/03/03 23:12:43, Devlin wrote:
> > If we don't have any listeners for OnUpdated[WithMemory], won't this cause a
> > good deal of extra work for the task manager?  Could we instead wait until
we
> > have a listener for OnUpdated and set the refresh time then?  (Optionally
> > setting it for an obscenely long time here)
> 
> True. This doesn't add an observer to the task manager yet. It just sets up
this
> observer with its default settings. GetRefreshTypesFlagIncludeOptionalData()
> doesn't include the memory refresh flag. ProcessesEventRouter::ListenerAdded()
> is where we start observing the task manager and add the memory refresh flag
if
> a listener is interested in OnUpdatedWithMemory().

Right.  The case I'm trying to defend against is that an extension adds a
listener for, e.g., onProcessCreated.  When that happens, we add this as an
observer to the TM.  But if that's the only listener, there's no reason to make
the TM regenerate the metrics for all processes, when all we really need is the
onCreated signal - but we'd still have it update every second.  It'd be good to
only force the TM to refresh all the metrics if we have a consumer for those.

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:132: void
OnProcessHandleUI(base::ProcessHandle handle);
On 2016/03/08 01:37:29, afakhry wrote:
> On 2016/03/03 23:12:43, Devlin wrote:
> > nit: ReceivedProcessHandleOnUI
> 
> Done.

I think this one was only half-done - OnProcessHandleOnUI doesn't quite make
sense. ;)

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:41:
base::LazyInstance<BrowserContextKeyedAPIFactory<ProcessesAPI> >
nit: s/> >/>>

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:186:
task_management::TaskManagerInterface::GetTaskManager()->RemoveObserver(
This looks like something that should happen in ~TaskManagerObserver() to me. 
Is there any time we'd *not* want to do this?

Also, isn't observed_task_manager() equivalent to this?  i.e., can't this be
observed_task_manager()->RemoveObserver(this)
?

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:491: if (handle ==
base::kNullProcessHandle) {
Should this be an else to match the if on 488?  Is there ever a time that an
RPH's process handle would be null (and then found on the io thread)?

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:507: const char*
error_message = nullptr;
These ten-ish lines can be shared between here and OnProcessHandleOnUI().  Make
a helper function that returns a ResponseValue, and here you could do:
return RespondNow(TerminateIfAllowed(handle)) and in the other
Respond(TerminateIfAllowed(handle)) (and given that then there will only be one
call, you might also coalesce TerminateIfAllowed, Terminate, and CanTerminate
into fewer [one?] functions)

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:53: // If a task with
task |id| was just added or about to be removed, should we
nit: this comment is too implementation-specific, I think, and my guess.  I'd
just say "Returns true if the given |task| should be reported as created or
removed. |out_child_process_host_id| will be filled...".  The rest can be
comments in the function itself.  Otherwise, I suspect we'll change the
implementation and leave people reading the comment very confused.

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:105:
scoped_ptr<ProcessesEventRouter> processes_event_router_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1584473004/diff/380001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/processes/onupdated_with_memory/background.js
(right):

https://codereview.chromium.org/1584473004/diff/380001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/processes/onupdated_with_memory/background.js:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/1584473004/diff/380001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/processes/onupdated_with_memory/background.js:8:
console.log("Received update with memory.");
Should we check at all that we did receive processes with memory?

[afakhry, 2016-03-09 02:10:14]

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:41:
base::LazyInstance<BrowserContextKeyedAPIFactory<ProcessesAPI> >
On 2016/03/08 21:27:22, Devlin wrote:
> nit: s/> >/>>

Done.

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:186:
task_management::TaskManagerInterface::GetTaskManager()->RemoveObserver(
On 2016/03/08 21:27:22, Devlin wrote:
> This looks like something that should happen in ~TaskManagerObserver() to me. 
> Is there any time we'd *not* want to do this?
> 
> Also, isn't observed_task_manager() equivalent to this?  i.e., can't this be
> observed_task_manager()->RemoveObserver(this)
> ?

I'm not sure why I haven't thought of this before. Thanks! Done.

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:491: if (handle ==
base::kNullProcessHandle) {
On 2016/03/08 21:27:22, Devlin wrote:
> Should this be an else to match the if on 488?  Is there ever a time that an
> RPH's process handle would be null (and then found on the io thread)?

Yes if the renderer is still starting, but you are right, that doesn't mean we
will get anything on the IO thread. If it's a renderer, then it's a renderer.

Done.

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:507: const char*
error_message = nullptr;
On 2016/03/08 21:27:22, Devlin wrote:
> These ten-ish lines can be shared between here and OnProcessHandleOnUI(). 
Make
> a helper function that returns a ResponseValue, and here you could do:
> return RespondNow(TerminateIfAllowed(handle)) and in the other
> Respond(TerminateIfAllowed(handle)) (and given that then there will only be
one
> call, you might also coalesce TerminateIfAllowed, Terminate, and CanTerminate
> into fewer [one?] functions)

Great suggestion. Thanks. Done.

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.h (right):

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:53: // If a task with
task |id| was just added or about to be removed, should we
On 2016/03/08 21:27:22, Devlin wrote:
> nit: this comment is too implementation-specific, I think, and my guess.  I'd
> just say "Returns true if the given |task| should be reported as created or
> removed. |out_child_process_host_id| will be filled...".  The rest can be
> comments in the function itself.  Otherwise, I suspect we'll change the
> implementation and leave people reading the comment very confused.

Agree. Makes sense. Done.

https://codereview.chromium.org/1584473004/diff/380001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.h:105:
scoped_ptr<ProcessesEventRouter> processes_event_router_;
On 2016/03/08 21:27:22, Devlin wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1584473004/diff/380001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/processes/onupdated_with_memory/background.js
(right):

https://codereview.chromium.org/1584473004/diff/380001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/processes/onupdated_with_memory/background.js:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/03/08 21:27:23, Devlin wrote:
> no (c)

Done.

https://codereview.chromium.org/1584473004/diff/380001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/processes/onupdated_with_memory/background.js:8:
console.log("Received update with memory.");
On 2016/03/08 21:27:22, Devlin wrote:
> Should we check at all that we did receive processes with memory?

Yes this is already part of the test here
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...

[Devlin, 2016-03-09 15:16:12]

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:178:
base::TimeDelta::FromSeconds(1),
On 2016/03/08 21:27:22, Devlin wrote:
> On 2016/03/08 01:37:29, afakhry wrote:
> > On 2016/03/03 23:12:43, Devlin wrote:
> > > If we don't have any listeners for OnUpdated[WithMemory], won't this cause
a
> > > good deal of extra work for the task manager?  Could we instead wait until
> we
> > > have a listener for OnUpdated and set the refresh time then?  (Optionally
> > > setting it for an obscenely long time here)
> > 
> > True. This doesn't add an observer to the task manager yet. It just sets up
> this
> > observer with its default settings. GetRefreshTypesFlagIncludeOptionalData()
> > doesn't include the memory refresh flag.
ProcessesEventRouter::ListenerAdded()
> > is where we start observing the task manager and add the memory refresh flag
> if
> > a listener is interested in OnUpdatedWithMemory().
> 
> Right.  The case I'm trying to defend against is that an extension adds a
> listener for, e.g., onProcessCreated.  When that happens, we add this as an
> observer to the TM.  But if that's the only listener, there's no reason to
make
> the TM regenerate the metrics for all processes, when all we really need is
the
> onCreated signal - but we'd still have it update every second.  It'd be good
to
> only force the TM to refresh all the metrics if we have a consumer for those.

Looks like you missed this one?

[afakhry, 2016-03-09 18:46:37]

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:178:
base::TimeDelta::FromSeconds(1),
On 2016/03/09 15:16:12, Devlin wrote:
> On 2016/03/08 21:27:22, Devlin wrote:
> > On 2016/03/08 01:37:29, afakhry wrote:
> > > On 2016/03/03 23:12:43, Devlin wrote:
> > > > If we don't have any listeners for OnUpdated[WithMemory], won't this
cause
> a
> > > > good deal of extra work for the task manager?  Could we instead wait
until
> > we
> > > > have a listener for OnUpdated and set the refresh time then? 
(Optionally
> > > > setting it for an obscenely long time here)
> > > 
> > > True. This doesn't add an observer to the task manager yet. It just sets
up
> > this
> > > observer with its default settings.
GetRefreshTypesFlagIncludeOptionalData()
> > > doesn't include the memory refresh flag.
> ProcessesEventRouter::ListenerAdded()
> > > is where we start observing the task manager and add the memory refresh
flag
> > if
> > > a listener is interested in OnUpdatedWithMemory().
> > 
> > Right.  The case I'm trying to defend against is that an extension adds a
> > listener for, e.g., onProcessCreated.  When that happens, we add this as an
> > observer to the TM.  But if that's the only listener, there's no reason to
> make
> > the TM regenerate the metrics for all processes, when all we really need is
> the
> > onCreated signal - but we'd still have it update every second.  It'd be good
> to
> > only force the TM to refresh all the metrics if we have a consumer for
those.
> 
> Looks like you missed this one?

Yes, I did. Thanks for the reminder. All events
onCreated/onUpdated/onUpdatedWithMemory/onUnresponsive require sending some
stats about the process. The only event 'onExited' doesn't. But I see your point
now. Please take a look at the way things are now. If the refresh flags are set
to REFRESH_TYPE_NONE, then the task manager refresh will essentially do nothing.

[afakhry, 2016-03-10 18:56:53]

On 2016/03/09 18:46:37, afakhry wrote:
>
https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
> File chrome/browser/extensions/api/processes/processes_api.cc (right):
> 
>
https://codereview.chromium.org/1584473004/diff/360001/chrome/browser/extensi...
> chrome/browser/extensions/api/processes/processes_api.cc:178:
> base::TimeDelta::FromSeconds(1),
> On 2016/03/09 15:16:12, Devlin wrote:
> > On 2016/03/08 21:27:22, Devlin wrote:
> > > On 2016/03/08 01:37:29, afakhry wrote:
> > > > On 2016/03/03 23:12:43, Devlin wrote:
> > > > > If we don't have any listeners for OnUpdated[WithMemory], won't this
> cause
> > a
> > > > > good deal of extra work for the task manager?  Could we instead wait
> until
> > > we
> > > > > have a listener for OnUpdated and set the refresh time then? 
> (Optionally
> > > > > setting it for an obscenely long time here)
> > > > 
> > > > True. This doesn't add an observer to the task manager yet. It just sets
> up
> > > this
> > > > observer with its default settings.
> GetRefreshTypesFlagIncludeOptionalData()
> > > > doesn't include the memory refresh flag.
> > ProcessesEventRouter::ListenerAdded()
> > > > is where we start observing the task manager and add the memory refresh
> flag
> > > if
> > > > a listener is interested in OnUpdatedWithMemory().
> > > 
> > > Right.  The case I'm trying to defend against is that an extension adds a
> > > listener for, e.g., onProcessCreated.  When that happens, we add this as
an
> > > observer to the TM.  But if that's the only listener, there's no reason to
> > make
> > > the TM regenerate the metrics for all processes, when all we really need
is
> > the
> > > onCreated signal - but we'd still have it update every second.  It'd be
good
> > to
> > > only force the TM to refresh all the metrics if we have a consumer for
> those.
> > 
> > Looks like you missed this one?
> 
> Yes, I did. Thanks for the reminder. All events
> onCreated/onUpdated/onUpdatedWithMemory/onUnresponsive require sending some
> stats about the process. The only event 'onExited' doesn't. But I see your
point
> now. Please take a look at the way things are now. If the refresh flags are
set
> to REFRESH_TYPE_NONE, then the task manager refresh will essentially do
nothing.

Friendly ping.

[Devlin, 2016-03-10 19:02:39]

lgtm!

[ncarter (slow), 2016-03-10 21:01:04]

still lgtm, one nit

https://codereview.chromium.org/1584473004/diff/440001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/440001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:23: 
Weird newline here.

[afakhry, 2016-03-11 03:38:11]

https://codereview.chromium.org/1584473004/diff/440001/chrome/browser/extensi...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/1584473004/diff/440001/chrome/browser/extensi...
chrome/browser/extensions/api/processes/processes_api.cc:23: 
On 2016/03/10 21:01:04, ncarter wrote:
> Weird newline here.

Done.

[afakhry, 2016-03-11 03:38:32]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2016-03-11 03:38:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1584473004/#ps460001
(title: "Fix nit.")

[commit-bot: I haz the power, 2016-03-11 03:39:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1584473004/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1584473004/460001

[commit-bot: I haz the power, 2016-03-11 03:48:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 03:48:39]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[afakhry, 2016-03-11 17:42:24]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2016-03-11 17:43:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1584473004/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1584473004/460001

[commit-bot: I haz the power, 2016-03-11 19:27:57]

Description was changed from

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. 

The API of the process info has been fixed, since processes don't have titles,
instead we added a list of tasks running in that process, with their titles and
optional tab IDs.

BUG=525201,591581
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. 

The API of the process info has been fixed, since processes don't have titles,
instead we added a list of tasks running in that process, with their titles and
optional tab IDs.

BUG=525201,591581
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

[commit-bot: I haz the power, 2016-03-11 19:27:58]

Committed patchset #23 (id:460001)

[commit-bot: I haz the power, 2016-03-11 19:29:31]

Description was changed from

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation. 

The API of the process info has been fixed, since processes don't have titles,
instead we added a list of tasks running in that process, with their titles and
optional tab IDs.

BUG=525201,591581
TEST=browser_tests --gtest_filter=ProcessesApiTest.*
==========

to

==========
Migrate ProcessesEventRouter to the new task manager

This CL migrates the dependency of the ProcessesAPIs to the new task manager
implementation.

The API of the process info has been fixed, since processes don't have titles,
instead we added a list of tasks running in that process, with their titles and
optional tab IDs.

BUG=525201,591581
TEST=browser_tests --gtest_filter=ProcessesApiTest.*

Committed: https://crrev.com/98241839b528f945fabb03f38fb1ea6a3e837f09
Cr-Commit-Position: refs/heads/master@{#380696}
==========

[commit-bot: I haz the power, 2016-03-11 19:29:34]

Patchset 23 (id:??) landed as
https://crrev.com/98241839b528f945fabb03f38fb1ea6a3e837f09
Cr-Commit-Position: refs/heads/master@{#380696}