[chromecast] Add cast_shell --accept-resource-provider switch

chromecast/base/chromecast_switches.cc

Index: chromecast/base/chromecast_switches.cc
diff --git a/chromecast/base/chromecast_switches.cc b/chromecast/base/chromecast_switches.cc
index 469768cea65d3114f955b7ea50058dc7ffa7b8e9..59a7f7c07647ee9a482c0cca830b9822faad372a 100644
--- a/chromecast/base/chromecast_switches.cc
+++ b/chromecast/base/chromecast_switches.cc
@@ -2,10 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/command_line.h"

[halliwell, 2016/03/05 01:01:50]

ordering nit: put the header corresponding to the cc file first, then everything
else in a separate group

[jyw, 2016/03/08 02:09:24]

Done.

 #include "chromecast/base/chromecast_switches.h"
 
 namespace switches {
 
+namespace {
+// Value indicating whether flag from command line switch is false.
+const char kSwitchValueFalse[] = "false";
+}  // namespace
+
 // Enable the CMA media pipeline.
 const char kEnableCmaMediaPipeline[] = "enable-cma-media-pipeline";
 
@@ -32,6 +38,10 @@ const char kLastLaunchedApp[] = "last-launched-app";
 // started.
 const char kPreviousApp[] = "previous-app";
 
+// Flag indicating that a resource provider must be set up to provide cast
+// receiver with resources. Apps cannot start until provided resources.
+const char kAcceptResourceProvider[] = "accept-resource-provider";
+
 // Size of the ALSA output buffer in frames. This directly sets the latency of
 // the output device. Latency can be calculated by multiplying the sample rate
 // by the output buffer size.
@@ -56,3 +66,23 @@ const char kAlsaCheckCloseTimeout[] = "alsa-check-close-timeout";
 const char kAlsaNumOutputChannels[] = "alsa-num-output-channels";
 
 }  // namespace switches
+
+namespace base {
+
+bool GetSwitchValueBoolean(const std::string& switch_string) {
+  const base::CommandLine* command_line =
+      base::CommandLine::ForCurrentProcess();
+  return command_line->HasSwitch(switch_string) &&
+         command_line->GetSwitchValueASCII(switch_string)
+             .compare(switches::kSwitchValueFalse);

[halliwell, 2016/03/05 01:01:50]

seems like this function accepts any old value that's not "false".  In
particular, 'FALSE', 'False', '0' and various other misleading things would
evaluate true.

Should we compare to an explicit 'true' value and name it IsSwitchExplicitTrue
for symmetry with the other function?

[byungchul, 2016/03/07 17:19:40]

These functions are used to disable a feature explicitly especially for OEMs
when it is enabled by default process.json. For example, when default
process.json has --accept-resource-provider=true and OEM wants to disable it,
they just give --accept-resource-provider=false instead of creating a separate
process.json. We don't encourage OEMs to have separate process.json files
because their own process.json may have command line flags obsoleted later, or
not enable some new features.

GetSwitchValueBoolean() is used to make a flag false by default. So,
no flag -> false,
flag without value -> true,
flag with a value except "false" -> true,
flag with a value "false" -> false

while IsSwitchExplicitFalse() makes a flag true by default. So,
no flag -> true,
flag without value -> true,
flag with a value except "false" -> true,
flag with a value "false" -> false

[halliwell, 2016/03/07 23:33:55]

Thanks for explanation.  It definitely sounds good to keep these functions for
consistency in argument handling.

However, this motivation is not clear from the CL as it stands.  I would
suggest:
1) Adding some comment similar to yours here.
2) Naming the two functions in a consistent/symmetric way, to reflect the fact
that they are the same thing but with different default value.

[kmackay, 2016/03/08 00:06:11]

Or, just have one function, and pass in the default value?

[kmackay, 2016/03/08 00:06:11]

I'd rather just accept 'true' or 'false' (or no value). If a flag is specified
with some other random thing, print an error and use the default value (as if no
flag was specified at all).

[jyw, 2016/03/08 02:09:24]

Done.

+}
+
+bool IsSwitchExplicitFalse(const std::string& switch_string) {
+  const base::CommandLine* command_line =
+      base::CommandLine::ForCurrentProcess();
+  return command_line->HasSwitch(switch_string) &&
+         command_line->GetSwitchValueASCII(switch_string)
+                 .compare(switches::kSwitchValueFalse) == 0;

[halliwell, 2016/03/05 01:10:58]

Also, just remembered: GetSwitchValueASCII returns an empty string if the switch
doesn't exist.  So it's redundant to call HasSwitch.

Which also makes this function have even less value for me as a utility :)

[byungchul, 2016/03/07 17:19:40]

One benefit of these functions is consistency in command line flag pattern.

+}
+
+}  // namespace base