Add a task manager column that shows memory state of processes

Add a task manager column that shows memory state of processes

When memory coordinator is enabled, each process has its memory state.
This CL adds a column to the task manager to show memory states so that
users can see these states.

BUG=673216
Committed: https://crrev.com/1cccc93926ed0ff7cc3f5674834559a8747135b2
Cr-Commit-Position: refs/heads/master@{#441584}

[bashi, 2016-12-22 03:15:33]

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

[commit-bot: I haz the power, 2016-12-22 03:15:45]

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-12-22 03:39:31]

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

[commit-bot: I haz the power, 2016-12-22 03:39:32]

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_...)

[bashi, 2016-12-22 06:06:50]

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

[commit-bot: I haz the power, 2016-12-22 06:07:00]

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

[bashi, 2016-12-22 06:09:21]

Description was changed from

==========
WIP: Add a task manager column that shows memory state of processes

When memory coordinator is enabled, each process has its memory state.
This CL adds a column to the task manager to show memory states so that
users can see these states.

BUG=673216
==========

to

==========
Add a task manager column that shows memory state of processes

When memory coordinator is enabled, each process has its memory state.
This CL adds a column to the task manager to show memory states so that
users can see these states.

BUG=673216
==========

[commit-bot: I haz the power, 2016-12-22 07:17:57]

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

[commit-bot: I haz the power, 2016-12-22 07:17:58]

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_...)

[bashi, 2016-12-22 07:51:50]

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

[commit-bot: I haz the power, 2016-12-22 07:52:05]

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

[bashi, 2016-12-22 07:56:36]

bashi@chromium.org changed reviewers:
+ afakhry@chromium.org

[bashi, 2016-12-22 07:56:37]

afakhry@: PTAL?

[commit-bot: I haz the power, 2016-12-22 08:59:32]

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

[commit-bot: I haz the power, 2016-12-22 08:59:33]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-01-03 18:53:17]

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

https://codereview.chromium.org/2593233003/diff/40001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:66: case
IDS_TASK_MANAGER_PROCESS_PRIORITY_COLUMN:
Since the Memory State is a per-process value, please add
IDS_TASK_MANAGER_MEMORY_STATE_COLUMN here.

https://codereview.chromium.org/2593233003/diff/40001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:148: if
(base::FeatureList::IsEnabled(features::kMemoryCoordinator)) {
I noticed we do this test twice; here and in TaskGroup::Refresh(), so if the
feature is disabled, the value will be unknown. Can we just use N/A for
"unknown" and avoid repeating this test? Or does "unknown" convey something
important if the feature is enabled?

https://codereview.chromium.org/2593233003/diff/40001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:435: case
IDS_TASK_MANAGER_PROFILE_NAME_COLUMN:
Please add IDS_TASK_MANAGER_MEMORY_STATE_COLUMN so that we can sort the tasks by
the Memory State column, and make sure that it's working.

[bashi, 2017-01-05 00:09:05]

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

[commit-bot: I haz the power, 2017-01-05 00:09:36]

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

[bashi, 2017-01-05 00:51:49]

Thank you for review!

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

https://codereview.chromium.org/2593233003/diff/40001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:66: case
IDS_TASK_MANAGER_PROCESS_PRIORITY_COLUMN:
On 2017/01/03 18:53:16, afakhry wrote:
> Since the Memory State is a per-process value, please add
> IDS_TASK_MANAGER_MEMORY_STATE_COLUMN here.

Done.

https://codereview.chromium.org/2593233003/diff/40001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:148: if
(base::FeatureList::IsEnabled(features::kMemoryCoordinator)) {
On 2017/01/03 18:53:17, afakhry wrote:
> I noticed we do this test twice; here and in TaskGroup::Refresh(), so if the
> feature is disabled, the value will be unknown. Can we just use N/A for
> "unknown" and avoid repeating this test? Or does "unknown" convey something
> important if the feature is enabled?

Done.

https://codereview.chromium.org/2593233003/diff/40001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:435: case
IDS_TASK_MANAGER_PROFILE_NAME_COLUMN:
On 2017/01/03 18:53:17, afakhry wrote:
> Please add IDS_TASK_MANAGER_MEMORY_STATE_COLUMN so that we can sort the tasks
by
> the Memory State column, and make sure that it's working.

Done.

[afakhry, 2017-01-05 00:57:30]

lgtm with a nit.

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

https://codereview.chromium.org/2593233003/diff/60001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:150: // TODO(bashi):
i18n.
Nit: This TODO is very vague. Can you please explain what exactly that needs to
be done?

[bashi, 2017-01-05 01:05:16]

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

[commit-bot: I haz the power, 2017-01-05 01:05:45]

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

[bashi, 2017-01-05 01:05:52]

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

https://codereview.chromium.org/2593233003/diff/60001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:150: // TODO(bashi):
i18n.
On 2017/01/05 00:57:30, afakhry wrote:
> Nit: This TODO is very vague. Can you please explain what exactly that needs
to
> be done?

Done.

[afakhry, 2017-01-05 01:27:11]

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

https://codereview.chromium.org/2593233003/diff/60001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:150: // TODO(bashi):
i18n.
On 2017/01/05 01:05:52, bashi1 (ooo Dec 28 - Jan 4) wrote:
> On 2017/01/05 00:57:30, afakhry wrote:
> > Nit: This TODO is very vague. Can you please explain what exactly that needs
> to
> > be done?
> 
> Done.

In this case it's quite simple to do here. Consider what we did for the process
priority. Take a look at the TaskManagerValuesStringifier constructor and the
way we preload the localized backgrounded/foregrounded values.

[commit-bot: I haz the power, 2017-01-05 01:54:27]

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

[commit-bot: I haz the power, 2017-01-05 01:54:29]

Dry run: This issue passed the CQ dry run.

[bashi, 2017-01-05 02:18:22]

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

https://codereview.chromium.org/2593233003/diff/60001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:150: // TODO(bashi):
i18n.
On 2017/01/05 01:27:10, afakhry wrote:
> On 2017/01/05 01:05:52, bashi1 (ooo Dec 28 - Jan 4) wrote:
> > On 2017/01/05 00:57:30, afakhry wrote:
> > > Nit: This TODO is very vague. Can you please explain what exactly that
needs
> > to
> > > be done?
> > 
> > Done.
> 
> In this case it's quite simple to do here. Consider what we did for the
process
> priority. Take a look at the TaskManagerValuesStringifier constructor and the
> way we preload the localized backgrounded/foregrounded values.

Thanks for the suggestion. The MemoryState enums aren't stabilized yet (we may
add/remove states) and I thought it would be better to localize them later. But
on second thought I think we should localize them anyway. Done.

[bashi, 2017-01-05 02:22:20]

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

[commit-bot: I haz the power, 2017-01-05 02:22:42]

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-05 02:50:25]

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

