[Feature Switch][Media Router] Add required field trials to MR switch.

[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch that takes the
place of a singular field_trial_name. When deciding whether to enable or
disable a feature with field trials, all field trials will be queried
(though short circuiting can occur):
- If user is in "Enabled" group for all field trials, then enable
feature.
- If user is in "Disabled group for any field trials, then disable
feature.
- Otherwise, fall back to default value.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

BUG=541315
Committed: https://crrev.com/7c91fae3f88773af47a79f4a3bceaccfa1b27ea5
Cr-Commit-Position: refs/heads/master@{#374834}

[imcheng, 2016-02-09 01:01:57]

Description was changed from

==========
[Feature Switch][Media Router] Add dependent field trial to MR switch.

Add a generic dependent_field_trials to FeatureSwitch. When deciding
whether to enable a feature with field trials, all dependent field
trials (if specified), in addition to the original field trial, must
all be enabled.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

Also fix FeatureSwitch::extension_action_redesign(). The current
logic is incorrect for users with cast extension.


BUG=541315
==========

to

==========
[Feature Switch][Media Router] Add dependent field trial to MR switch.

Add a generic dependent_field_trials to FeatureSwitch. When deciding
whether to enable a feature with field trials, all dependent field
trials (if specified), in addition to the original field trial, must
all be enabled.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

Also fix FeatureSwitch::extension_action_redesign(). The current
logic is incorrect for users with cast extension. This patch changes
it to look at the MR flag value.
- If MR is enabled by flag, then the override switch is returned.
- If MR is enabled by field trial, then by definition EAR field
  trial is also enabled.
- By the time MR is enabled by default, so should EAR.


BUG=541315
==========

[imcheng, 2016-02-09 01:12:28]

Patchset #1 (id:1) has been deleted

[imcheng, 2016-02-09 01:17:49]

imcheng@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[imcheng, 2016-02-09 01:17:50]

Devlin: PTAL. As discussed I added a generic dependent_field_trials to
FeatureSwitch. I added one more constructor to the list of many. It's not ideal,
but my intention is to merge this to M49, so I didn't want to touch too many
files here.

[Devlin, 2016-02-09 18:45:14]

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
File extensions/common/feature_switch.cc (right):

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.cc:134: kMediaRouterFlag) == "1")
This doesn't account for the case of, e.g., --enable-media-router (i.e., the
"legacy" enable flag).  Why is it incorrect to just check media_router()?

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.cc:184: const char* field_trial_name,
Could we instead just always take a vector of required field trials that
includes the base field trial?  I think that works for all the logic (just
iterate over each field trial and make sure it's present) but I might be missing
an edge case.

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.cc:215: FeatureSwitch::~FeatureSwitch() =
default;
we don't really use this style anywhere else that I've seen.  Prefer the
standard empty braces.

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.cc:231: if (!default_value_ &&
command_line_->HasSwitch(GetLegacyEnableFlag()))
unrelated to this change, but these should really just be:
if (command_line_->HasSwitch())

Otherwise, we could improperly ignore an enable/disable switch if there is a
field trial active.

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
File extensions/common/feature_switch.h (right):

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.h:80: const std::vector<const char*>&
dependent_field_trials,
"dependent" is the wrong word here - it should probably be "required"

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.h:106: std::vector<const char*>
dependent_field_trials_;
this should probably just be std::vector<std::string>

[imcheng, 2016-02-09 21:32:36]

Description was changed from

==========
[Feature Switch][Media Router] Add dependent field trial to MR switch.

Add a generic dependent_field_trials to FeatureSwitch. When deciding
whether to enable a feature with field trials, all dependent field
trials (if specified), in addition to the original field trial, must
all be enabled.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

Also fix FeatureSwitch::extension_action_redesign(). The current
logic is incorrect for users with cast extension. This patch changes
it to look at the MR flag value.
- If MR is enabled by flag, then the override switch is returned.
- If MR is enabled by field trial, then by definition EAR field
  trial is also enabled.
- By the time MR is enabled by default, so should EAR.


BUG=541315
==========

to

==========
[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch. When deciding
whether to enable a feature with field trials, all dependent field
trials (if specified), in addition to the original field trial, must
all be enabled.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

Also fix FeatureSwitch::extension_action_redesign(). The current
logic is incorrect for users with cast extension. This patch changes
it to look at the MR flag value.
- If MR is enabled by flag, then the override switch is returned.
- If MR is enabled by field trial, then by definition EAR field
  trial is also enabled.
- By the time MR is enabled by default, so should EAR.


BUG=541315
==========

[imcheng, 2016-02-09 21:35:26]

Description was changed from

==========
[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch. When deciding
whether to enable a feature with field trials, all dependent field
trials (if specified), in addition to the original field trial, must
all be enabled.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

Also fix FeatureSwitch::extension_action_redesign(). The current
logic is incorrect for users with cast extension. This patch changes
it to look at the MR flag value.
- If MR is enabled by flag, then the override switch is returned.
- If MR is enabled by field trial, then by definition EAR field
  trial is also enabled.
- By the time MR is enabled by default, so should EAR.


BUG=541315
==========

to

==========
[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch that takes the
place of a singular field_trial_name. When deciding whether to enable or
disable a feature with field trials, all field trials will be queried
(though short circuiting can occur):
- If user is in "Enabled" group for all field trials, then enable
feature.
- If user is in "Disabled group for any field trials, then disable
feature.
- Otherwise, fall back to default value.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

Also fix FeatureSwitch::extension_action_redesign(). The current
logic is incorrect for users with cast extension. This patch changes
it to look at the MR flag value.
- If MR is enabled by flag, then the override switch is returned.
- If MR is enabled by field trial, then by definition EAR field
  trial is also enabled.
- By the time MR is enabled by default, so should EAR.


BUG=541315
==========

[imcheng, 2016-02-09 21:35:40]

PTAL.

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
File extensions/common/feature_switch.cc (right):

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.cc:134: kMediaRouterFlag) == "1")
On 2016/02/09 18:45:14, Devlin wrote:
> This doesn't account for the case of, e.g., --enable-media-router (i.e., the
> "legacy" enable flag).  Why is it incorrect to just check media_router()?

Checking media_router() isn't correct for users with cast extension -- we should
be checking media_router_with_cast_extension() instead. But to do so requires
providing an extra parameter to this function, which isn't good as this function
is called from many places. Checking the flag should still return the right
behavior here (i.e., MR enabled implies EAR enabled).

--enable-media-router was removed in the same release the FeatureSwitch was
added. M48 and onward use --media-router=1, and MR isn't officially supported in
M49 anyway. Do we really need to check for that?

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.cc:184: const char* field_trial_name,
On 2016/02/09 18:45:14, Devlin wrote:
> Could we instead just always take a vector of required field trials that
> includes the base field trial?  I think that works for all the logic (just
> iterate over each field trial and make sure it's present) but I might be
missing
> an edge case.

I think it depends on how you interpret the required field trials. If we don't
need to make a distinction between the base field trial and others then it's
probably fine to do that. The new logic will be:

Enable by experiment if user is in Enabled group for all experiments
Disable by experiment if user is in Disabled group for any experiments
Use default value otherwise (i.e., Control group in at least 1 experiment, with
no Disabled group)

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.cc:215: FeatureSwitch::~FeatureSwitch() =
default;
On 2016/02/09 18:45:14, Devlin wrote:
> we don't really use this style anywhere else that I've seen.  Prefer the
> standard empty braces.

Done.

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.cc:231: if (!default_value_ &&
command_line_->HasSwitch(GetLegacyEnableFlag()))
On 2016/02/09 18:45:14, Devlin wrote:
> unrelated to this change, but these should really just be:
> if (command_line_->HasSwitch())
> 
> Otherwise, we could improperly ignore an enable/disable switch if there is a
> field trial active.

Ok, I will file a bug for it.

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
File extensions/common/feature_switch.h (right):

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.h:80: const std::vector<const char*>&
dependent_field_trials,
On 2016/02/09 18:45:14, Devlin wrote:
> "dependent" is the wrong word here - it should probably be "required"

Done.

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.h:106: std::vector<const char*>
dependent_field_trials_;
On 2016/02/09 18:45:14, Devlin wrote:
> this should probably just be std::vector<std::string>

Done.

[imcheng, 2016-02-09 21:37:52]

Patchset #2 (id:40001) has been deleted

[Devlin, 2016-02-10 18:08:33]

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
File extensions/common/feature_switch.cc (right):

https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
extensions/common/feature_switch.cc:134: kMediaRouterFlag) == "1")
On 2016/02/09 21:35:39, imcheng1 wrote:
> On 2016/02/09 18:45:14, Devlin wrote:
> > This doesn't account for the case of, e.g., --enable-media-router (i.e., the
> > "legacy" enable flag).  Why is it incorrect to just check media_router()?
> 
> Checking media_router() isn't correct for users with cast extension -- we
should
> be checking media_router_with_cast_extension() instead. But to do so requires
> providing an extra parameter to this function, which isn't good as this
function
> is called from many places. Checking the flag should still return the right
> behavior here (i.e., MR enabled implies EAR enabled).
> 
> --enable-media-router was removed in the same release the FeatureSwitch was
> added. M48 and onward use --media-router=1, and MR isn't officially supported
in
> M49 anyway. Do we really need to check for that?

