cocoa: add --use-views-task-manager

cocoa: add ViewsTaskManager feature

This feature uses the Views task manager instead of the Cocoa one. It looks strange
unless you also use --secondary-ui-md to get the Harmony controls.

BUG=670579
Review-Url: https://codereview.chromium.org/2724923004
Cr-Commit-Position: refs/heads/master@{#457073}
Committed: https://chromium.googlesource.com/chromium/src/+/63762e98af73be680ff4d8237cee0e2240faf397

[Elly Fong-Jones, 2017-03-02 18:43:58]

Description was changed from

==========
cocoa: add --use-views-task-manager

This flag uses the Views task manager instead of the Cocoa one. It looks strange
unless you also use --secondary-ui-md to get the Harmony controls.

BUG=670579
==========

to

==========
cocoa: add --use-views-task-manager

This flag uses the Views task manager instead of the Cocoa one. It looks strange
unless you also use --secondary-ui-md to get the Harmony controls.

BUG=670579
==========

[Elly Fong-Jones, 2017-03-02 18:43:58]

ellyjones@chromium.org changed reviewers:
+ tapted@chromium.org

[Elly Fong-Jones, 2017-03-02 18:44:05]

tapted: ptal? :)

[shrike, 2017-03-02 18:58:24]

shrike@chromium.org changed reviewers:
+ shrike@chromium.org

[shrike, 2017-03-02 18:58:24]

Should --secondary-ui-md maybe be implicit for this (is there a reason not to
always run that way)?

[Elly Fong-Jones, 2017-03-02 19:16:14]

On 2017/03/02 18:58:24, shrike wrote:
> Should --secondary-ui-md maybe be implicit for this (is there a reason not to
> always run that way)?

I guess it could, but I don't really like switches implying other switches, just
aesthetically. I could make this switch do nothing if you don't have Harmony
turned on?

[shrike, 2017-03-02 19:25:02]

On 2017/03/02 19:16:14, Elly Fong-Jones wrote:
> On 2017/03/02 18:58:24, shrike wrote:
> > Should --secondary-ui-md maybe be implicit for this (is there a reason not
to
> > always run that way)?
> 
> I guess it could, but I don't really like switches implying other switches,
just
> aesthetically. I could make this switch do nothing if you don't have Harmony
> turned on?

I don't have real strong feelings about it, so whichever way you go is OK with
me.

[tapted, 2017-03-02 22:44:40]

On 2017/03/02 19:25:02, shrike wrote:
> On 2017/03/02 19:16:14, Elly Fong-Jones wrote:
> > On 2017/03/02 18:58:24, shrike wrote:
> > > Should --secondary-ui-md maybe be implicit for this (is there a reason not
> to
> > > always run that way)?
> > 
> > I guess it could, but I don't really like switches implying other switches,
> just
> > aesthetically. I could make this switch do nothing if you don't have Harmony
> > turned on?
> 
> I don't have real strong feelings about it, so whichever way you go is OK with
> me.

I think looking weird is easier to diagnose than not appearing at all. And I
don't have any great ideas for coupling together the switches - I think keeping
them uncoupled is the right way to go about it.

https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
File chrome/browser/ui/cocoa/task_manager_mac.mm (right):

https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
chrome/browser/ui/cocoa/task_manager_mac.mm:23: #include
"chrome/common/chrome_switches.h"
I think base/feature_list.h is the shiny new way to do all this. Pretty sure
then the changes are wholly contained in task_manager_mac.mm, plus we get
easy/automatic Finch integration if we want that, and a nicer avenue to add a
chrome://flag (if we want that).

The downside is just a little less convenience on the command line (i.e.
--enable_features=..).

Actually, I think we want the chrome://flag entry too -- wdyt?

[shrike, 2017-03-02 23:58:05]

https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
File chrome/browser/ui/cocoa/task_manager_mac.mm (right):

https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
chrome/browser/ui/cocoa/task_manager_mac.mm:23: #include
"chrome/common/chrome_switches.h"
On 2017/03/02 22:44:39, tapted wrote:
> Actually, I think we want the chrome://flag entry too -- wdyt?

We do need a chrome://flag entry. I look at MacViews/Harmony changes in Canary
and don't generally run it from the command line.

[Elly Fong-Jones, 2017-03-07 18:02:07]

On 2017/03/02 23:58:05, shrike wrote:
>
https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
> File chrome/browser/ui/cocoa/task_manager_mac.mm (right):
> 
>
https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
> chrome/browser/ui/cocoa/task_manager_mac.mm:23: #include
> "chrome/common/chrome_switches.h"
> On 2017/03/02 22:44:39, tapted wrote:
> > Actually, I think we want the chrome://flag entry too -- wdyt?
> 
> We do need a chrome://flag entry. I look at MacViews/Harmony changes in Canary
> and don't generally run it from the command line.

Okay, added a flag.

tapted: what do you think?

[tapted, 2017-03-07 22:52:26]

https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
File chrome/browser/ui/cocoa/task_manager_mac.mm (right):

https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
chrome/browser/ui/cocoa/task_manager_mac.mm:23: #include
"chrome/common/chrome_switches.h"
On 2017/03/02 22:44:39, tapted wrote:
> I think base/feature_list.h is the shiny new way to do all this. Pretty sure
> then the changes are wholly contained in task_manager_mac.mm, plus we get
> easy/automatic Finch integration if we want that, and a nicer avenue to add a
> chrome://flag (if we want that).
> 
> The downside is just a little less convenience on the command line (i.e.
> --enable_features=..).


