Improve TDI render process naming in the task manager.

Improve TDI render process naming in the task manager.

BUG=641776
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/85cacb7b374d9018a67202f6082a4100423380b2
Cr-Commit-Position: refs/heads/master@{#427855}

[Avi (use Gerrit), 2016-10-20 22:48:43]

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

[Avi (use Gerrit), 2016-10-20 22:48:43]

Description was changed from

==========
Improve TDI render process naming in the task manager.

BUG=641776
==========

to

==========
Improve TDI render process naming in the task manager.

BUG=641776
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-20 22:49:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-10-20 22:52:03]

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

[commit-bot: I haz the power, 2016-10-20 22:52:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 00:59:27]

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

[commit-bot: I haz the power, 2016-10-21 00:59:28]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-24 20:49:51]

avi@chromium.org changed reviewers:
+ nick@chromium.org, ojan@chromium.org

[ncarter (slow), 2016-10-24 22:55:31]

https://codereview.chromium.org/2437253002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2437253002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:3550: +        <message
name="IDS_TASK_MANAGER_ISOLATED_INCOGNITO_SUBFRAMES_PREFIX" desc="The prefix for
an Incognito out-of-process-iframe process row in the Task Manager for subframes
that were isolated because of 'top document isolation' mode">
"process row" -> "row" ?

https://codereview.chromium.org/2437253002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_manager/providers/web_contents/subframe_task.cc
(right):

https://codereview.chromium.org/2437253002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/subframe_task.cc:40: void
SubframeTask::UpdateTitle() {
Could you add a task_manager_browsertest that does the following:

 - enables with TDI (you'll need to skip the test in --site-per-process)

 - Opens a(b(c)) or something similar

 - Waits for titles on the parent and child rows.

 - Updates the title via script in the main frame.

 - Waits for a title change for the subframe row.

IIUC this ought to work given the original comment in ::UpdateTitle, but I can
see that regressing.

[Avi (use Gerrit), 2016-10-25 21:38:18]

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

[commit-bot: I haz the power, 2016-10-25 21:38:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 23:02:33]

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

[commit-bot: I haz the power, 2016-10-25 23:02:34]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-26 00:41:39]

This should work now. Nick, wdyt?

https://codereview.chromium.org/2437253002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2437253002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:3550: +        <message
name="IDS_TASK_MANAGER_ISOLATED_INCOGNITO_SUBFRAMES_PREFIX" desc="The prefix for
an Incognito out-of-process-iframe process row in the Task Manager for subframes
that were isolated because of 'top document isolation' mode">
On 2016/10/24 22:55:31, ncarter wrote:
> "process row" -> "row" ?

Done.

https://codereview.chromium.org/2437253002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_manager/providers/web_contents/subframe_task.cc
(right):

https://codereview.chromium.org/2437253002/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/subframe_task.cc:40: void
SubframeTask::UpdateTitle() {
On 2016/10/24 22:55:31, ncarter wrote:
> Could you add a task_manager_browsertest that does the following:
> 
>  - enables with TDI (you'll need to skip the test in --site-per-process)
> 
>  - Opens a(b(c)) or something similar
> 
>  - Waits for titles on the parent and child rows.
> 
>  - Updates the title via script in the main frame.
> 
>  - Waits for a title change for the subframe row.
> 
> IIUC this ought to work given the original comment in ::UpdateTitle, but I can
> see that regressing.

Done.

[ncarter (slow), 2016-10-26 17:45:27]

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
File
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
(right):

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:179:
main_frame_task->UpdateRapporSampleName();
Maybe do this only if navigation_handle->IsInMainFrame()?

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:181:
for (const auto& frame_pair : tasks_by_frames_) {
In the a(b, b) case, or the non-oopif case, this will visit some tasks several
times, since a Task can correspond to several frames and if so, the Task will
appear multiple times in |tasks_by_frames_|.

To visit each task exactly once, use the loop pattern seen in
WebContentsEntry::ClearAllTasks:

for (const auto& pair : frames_by_site_instance_) {
  const FramesList& frames_list = pair.second;
  DCHECK(!frames_list.empty());
  RendererTask* task = tasks_by_frames_[frames_list[0]];
}

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:197:
for (const auto& frame_pair : tasks_by_frames_) {
Same here. Might be worth extracting a function to do this loop.

[Avi (use Gerrit), 2016-10-26 19:08:26]

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

[commit-bot: I haz the power, 2016-10-26 19:09:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 20:34:04]

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

[commit-bot: I haz the power, 2016-10-26 20:34:05]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-26 20:37:28]

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
File
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
(right):

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:179:
main_frame_task->UpdateRapporSampleName();
On 2016/10/26 17:45:27, ncarter wrote:
> Maybe do this only if navigation_handle->IsInMainFrame()?

Maybe? I'm not sure if that would change the rappor metric, adding that
qualification. Are you suggesting that's something I should do?

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:181:
for (const auto& frame_pair : tasks_by_frames_) {
On 2016/10/26 17:45:27, ncarter wrote:
> In the a(b, b) case, or the non-oopif case, this will visit some tasks several
> times, since a Task can correspond to several frames and if so, the Task will
> appear multiple times in |tasks_by_frames_|.
> 
> To visit each task exactly once, use the loop pattern seen in
> WebContentsEntry::ClearAllTasks:
> 
> for (const auto& pair : frames_by_site_instance_) {
>   const FramesList& frames_list = pair.second;
>   DCHECK(!frames_list.empty());
>   RendererTask* task = tasks_by_frames_[frames_list[0]];
> }

Done.

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:197:
for (const auto& frame_pair : tasks_by_frames_) {
On 2016/10/26 17:45:27, ncarter wrote:
> Same here. Might be worth extracting a function to do this loop.

Done.

[ncarter (slow), 2016-10-26 20:55:48]

Awesome. lgtm

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
File
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
(right):

https://codereview.chromium.org/2437253002/diff/40001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:179:
main_frame_task->UpdateRapporSampleName();
On 2016/10/26 20:37:28, Avi wrote:
> On 2016/10/26 17:45:27, ncarter wrote:
> > Maybe do this only if navigation_handle->IsInMainFrame()?
> 
> Maybe? I'm not sure if that would change the rappor metric, adding that
> qualification. Are you suggesting that's something I should do?

Ah, ignore this comment; I was mistaken about what was going on here.

[Avi (use Gerrit), 2016-10-26 22:33:09]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2016-10-26 22:33:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 22:39:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-26 22:42:48]

Description was changed from

==========
Improve TDI render process naming in the task manager.

BUG=641776
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Improve TDI render process naming in the task manager.

BUG=641776
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/85cacb7b374d9018a67202f6082a4100423380b2
Cr-Commit-Position: refs/heads/master@{#427855}
==========

[commit-bot: I haz the power, 2016-10-26 22:42:50]

Patchset 4 (id:??) landed as
https://crrev.com/85cacb7b374d9018a67202f6082a4100423380b2
Cr-Commit-Position: refs/heads/master@{#427855}