So we need to check media_router() for most users but
media_router_with_extension() for users with the cast extension?  Do we want to
force enable EAR for users of media_router() without the cast extension?  Or
could we just make this check media_router_with_cast_extension() all the time,
and not enable it for media_router() (if the user is only in the MR trial group,
e.g.)?

[imcheng, 2016-02-10 18:31:28]

On 2016/02/10 18:08:33, Devlin wrote:
>
https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
> File extensions/common/feature_switch.cc (right):
> 
>
https://codereview.chromium.org/1680823004/diff/20001/extensions/common/featu...
> extensions/common/feature_switch.cc:134: kMediaRouterFlag) == "1")
> On 2016/02/09 21:35:39, imcheng1 wrote:
> > On 2016/02/09 18:45:14, Devlin wrote:
> > > This doesn't account for the case of, e.g., --enable-media-router (i.e.,
the
> > > "legacy" enable flag).  Why is it incorrect to just check media_router()?
> > 
> > Checking media_router() isn't correct for users with cast extension -- we
> should
> > be checking media_router_with_cast_extension() instead. But to do so
requires
> > providing an extra parameter to this function, which isn't good as this
> function
> > is called from many places. Checking the flag should still return the right
> > behavior here (i.e., MR enabled implies EAR enabled).
> > 
> > --enable-media-router was removed in the same release the FeatureSwitch was
> > added. M48 and onward use --media-router=1, and MR isn't officially
supported
> in
> > M49 anyway. Do we really need to check for that?
> 
> So we need to check media_router() for most users but
> media_router_with_extension() for users with the cast extension?  Do we want
to
> force enable EAR for users of media_router() without the cast extension?  Or
> could we just make this check media_router_with_cast_extension() all the time,
> and not enable it for media_router() (if the user is only in the MR trial
group,
> e.g.)?

