color: Use base::Feature to control color correct rendering

color: Use base::Feature to control color correct rendering

Add an explicit enable command line flag in
ShellMainDelegate::BasicStartupComplete to enable this feature
for layout tests, because it is already enabled for layout tests.

R=asvitkine
TBR=avi (content/), xiyuan (chrome/browser/chromeos/login)
BUG=713889
Review-Url: https://codereview.chromium.org/2945453002
Cr-Commit-Position: refs/heads/master@{#480262}
Committed: https://chromium.googlesource.com/chromium/src/+/708f986f71d8059c05c0f05b37d8c798a877ffde

[ccameron, 2017-06-15 19:50:53]

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

[commit-bot: I haz the power, 2017-06-15 19:51:31]

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-06-15 19:55:24]

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

[commit-bot: I haz the power, 2017-06-15 19:55:25]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[ccameron, 2017-06-15 19:56:04]

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

[commit-bot: I haz the power, 2017-06-15 19:56:34]

Dry run: CQ is trying da patch.

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

[ccameron, 2017-06-15 20:23:54]

ccameron@chromium.org changed reviewers:
+ xhwang@chromium.org

[ccameron, 2017-06-15 20:23:55]

ptal, since this goes against the suggestion in base/feature.h

I feel quite convinced that this is the right move, but I'd like to make sure
everyone is on the same page.

https://codereview.chromium.org/2945453002/diff/20001/content/shell/app/shell...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/2945453002/diff/20001/content/shell/app/shell...
content/shell/app/shell_main_delegate.cc:236: 
This is doing what was explicitly not supposed to be done in
https://cs.chromium.org/chromium/src/base/feature_list.h?rcl=8d1a0d180b7a9131...

Believe this this is the right thing to do, and is in line with what the virtual
test suites do
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/VirtualTe...

This also gets executed before the command line is parsed, and is to last for
the lifetime of the browser, so I think it's the right thing to do.

[ccameron, 2017-06-15 20:54:57]

ccameron@chromium.org changed reviewers:
+ asvitkine@google.com

[ccameron, 2017-06-15 20:54:58]

+asvitkine for base::Feature-ness.

[commit-bot: I haz the power, 2017-06-15 21:01:47]

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

[commit-bot: I haz the power, 2017-06-15 21:01:48]

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

[Alexei Svitkine (slow), 2017-06-15 21:04:40]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[Alexei Svitkine (slow), 2017-06-15 21:04:41]

Asking some clarifying questions.

https://codereview.chromium.org/2945453002/diff/20001/content/shell/app/shell...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/2945453002/diff/20001/content/shell/app/shell...
content/shell/app/shell_main_delegate.cc:236: 
On 2017/06/15 20:23:55, ccameron wrote:
> This is doing what was explicitly not supposed to be done in
>
https://cs.chromium.org/chromium/src/base/feature_list.h?rcl=8d1a0d180b7a9131...
> 
> Believe this this is the right thing to do, and is in line with what the
virtual
> test suites do
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/VirtualTe...
> 
> This also gets executed before the command line is parsed, and is to last for
> the lifetime of the browser, so I think it's the right thing to do.

(Just trying to understand.)

Is the goal to enable it for all tests? Why that and not set it on by default
everywhere? i.e. why test something different than what we ship to users? Have
you looked at whether go/field-trial-testing-config would achieve what you're
after?

[xhwang, 2017-06-15 21:11:12]

xhwang@chromium.org changed reviewers:
- xhwang@chromium.org

[xhwang, 2017-06-15 21:11:12]

Alexei is the expert on base::Feature. I'll defer the review of this CL to him.

[ccameron, 2017-06-15 21:26:25]

https://codereview.chromium.org/2945453002/diff/20001/content/shell/app/shell...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/2945453002/diff/20001/content/shell/app/shell...
content/shell/app/shell_main_delegate.cc:236: 
(related oops -- this was supposed to be in the "if in layout test" block, not
outside of it -- fixed in the next patchset).

On 2017/06/15 21:04:40, Alexei Svitkine (slow) wrote:
> On 2017/06/15 20:23:55, ccameron wrote:
> > This is doing what was explicitly not supposed to be done in
> >
>
https://cs.chromium.org/chromium/src/base/feature_list.h?rcl=8d1a0d180b7a9131...
> > 
> > Believe this this is the right thing to do, and is in line with what the
> virtual
> > test suites do
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/VirtualTe...
> > 
> > This also gets executed before the command line is parsed, and is to last
for
> > the lifetime of the browser, so I think it's the right thing to do.
> 
> (Just trying to understand.)
> 
> Is the goal to enable it for all tests? Why that and not set it on by default
> everywhere? i.e. why test something different than what we ship to users? Have
> you looked at whether go/field-trial-testing-config would achieve what you're
> after?