[commit-bot: I haz the power, 2017-01-05 02:50:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bashi, 2017-01-05 02:54:21]

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

[commit-bot: I haz the power, 2017-01-05 02:54:39]

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-05 03:48:18]

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

[commit-bot: I haz the power, 2017-01-05 03:48:19]

Dry run: This issue passed the CQ dry run.

[bashi, 2017-01-05 04:19:55]

The CQ bit was checked by bashi@chromium.org

[bashi, 2017-01-05 04:19:55]

The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org
Link to the patchset: https://codereview.chromium.org/2593233003/#ps120001
(title: "fix compile error")

[commit-bot: I haz the power, 2017-01-05 04:20:24]

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-05 04:24:46]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483589995016650,
"parent_rev": "e3fbe6fef0fe6bc79ddfaaf4bdc1dcaaaca4a81a", "commit_rev":
"4f81954b07f0c92e7199af52668deb380393d050"}

[commit-bot: I haz the power, 2017-01-05 04:25:14]

Description was changed from

==========
Add a task manager column that shows memory state of processes

When memory coordinator is enabled, each process has its memory state.
This CL adds a column to the task manager to show memory states so that
users can see these states.

BUG=673216
==========

to

==========
Add a task manager column that shows memory state of processes

When memory coordinator is enabled, each process has its memory state.
This CL adds a column to the task manager to show memory states so that
users can see these states.

BUG=673216

Review-Url: https://codereview.chromium.org/2593233003
==========

[commit-bot: I haz the power, 2017-01-05 04:25:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2017-01-05 04:29:12]

Description was changed from

==========
Add a task manager column that shows memory state of processes

When memory coordinator is enabled, each process has its memory state.
This CL adds a column to the task manager to show memory states so that
users can see these states.

BUG=673216

Review-Url: https://codereview.chromium.org/2593233003
==========

to

==========
Add a task manager column that shows memory state of processes

When memory coordinator is enabled, each process has its memory state.
This CL adds a column to the task manager to show memory states so that
users can see these states.

BUG=673216

Committed: https://crrev.com/1cccc93926ed0ff7cc3f5674834559a8747135b2
Cr-Commit-Position: refs/heads/master@{#441584}
==========

[commit-bot: I haz the power, 2017-01-05 04:29:13]

Patchset 7 (id:??) landed as
https://crrev.com/1cccc93926ed0ff7cc3f5674834559a8747135b2
Cr-Commit-Position: refs/heads/master@{#441584}

[afakhry, 2017-01-05 17:00:20]

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

https://codereview.chromium.org/2593233003/diff/60001/chrome/browser/ui/task_...
chrome/browser/ui/task_manager/task_manager_table_model.cc:150: // TODO(bashi):
i18n.
On 2017/01/05 02:18:22, bashi wrote:
> On 2017/01/05 01:27:10, afakhry wrote:
> > On 2017/01/05 01:05:52, bashi1 (ooo Dec 28 - Jan 4) wrote:
> > > On 2017/01/05 00:57:30, afakhry wrote:
> > > > Nit: This TODO is very vague. Can you please explain what exactly that
> needs
> > > to
> > > > be done?
> > > 
> > > Done.
> > 
> > In this case it's quite simple to do here. Consider what we did for the
> process
> > priority. Take a look at the TaskManagerValuesStringifier constructor and
the
> > way we preload the localized backgrounded/foregrounded values.
> 
> Thanks for the suggestion. The MemoryState enums aren't stabilized yet (we may
> add/remove states) and I thought it would be better to localize them later.
But
> on second thought I think we should localize them anyway. Done.

Perfect, thanks!