Wouldn't it be an issue for a user without no cast extension, is not in
"Enabled"
group of MediaRouterExperiment, but is in "Enabled" group of
MediaRouterWithCastExtensionExperiment? This user will probably not have MR
enabled,
but we would be forcing EAR for that user.

[Devlin, 2016-02-10 18:45:59]

On 2016/02/10 18:31:28, imcheng1 wrote:
> Wouldn't it be an issue for a user without no cast extension, is not in
> "Enabled"
> group of MediaRouterExperiment, but is in "Enabled" group of
> MediaRouterWithCastExtensionExperiment? This user will probably not have MR
> enabled,
> but we would be forcing EAR for that user.

Buuuut... we can't have that happen, right?  Because
MediaRouterWithCastExtensionExperiment is dependent on EAR as of this patch - so
media_router_with_cast_extension() can only be enabled if EAR is already on.

[imcheng, 2016-02-10 20:46:30]

On 2016/02/10 18:45:59, Devlin wrote:
> On 2016/02/10 18:31:28, imcheng1 wrote:
> > Wouldn't it be an issue for a user without no cast extension, is not in
> > "Enabled"
> > group of MediaRouterExperiment, but is in "Enabled" group of
> > MediaRouterWithCastExtensionExperiment? This user will probably not have MR
> > enabled,
> > but we would be forcing EAR for that user.
> 
> Buuuut... we can't have that happen, right?  Because
> MediaRouterWithCastExtensionExperiment is dependent on EAR as of this patch -
so
> media_router_with_cast_extension() can only be enabled if EAR is already on.

