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

content/shell/app/shell_main_delegate.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/shell/app/shell_main_delegate.h"
 
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/cpu.h"
 #include "base/files/file.h"
@@ skipping 195 matching lines @@
     command_line.AppendSwitchASCII(switches::kHostResolverRules,
                                    "MAP *.test 127.0.0.1");
 
     command_line.AppendSwitch(switches::kEnablePartialRaster);
 
     if (!command_line.HasSwitch(switches::kForceGpuRasterization) &&
         !command_line.HasSwitch(switches::kEnableGpuRasterization)) {
       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");
 
     // We want stable/baseline results when running layout tests.
     command_line.AppendSwitch(switches::kDisableSkiaRuntimeOpts);
 
     command_line.AppendSwitch(cc::switches::kDisallowNonExactResourceReuse);
 
     // Unless/until WebM files are added to the media layout tests, we need to
     // avoid removing MP4/H264/AAC so that layout tests can run on Android.
 #if !defined(OS_ANDROID)
     media::RemoveProprietaryMediaTypesAndCodecsForTests();
 #endif
 
     if (!BlinkTestPlatformInitialize()) {
       *exit_code = 1;
       return true;
     }
   }
 

[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
                             : new ShellContentClient);
   SetContentClient(content_client_.get());
 
   return false;
 }
 
 void ShellMainDelegate::PreSandboxStartup() {
@@ skipping 132 matching lines @@
 
   return renderer_client_.get();
 }
 
 ContentUtilityClient* ShellMainDelegate::CreateContentUtilityClient() {
   utility_client_.reset(new ShellContentUtilityClient);
   return utility_client_.get();
 }
 
 }  // namespace content