Fix losing of scoped feature channel in tests when it's equal to stable

extensions/common/features/feature_channel.cc

Index: extensions/common/features/feature_channel.cc
diff --git a/extensions/common/features/feature_channel.cc b/extensions/common/features/feature_channel.cc
index 13bd6e1dcaee847aaccae4e4510fac40a71aea5f..2cb3d767709b1f54385aa7a688febfe069753c8b 100644
--- a/extensions/common/features/feature_channel.cc
+++ b/extensions/common/features/feature_channel.cc
@@ -8,8 +8,9 @@
 
 namespace {
 
-const version_info::Channel kDefaultChannel = version_info::Channel::STABLE;
-version_info::Channel g_current_channel = kDefaultChannel;
+version_info::Channel g_current_channel = version_info::Channel::STABLE;
+// The number of currently existing instances of |ScopedCurrentChannel|.
+int g_current_channel_override_count = 0;
 
 }  // namespace
 
@@ -20,21 +21,19 @@ version_info::Channel GetCurrentChannel() {
 }
 
 void SetCurrentChannel(version_info::Channel channel) {
-  g_current_channel = channel;
-}
-
-version_info::Channel GetDefaultChannel() {
-  return kDefaultChannel;
+  if (!g_current_channel_override_count)

[Devlin, 2017/05/15 20:34:47]

We'd have this issue already, but I wonder if there's any concern around the
flow:

ScopedCurrentChannel(BETA);
<InitializeSystem>
  SetCurrentChannel(CANARY);   // Ignores; doesn't set.
~ScopedCurrentChannel();

The end result is that the current channel isn't anything we set it to.

The way around this would be, conceptually, to have a
g_current_channel;
optional g_overridden_channel;

GetCurrentChannel() {
  return g_overridden_channel ? *g_overridden_channel : g_current_channel;
}

But since optionals are non-POD, there's actually a bit more complexity in
there.  We could solve it with a lazy instance, or a bool did_set, or a counter
like you have here.

I'm not sure if it's worth addressing that use case, since really we don't
*expect* overrides to be appearing and disappearing throughout tests (and if
they do, it's probably a bug, since that means some systems would be initialized
with certain assumptions, but not others), so I don't know how far we want to
take that... just figured I'd bring it up.  WDYT?

+    g_current_channel = channel;
 }
 
 ScopedCurrentChannel::ScopedCurrentChannel(version_info::Channel channel)
-    : original_channel_(version_info::Channel::UNKNOWN) {
-  original_channel_ = GetCurrentChannel();
-  SetCurrentChannel(channel);
+    : original_channel_(g_current_channel) {
+  g_current_channel = channel;
+  ++g_current_channel_override_count;
 }
 
 ScopedCurrentChannel::~ScopedCurrentChannel() {
-  SetCurrentChannel(original_channel_);
+  --g_current_channel_override_count;
+  g_current_channel = original_channel_;

[lazyboy, 2017/05/03 19:01:14]

Q: Two ScoppedCurrentChannel-s only nest, but cannot overlap? Otherwise this
will fail:
ScopeA
ScopeB
~ScopeA
~ScopeB

[emaxx, 2017/05/07 01:54:03]

Yes, non-nesting overlapping won't work. I don't think it will be easy to
support that (also it'll be arguably a weird semantics anyway). Do you think
it's worth documenting in the class comment that this case is forbidden?

 }
 
 }  // namespace extensions