Yeah but the problem is that the logic to select the
extension_action_redesign_override FeatureSwitch is incorrect, because in the
above scenario, media_router_with_cast_extension()->IsEnabled() returns true for
that user, but in fact they do not actually have Media Router enabled (i.e.,
media_router()->Enabled() returns false because they are not in
MediaRouterExperiment).

[imcheng, 2016-02-10 20:49:39]

On 2016/02/10 20:46:30, imcheng1 wrote:
> On 2016/02/10 18:45:59, Devlin wrote:
> > On 2016/02/10 18:31:28, imcheng1 wrote:
> > > Wouldn't it be an issue for a user without no cast extension, is not in
> > > "Enabled"
> > > group of MediaRouterExperiment, but is in "Enabled" group of
> > > MediaRouterWithCastExtensionExperiment? This user will probably not have
MR
> > > enabled,
> > > but we would be forcing EAR for that user.
> > 
> > Buuuut... we can't have that happen, right?  Because
> > MediaRouterWithCastExtensionExperiment is dependent on EAR as of this patch
-
> so
> > media_router_with_cast_extension() can only be enabled if EAR is already on.
> 
> Yeah but the problem is that the logic to select the
> extension_action_redesign_override FeatureSwitch is incorrect, because in the
> above scenario, media_router_with_cast_extension()->IsEnabled() returns true
for
> that user, but in fact they do not actually have Media Router enabled (i.e.,
> media_router()->Enabled() returns false because they are not in
> MediaRouterExperiment).

Ok, I think I get the (unfortunately confusing) logic now. If
media_router_with_cast_extension()->IsEnabled() returns true, it implies EAR
experiment is already enabled. So, no harm in returning the
extension_action_redesignoverride FeatureSwitch. Is my understanding correct?

[Devlin, 2016-02-10 20:49:54]

On 2016/02/10 20:46:30, imcheng1 wrote:
> Yeah but the problem is that the logic to select the
> extension_action_redesign_override FeatureSwitch is incorrect, because in the
> above scenario, media_router_with_cast_extension()->IsEnabled() returns true
for
> that user, but in fact they do not actually have Media Router enabled (i.e.,
> media_router()->Enabled() returns false because they are not in
> MediaRouterExperiment).

I'm still not very familiar with the MR experiments.  Why would the user be in
MR w/ extension if they're not in some MR experiment?

All that said, the logic isn't necessarily wrong, because the only way they'd be
in *either* experiment group is if EAR is already enabled, right?  Except with
using command line flags, and there's only the one to worry about.

[Devlin, 2016-02-10 20:50:38]

On 2016/02/10 20:49:39, imcheng1 wrote:
> On 2016/02/10 20:46:30, imcheng1 wrote:
> > On 2016/02/10 18:45:59, Devlin wrote:
> > > On 2016/02/10 18:31:28, imcheng1 wrote:
> > > > Wouldn't it be an issue for a user without no cast extension, is not in
> > > > "Enabled"
> > > > group of MediaRouterExperiment, but is in "Enabled" group of
> > > > MediaRouterWithCastExtensionExperiment? This user will probably not have
> MR
> > > > enabled,
> > > > but we would be forcing EAR for that user.
> > > 
> > > Buuuut... we can't have that happen, right?  Because
> > > MediaRouterWithCastExtensionExperiment is dependent on EAR as of this
patch
> -
> > so
> > > media_router_with_cast_extension() can only be enabled if EAR is already
on.
> > 
> > Yeah but the problem is that the logic to select the
> > extension_action_redesign_override FeatureSwitch is incorrect, because in
the
> > above scenario, media_router_with_cast_extension()->IsEnabled() returns true
> for
> > that user, but in fact they do not actually have Media Router enabled (i.e.,
> > media_router()->Enabled() returns false because they are not in
> > MediaRouterExperiment).
> 
> Ok, I think I get the (unfortunately confusing) logic now. If
> media_router_with_cast_extension()->IsEnabled() returns true, it implies EAR
> experiment is already enabled. So, no harm in returning the
> extension_action_redesignoverride FeatureSwitch. Is my understanding correct?