For the moment is just for layout tests -- I've already enabled it
just-on-layout tests (which required lots of rounding error rebaselines). Since
then realized that using a base::Feature instead of a --enableX flag would make
it easier to roll out, mostly to allow things like
go/field-trial-testing-config, and to make changing default behavior easier (I
didn't know about base::Feature until recently).

The idea of enabling for just-layout-tests-first was to get a manageable bug
stream -- the layout tests found a few dozen bugs, and now that we have no known
bugs there, I'm starting to enable for all tests, and then to users.

This is going to be enabled for all users soon (M61 deadline for Android O DR),
and the feature flag will be deleted in M62 (along with this AppendSwitch call,
unless someone else starts using it before it goes away).

[Alexei Svitkine (slow), 2017-06-16 14:47:25]

Ok, seems reasonable to me - but can you mention the purpose (layout tests) in
the CL description?

lgtm

https://codereview.chromium.org/2945453002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2945453002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:335: if
(base::FeatureList::IsEnabled(features::kColorCorrectRendering)) {
Nit: No {}'s

[ccameron, 2017-06-16 19:25:56]

Description was changed from

==========
color: Use base::Feature to control color correct rendering

BUG=713889
==========

to

==========
color: Use base::Feature to control color correct rendering

Add an explicit enable command line flag in
ShellMainDelegate::BasicStartupComplete to enable this feature
for layout tests, because it is already enabled for layout tests.

BUG=713889
==========

[ccameron, 2017-06-16 21:46:35]

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

[commit-bot: I haz the power, 2017-06-16 21:46:54]

Dry run: CQ is trying da patch.

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

[ccameron, 2017-06-16 22:08:36]

Description was changed from

==========
color: Use base::Feature to control color correct rendering

Add an explicit enable command line flag in
ShellMainDelegate::BasicStartupComplete to enable this feature
for layout tests, because it is already enabled for layout tests.

BUG=713889
==========

to

==========
color: Use base::Feature to control color correct rendering

Add an explicit enable command line flag in
ShellMainDelegate::BasicStartupComplete to enable this feature
for layout tests, because it is already enabled for layout tests.

R=asvitkine
TBR=avi (content/), xiyuan (chrome/browser/chromeos/login)
BUG=713889
==========

[ccameron, 2017-06-16 22:08:42]

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

[commit-bot: I haz the power, 2017-06-16 22:09:01]

Dry run: CQ is trying da patch.

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

[ccameron, 2017-06-16 22:15:26]

Thanks!

https://codereview.chromium.org/2945453002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2945453002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:335: if
(base::FeatureList::IsEnabled(features::kColorCorrectRendering)) {
On 2017/06/16 14:47:24, Alexei Svitkine (slow) wrote:
> Nit: No {}'s

Done.

[ccameron, 2017-06-16 22:15:37]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2017-06-16 22:15:42]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-06-16 22:15:43]

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

[commit-bot: I haz the power, 2017-06-16 22:15:48]

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-06-16 23:38:10]

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

[commit-bot: I haz the power, 2017-06-16 23:38:13]

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

[ccameron, 2017-06-16 23:53:47]

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

[commit-bot: I haz the power, 2017-06-16 23:54:11]

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-06-17 01:19:15]

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

[commit-bot: I haz the power, 2017-06-17 01:19:16]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-06-17 01:22:01]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-06-17 01:22:02]

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

[commit-bot: I haz the power, 2017-06-17 01:22: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-06-17 01:29:42]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497662521795040,
"parent_rev": "c36c650cf291de889b00938e600e2bf1a54f5772", "commit_rev":
"708f986f71d8059c05c0f05b37d8c798a877ffde"}

[commit-bot: I haz the power, 2017-06-17 01:29:58]

Description was changed from

==========
color: Use base::Feature to control color correct rendering

Add an explicit enable command line flag in
ShellMainDelegate::BasicStartupComplete to enable this feature
for layout tests, because it is already enabled for layout tests.

R=asvitkine
TBR=avi (content/), xiyuan (chrome/browser/chromeos/login)
BUG=713889
==========

to

==========
color: Use base::Feature to control color correct rendering

Add an explicit enable command line flag in
ShellMainDelegate::BasicStartupComplete to enable this feature
for layout tests, because it is already enabled for layout tests.

R=asvitkine
TBR=avi (content/), xiyuan (chrome/browser/chromeos/login)
BUG=713889

Review-Url: https://codereview.chromium.org/2945453002
Cr-Commit-Position: refs/heads/master@{#480262}
Committed:
https://chromium.googlesource.com/chromium/src/+/708f986f71d8059c05c0f05b37d8...
==========

[commit-bot: I haz the power, 2017-06-17 01:29:59]

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