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

content/shell/app/shell_main_delegate.cc

Index: content/shell/app/shell_main_delegate.cc
diff --git a/content/shell/app/shell_main_delegate.cc b/content/shell/app/shell_main_delegate.cc
index f496af7313ade6096dea3592b6f5ee883fdc686d..e3a3954958665e530e27154c16cfe1bbbf215b90 100644
--- a/content/shell/app/shell_main_delegate.cc
+++ b/content/shell/app/shell_main_delegate.cc
@@ -213,9 +213,7 @@ bool ShellMainDelegate::BasicStartupComplete(int* exit_code) {
       command_line.AppendSwitch(switches::kDisableGpuRasterization);
     }
 
-    // Enable color correct rendering. If the virtual test suite didn't specify
-    // a color space, then use sRGB.
-    command_line.AppendSwitch(switches::kEnableColorCorrectRendering);
+    // If the virtual test suite didn't specify a color space, then force sRGB.
     if (!command_line.HasSwitch(switches::kForceColorProfile))
       command_line.AppendSwitchASCII(switches::kForceColorProfile, "srgb");
 
@@ -236,6 +234,13 @@ bool ShellMainDelegate::BasicStartupComplete(int* exit_code) {
     }
   }
 

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

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.

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

(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?

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

(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:
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).

+  // Enable additional base::Features. Note that there already may exist a list
+  // of enabled features from the virtual or physical test suite.
+  std::string enabled_features =
+      command_line.GetSwitchValueASCII(switches::kEnableFeatures);
+  enabled_features = "ColorCorrectRendering," + enabled_features;
+  command_line.AppendSwitchASCII(switches::kEnableFeatures, enabled_features);
+
   content_client_.reset(base::CommandLine::ForCurrentProcess()->HasSwitch(
                             switches::kRunLayoutTest)
                             ? new LayoutTestContentClient