Yes. :)  A comment summing that up would probably be worthwhile.

[imcheng, 2016-02-10 21:25:39]

On 2016/02/10 20:50:38, Devlin wrote:
> On 2016/02/10 20:49:39, imcheng1 wrote:
> > On 2016/02/10 20:46:30, imcheng1 wrote:
> > > On 2016/02/10 18:45:59, Devlin wrote:
> > > > On 2016/02/10 18:31:28, imcheng1 wrote:
> > > > > Wouldn't it be an issue for a user without no cast extension, is not
in
> > > > > "Enabled"
> > > > > group of MediaRouterExperiment, but is in "Enabled" group of
> > > > > MediaRouterWithCastExtensionExperiment? This user will probably not
have
> > MR
> > > > > enabled,
> > > > > but we would be forcing EAR for that user.
> > > > 
> > > > Buuuut... we can't have that happen, right?  Because
> > > > MediaRouterWithCastExtensionExperiment is dependent on EAR as of this
> patch
> > -
> > > so
> > > > media_router_with_cast_extension() can only be enabled if EAR is already
> on.
> > > 
> > > Yeah but the problem is that the logic to select the
> > > extension_action_redesign_override FeatureSwitch is incorrect, because in
> the
> > > above scenario, media_router_with_cast_extension()->IsEnabled() returns
true
> > for
> > > that user, but in fact they do not actually have Media Router enabled
(i.e.,
> > > media_router()->Enabled() returns false because they are not in
> > > MediaRouterExperiment).
> > 
> > Ok, I think I get the (unfortunately confusing) logic now. If
> > media_router_with_cast_extension()->IsEnabled() returns true, it implies EAR
> > experiment is already enabled. So, no harm in returning the
> > extension_action_redesignoverride FeatureSwitch. Is my understanding
correct?
> 
> Yes. :)  A comment summing that up would probably be worthwhile.

Cool. In that case it should be fine to check media_router()->IsEnabled(). I
reverted it to original behavior and added comments. PTAL.

[imcheng, 2016-02-10 21:34:08]

On 2016/02/10 20:49:54, Devlin wrote:
> On 2016/02/10 20:46:30, imcheng1 wrote:
> > Yeah but the problem is that the logic to select the
> > extension_action_redesign_override FeatureSwitch is incorrect, because in
the
> > above scenario, media_router_with_cast_extension()->IsEnabled() returns true
> for
> > that user, but in fact they do not actually have Media Router enabled (i.e.,
> > media_router()->Enabled() returns false because they are not in
> > MediaRouterExperiment).
> 
> I'm still not very familiar with the MR experiments.  Why would the user be in
> MR w/ extension if they're not in some MR experiment?
> 
> All that said, the logic isn't necessarily wrong, because the only way they'd
be
> in *either* experiment group is if EAR is already enabled, right?  Except with
> using command line flags, and there's only the one to worry about.

Sorry I missed this comment. Because the experiment framework does not support
separating users by whether they have installed a particular extension. So we
created a separate experiment MediaRouterWithCastExtensionExperiment, but we
actually assign groups for both experiments on users, and then only selecting
which experiment to query in Chrome for FeatureSwitch. So a user without cast
extension still has a group assigned in MediaRouterWithCastExtensionExperiment,
that's how media_router_with_cast_extension()->IsEnabled() can return true for
that user.

[imcheng, 2016-02-10 21:35:05]

Description was changed from

