Add a "Keepalive count" column to Task Manager.

Add a "Keepalive count" column to Task Manager.

This column applies only to extensions' lazy event pages, displaying the
number of active references to them - all other Tasks will display the
N/A string.

BUG=616636
Review-Url: https://codereview.chromium.org/2653563003
Cr-Commit-Position: refs/heads/master@{#447433}
Committed: https://chromium.googlesource.com/chromium/src/+/87ddb9993bf9cfa284c48659ce8b7f5391408e85

[Wez, 2017-01-23 18:26:43]

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

[commit-bot: I haz the power, 2017-01-23 18:27:28]

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, 2017-01-23 18:50:34]

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

[commit-bot: I haz the power, 2017-01-23 18:50:35]

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

[Wez, 2017-01-23 22:40:02]

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

[commit-bot: I haz the power, 2017-01-23 22:41:10]

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

[Wez, 2017-01-23 22:56:00]

wez@chromium.org changed reviewers:
+ nick@chromium.org, rdevlin.cronin@chromium.org

[Wez, 2017-01-23 22:56:01]

rdevlin.cronin: PTAL wrt exposing the lazy-event-page keepalive count to Task
Manager - that OK?

nick: PTAL at Task Manager specifics.

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

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/extension_task.cc:98: if
(view_type_ != extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE)
Devlin: Strictly the keepalive count is on the extension host/process, but it
seemed more natural to display it associated with the extension's background
page, since that's the Task whose lifetime matches that of the process - WDYT?

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/task_man...
File chrome/browser/task_manager/task_manager_observer.h (right):

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager_observer.h:25: enum RefreshType {
FYI: git cl format did this, not me.

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/ui/task_...
File chrome/browser/ui/task_manager/task_manager_columns.cc (left):

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_columns.cc:128:
def(IDS_TASK_MANAGER_MEMORY_STATE_COLUMN)
git cl format made a horrible mess of this, because the lack of punctuation
apparently confuses it, so I restructured it to avoid this big macro entirely.

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/ui/task_...
File chrome/browser/ui/task_manager/task_manager_columns.cc (right):

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_columns.cc:23: const TableColumnData
kColumns[] = {
FYI: Similarly, git cl format did this.

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/ui/task_...
File chrome/browser/ui/task_manager/task_manager_table_model.cc (right):

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:487: int column_id) {
FYI: I've not added anything for the keepalive column in here, because it
doesn't seem useful to be able to sort based on it.

[commit-bot: I haz the power, 2017-01-23 23:04:22]

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

[commit-bot: I haz the power, 2017-01-23 23:04:23]

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

[Devlin, 2017-01-23 23:52:41]

Cool!  Nick's a much better reviewer here, so I'm mostly deferring to him, but
excited to see this!

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

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/extension_task.cc:98: if
(view_type_ != extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE)
On 2017/01/23 22:56:01, Wez wrote:
> Devlin: Strictly the keepalive count is on the extension host/process, but it
> seemed more natural to display it associated with the extension's background
> page, since that's the Task whose lifetime matches that of the process - WDYT?

Overall, this - exposing a single count for an extension even though the count
may be from other views - sounds fine.  That said, there are some of these
columns in the TaskManager that are shared per-process - e.g. memory, cpu, and
network are all lumped together for all items in the same process.  I'm not sure
what the TaskManager-y way of doing that is (I assume Nick knows), but perhaps
handling it at that level, instead of by special-casing the background page, is
a better fit?  We don't quite want it one-per-process (because extensions can
share a process), but perhaps we can have a one-per-extension?

Or, perhaps it totally doesn't matter.  I'll defer to Nick's opinion.

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/extension_task.cc:104: if
(!extension)
Nick: When does this happen?  (It seems like it's copied from other places in
the file)

[Wez, 2017-01-23 23:59:54]

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

https://codereview.chromium.org/2653563003/diff/20001/chrome/browser/task_man...
chrome/browser/task_manager/providers/web_contents/extension_task.cc:98: if
(view_type_ != extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE)
On 2017/01/23 23:52:36, Devlin (catching up) wrote:
> On 2017/01/23 22:56:01, Wez wrote:
> > Devlin: Strictly the keepalive count is on the extension host/process, but
it
> > seemed more natural to display it associated with the extension's background
> > page, since that's the Task whose lifetime matches that of the process -
WDYT?
> 
> Overall, this - exposing a single count for an extension even though the count
> may be from other views - sounds fine.  That said, there are some of these
> columns in the TaskManager that are shared per-process - e.g. memory, cpu, and
> network are all lumped together for all items in the same process.  I'm not
sure
> what the TaskManager-y way of doing that is (I assume Nick knows), but perhaps
> handling it at that level, instead of by special-casing the background page,
is
> a better fit?  We don't quite want it one-per-process (because extensions can
> share a process), but perhaps we can have a one-per-extension?
> 
> Or, perhaps it totally doesn't matter.  I'll defer to Nick's opinion.