Per ^^^, is there a reason to not want to use base::FeatureList?

https://codereview.chromium.org/2724923004/diff/20001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2724923004/diff/20001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1643:
SINGLE_VALUE_TYPE(switches::kUseViewsTaskManager)},
(with FeatureList) FEATURE_VALUE_TYPE?

[Elly Fong-Jones, 2017-03-08 17:47:21]

On 2017/03/07 22:52:26, tapted wrote:
>
https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
> File chrome/browser/ui/cocoa/task_manager_mac.mm (right):
> 
>
https://codereview.chromium.org/2724923004/diff/1/chrome/browser/ui/cocoa/tas...
> chrome/browser/ui/cocoa/task_manager_mac.mm:23: #include
> "chrome/common/chrome_switches.h"
> On 2017/03/02 22:44:39, tapted wrote:
> > I think base/feature_list.h is the shiny new way to do all this. Pretty sure
> > then the changes are wholly contained in task_manager_mac.mm, plus we get
> > easy/automatic Finch integration if we want that, and a nicer avenue to add
a
> > chrome://flag (if we want that).
> > 
> > The downside is just a little less convenience on the command line (i.e.
> > --enable_features=..).
> 
> 
> Per ^^^, is there a reason to not want to use base::FeatureList?
> 
>
https://codereview.chromium.org/2724923004/diff/20001/chrome/browser/about_fl...
> File chrome/browser/about_flags.cc (right):
> 
>
https://codereview.chromium.org/2724923004/diff/20001/chrome/browser/about_fl...
> chrome/browser/about_flags.cc:1643:
> SINGLE_VALUE_TYPE(switches::kUseViewsTaskManager)},
> (with FeatureList) FEATURE_VALUE_TYPE?

ah, I forgot about your suggestion to use base::Feature, sorry :) I changed the
code over to that now.

[tapted, 2017-03-08 23:08:08]

https://codereview.chromium.org/2724923004/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2724923004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
CL Description: don't forget to update it :)

https://codereview.chromium.org/2724923004/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:33: #include "chrome/common/chrome_features.h"
do features belong in this file?  (i.e. chrome_features.h instead of
chrome_switches.h)

https://codereview.chromium.org/2724923004/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/task_manager_mac.mm (right):

https://codereview.chromium.org/2724923004/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/task_manager_mac.mm:12: #include "base/command_line.h"
nit: remove

[Elly Fong-Jones, 2017-03-10 17:39:54]

Description was changed from

==========
cocoa: add --use-views-task-manager

This flag uses the Views task manager instead of the Cocoa one. It looks strange
unless you also use --secondary-ui-md to get the Harmony controls.

BUG=670579
==========

to

==========
cocoa: add ViewsTaskManager feature

This feature uses the Views task manager instead of the Cocoa one. It looks
strange
unless you also use --secondary-ui-md to get the Harmony controls.

BUG=670579
==========

[Elly Fong-Jones, 2017-03-10 17:40:08]

tapted: ptal? :)

[tapted, 2017-03-13 02:03:06]

lgtm % nits

https://codereview.chromium.org/2724923004/diff/60001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2724923004/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:24: const base::Feature
kViewsTaskManager{"ViewsTaskManager",
nit: add a comment. (see chrome_features.h:18) E.g.

// Use the Toolkit-Views based Task Manager window.

https://codereview.chromium.org/2724923004/diff/60001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/2724923004/diff/60001/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:11: #include "base/feature_list.h"
nit: remove

[Elly Fong-Jones, 2017-03-13 15:22:27]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2017-03-13 15:22:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2724923004/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2017-03-13 15:22:47]

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-03-13 16:18:21]

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

[commit-bot: I haz the power, 2017-03-13 16:18:22]

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

[Elly Fong-Jones, 2017-03-14 17:20:58]

ellyjones@chromium.org changed reviewers:
+ isherman@chromium.org

[Elly Fong-Jones, 2017-03-14 17:21:09]

isherman: ptal as histograms owner? :)

[Ilya Sherman, 2017-03-14 18:40:24]

histograms.xml lgtm

[Elly Fong-Jones, 2017-03-15 13:07:48]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2017-03-15 13:07:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2724923004/#ps100001
(title: "add histogram enum entries")

[commit-bot: I haz the power, 2017-03-15 13:08:01]

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-03-15 14:04:37]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489583268731360,
"parent_rev": "49cf2866637ab37a2ef00e0b8abdcf761790d935", "commit_rev":
"63762e98af73be680ff4d8237cee0e2240faf397"}

[commit-bot: I haz the power, 2017-03-15 14:05:23]

Description was changed from

==========
cocoa: add ViewsTaskManager feature

This feature uses the Views task manager instead of the Cocoa one. It looks
strange
unless you also use --secondary-ui-md to get the Harmony controls.

BUG=670579
==========

to

==========
cocoa: add ViewsTaskManager feature

This feature uses the Views task manager instead of the Cocoa one. It looks
strange
unless you also use --secondary-ui-md to get the Harmony controls.

BUG=670579

Review-Url: https://codereview.chromium.org/2724923004
Cr-Commit-Position: refs/heads/master@{#457073}
Committed:
https://chromium.googlesource.com/chromium/src/+/63762e98af73be680ff4d8237cee...
==========

[commit-bot: I haz the power, 2017-03-15 14:05:25]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/63762e98af73be680ff4d8237cee...