==========
[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch that takes the
place of a singular field_trial_name. When deciding whether to enable or
disable a feature with field trials, all field trials will be queried
(though short circuiting can occur):
- If user is in "Enabled" group for all field trials, then enable
feature.
- If user is in "Disabled group for any field trials, then disable
feature.
- Otherwise, fall back to default value.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

Also fix FeatureSwitch::extension_action_redesign(). The current
logic is incorrect for users with cast extension. This patch changes
it to look at the MR flag value.
- If MR is enabled by flag, then the override switch is returned.
- If MR is enabled by field trial, then by definition EAR field
  trial is also enabled.
- By the time MR is enabled by default, so should EAR.


BUG=541315
==========

to

==========
[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch that takes the
place of a singular field_trial_name. When deciding whether to enable or
disable a feature with field trials, all field trials will be queried
(though short circuiting can occur):
- If user is in "Enabled" group for all field trials, then enable
feature.
- If user is in "Disabled group for any field trials, then disable
feature.
- Otherwise, fall back to default value.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

BUG=541315
==========

[Devlin, 2016-02-10 21:52:06]

lgtm

https://codereview.chromium.org/1680823004/diff/120001/extensions/common/feat...
File extensions/common/feature_switch.cc (right):

https://codereview.chromium.org/1680823004/diff/120001/extensions/common/feat...
extensions/common/feature_switch.cc:241: if (group_name == "Disabled") {
nit: Might as well nest this inside the above if.

[imcheng, 2016-02-10 21:57:42]

Thanks!

https://codereview.chromium.org/1680823004/diff/120001/extensions/common/feat...
File extensions/common/feature_switch.cc (right):

https://codereview.chromium.org/1680823004/diff/120001/extensions/common/feat...
extensions/common/feature_switch.cc:241: if (group_name == "Disabled") {
On 2016/02/10 21:52:06, Devlin wrote:
> nit: Might as well nest this inside the above if.

Done.

[imcheng, 2016-02-10 22:31:58]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2016-02-10 22:31:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1680823004/#ps140001
(title: "Addressed comment + fix comple")

[commit-bot: I haz the power, 2016-02-10 22:34:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1680823004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1680823004/140001

[commit-bot: I haz the power, 2016-02-10 23:59:32]

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

[commit-bot: I haz the power, 2016-02-10 23:59:32]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[imcheng (use chromium acct), 2016-02-11 00:52:05]

The CQ bit was checked by imcheng@google.com

[commit-bot: I haz the power, 2016-02-11 00:54:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1680823004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1680823004/140001

[commit-bot: I haz the power, 2016-02-11 02:07:05]

Description was changed from

==========
[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch that takes the
place of a singular field_trial_name. When deciding whether to enable or
disable a feature with field trials, all field trials will be queried
(though short circuiting can occur):
- If user is in "Enabled" group for all field trials, then enable
feature.
- If user is in "Disabled group for any field trials, then disable
feature.
- Otherwise, fall back to default value.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

BUG=541315
==========

to

==========
[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch that takes the
place of a singular field_trial_name. When deciding whether to enable or
disable a feature with field trials, all field trials will be queried
(though short circuiting can occur):
- If user is in "Enabled" group for all field trials, then enable
feature.
- If user is in "Disabled group for any field trials, then disable
feature.
- Otherwise, fall back to default value.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

BUG=541315
==========

[commit-bot: I haz the power, 2016-02-11 02:07:06]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-02-16 22:33:59]

Description was changed from

==========
[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch that takes the
place of a singular field_trial_name. When deciding whether to enable or
disable a feature with field trials, all field trials will be queried
(though short circuiting can occur):
- If user is in "Enabled" group for all field trials, then enable
feature.
- If user is in "Disabled group for any field trials, then disable
feature.
- Otherwise, fall back to default value.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

BUG=541315
==========

to

==========
[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch that takes the
place of a singular field_trial_name. When deciding whether to enable or
disable a feature with field trials, all field trials will be queried
(though short circuiting can occur):
- If user is in "Enabled" group for all field trials, then enable
feature.
- If user is in "Disabled group for any field trials, then disable
feature.
- Otherwise, fall back to default value.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

BUG=541315

Committed: https://crrev.com/7c91fae3f88773af47a79f4a3bceaccfa1b27ea5
Cr-Commit-Position: refs/heads/master@{#374834}
==========

[commit-bot: I haz the power, 2016-02-16 22:34:00]

Patchset 6 (id:??) landed as
https://crrev.com/7c91fae3f88773af47a79f4a3bceaccfa1b27ea5
Cr-Commit-Position: refs/heads/master@{#374834}