This may be moot, given site-isolation for extensions, but yes, I thought that
having this be associated with the extension rather than the process made most
sense, since that's where the keep-alive count is.

That said, presumably there is some underlying keep-alive count at the renderer
process level; perhaps that should also be exposed, on a per-TaskGroup (aka
per-process) basis?

[Wez, 2017-01-24 00:16:30]

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

[commit-bot: I haz the power, 2017-01-24 00:17:26]

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, 2017-01-24 00:47:23]

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

[commit-bot: I haz the power, 2017-01-24 00:47:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Wez, 2017-01-25 00:51:35]

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

[Wez, 2017-01-25 00:51:52]

nick: Ping

[commit-bot: I haz the power, 2017-01-25 00:52:09]

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, 2017-01-25 01:23:19]

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

[commit-bot: I haz the power, 2017-01-25 01:23:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Wez, 2017-01-26 22:13:40]

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

[Wez, 2017-01-26 22:13:53]

nick: Ping!

[commit-bot: I haz the power, 2017-01-26 22:14:12]

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, 2017-01-26 23:08:38]

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

[commit-bot: I haz the power, 2017-01-26 23:08:39]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-01-30 18:52:32]

Pingaling. :)

[ncarter (slow), 2017-01-31 19:56:45]

lgtm with one nit

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

https://codereview.chromium.org/2653563003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:3477: +          <message
name="IDS_TASK_MANAGER_KEEPALIVE_COUNT_COLUMN" desc="Task manager keepalive
count column.  Shows the keepalive count of background pages">
These should go into a little more depth since the keep-alive count is
crx-specific technical jargon; translation team members have filed bugs against
us before, if we aren't sufficiently specific in these descriptions.

Maybe:

Label for the "keep-alive count" column in the task manager. Relevant only for
chrome extension processes, this column shows a count of the activities that are
keeping the background page for a particular chrome extension alive."

[Wez, 2017-02-01 01:34:43]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-02-01 01:34:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2653563003/#ps80001
(title: "Address review comment re description")

[commit-bot: I haz the power, 2017-02-01 01:35:15]

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, 2017-02-01 02:58:49]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485912883524510,
"parent_rev": "4f83247321010b850e65b52e861400650d99880a", "commit_rev":
"87ddb9993bf9cfa284c48659ce8b7f5391408e85"}

[commit-bot: I haz the power, 2017-02-01 02:59:24]

Description was changed from

==========
Add a "Keepalive count" column to Task Manager.

This column applies only to extensions' lazy event pages, displaying the
number of active references to them - all other Tasks will display the
N/A string.

BUG=616636
==========

to

==========
Add a "Keepalive count" column to Task Manager.

This column applies only to extensions' lazy event pages, displaying the
number of active references to them - all other Tasks will display the
N/A string.

BUG=616636

Review-Url: https://codereview.chromium.org/2653563003
Cr-Commit-Position: refs/heads/master@{#447433}
Committed:
https://chromium.googlesource.com/chromium/src/+/87ddb9993bf9cfa284c48659ce8b...
==========

[commit-bot: I haz the power, 2017-02-01 02:59:25]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/87ddb9993bf9cfa284c48659ce8b...

[Wez, 2017-02-01 03:03:26]

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

https://codereview.chromium.org/2653563003/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:3477: +          <message
name="IDS_TASK_MANAGER_KEEPALIVE_COUNT_COLUMN" desc="Task manager keepalive
count column.  Shows the keepalive count of background pages">
On 2017/01/31 19:56:45, ncarter wrote:
> These should go into a little more depth since the keep-alive count is
> crx-specific technical jargon; translation team members have filed bugs
against
> us before, if we aren't sufficiently specific in these descriptions.
> 
> Maybe:
> 
> Label for the "keep-alive count" column in the task manager. Relevant only for
> chrome extension processes, this column shows a count of the activities that
are
> keeping the background page for a particular chrome extension alive."
> 

Thanks - good catch. Have updated the wording for the two instances.