Issue 1306653004: Expand FeatureList to support FieldTrial association.

Alexei Svitkine (slow)

Patchset #2 (id:20001) has been deleted

5 years, 3 months ago (2015-09-08 21:20:14 UTC) #1

Alexei Svitkine (slow)

asvitkine@chromium.org changed reviewers: + isherman@chromium.org, rkaplow@chromium.org

5 years, 3 months ago (2015-09-11 20:27:05 UTC) #9

rkaplow

https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.cc#newcode61 base/feature_list.cc:61: // |for_forced_state|. overriden https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.h File base/feature_list.h (right): https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.h#newcode94 base/feature_list.h:94: ...

5 years, 3 months ago (2015-09-14 21:20:12 UTC) #11

Alexei Svitkine (slow)

Thanks Rob, PTAL! https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.cc#newcode61 base/feature_list.cc:61: // |for_forced_state|. On 2015/09/14 21:20:12, rkaplow ...

5 years, 3 months ago (2015-09-14 22:03:44 UTC) #13

rkaplow

lgtm https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.h File base/feature_list.h (right): https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.h#newcode94 base/feature_list.h:94: // Registers a field trial to override the ...

5 years, 3 months ago (2015-09-15 17:34:08 UTC) #14

Alexei Svitkine (slow)

Thanks - let's see what Ilya thinks about the above. :) On Tue, Sep 15, ...

5 years, 3 months ago (2015-09-15 17:35:39 UTC) #15

Thanks - let's see what Ilya thinks about the above. :)

On Tue, Sep 15, 2015 at 1:34 PM, <rkaplow@chromium.org> wrote:

> lgtm
>
>
>
>
> https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.h
> File base/feature_list.h (right):
>
>
>
https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.h#ne...
> base/feature_list.h:94: // Registers a field trial to override the
> enabled state of the specified
> On 2015/09/14 22:03:43, Alexei Svitkine wrote:
>
>> On 2015/09/14 21:20:12, rkaplow wrote:
>> > shouldn't this just be override the state of the specificed
>>
> feature... why do
>
>> > you say 'enabled'?
>>
>
> Hmm, I said "enabled state" instead of "state" to clarify what "state"
>>
> refers
>
>> to. (Since perhaps a feature can have other states - e.g. one state is
>>
> whether
>
>> it has an associated trial. So "enabled state" means whether the
>>
> feature is
>
>> enabled or not.
>>
>
> But seems like this wording is confusing (at least, it was confusing
>>
> to you).
>
>> Can you think of better wording?
>>
>
> Could have just been me on that read. Probably fine.
>
>
>
https://codereview.chromium.org/1306653004/diff/170001/base/feature_list.h#ne...
> base/feature_list.h:115: OverrideState for_overridden_state,
> On 2015/09/14 22:03:43, Alexei Svitkine wrote:
>
>> On 2015/09/14 21:20:12, rkaplow wrote:
>> > seems a bit funny to call it for_overriden_state but not a big deal
>>
>
> I guess I wanted to make it clear that the association is with a given
>> overridden state. I could just call it |overridden_state|, but seems
>>
> it's not as
>
>> obvious then. Let me know what you think.
>>
>
> Hm, I'm still not mentally parsing how I'm supposed to be reading
> 'for_overriden_state' - still seems funny. Anyway not a big deal, up to
> you.
>
> https://codereview.chromium.org/1306653004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Ilya Sherman

Maybe my brain is just foggy from low sleep today, but I'm having a pretty ...

5 years, 3 months ago (2015-09-16 00:50:44 UTC) #16

Ilya Sherman

One thing that I think would help increase clarity would be to reduce the number ...

5 years, 3 months ago (2015-09-16 01:18:43 UTC) #17

Alexei Svitkine (slow)

On 2015/09/16 01:18:43, Ilya Sherman wrote: > One thing that I think would help increase ...

5 years, 3 months ago (2015-09-16 20:30:28 UTC) #18

On 2015/09/16 01:18:43, Ilya Sherman wrote:
> One thing that I think would help increase clarity would be to reduce the
number
> of repeated fields that we're adding.  Notably, a single study can only be
> associated with a single feature being forced on or off via the command-line. 
> Otherwise, if both forcing flags are present on the command-line, it is not
> obvious what group would be activated.  So, it probably makes sense for the
> fields describing forcing flags to be top-level in the study config, rather
than
> associated with individual experiments.
> 
> It's also not super clear to me how to reason about the possibility that a
> single study controls multiple features.  Suppose that those features are
forced
> via the command line.  What do we report for the study?  A 1:1 mapping between
> studies and features would make the whole system much easier to reason about. 
> What do we lose if we restrict things thusly?

I agree that a 1-1 mapping is easier to reason about, but there's less
flexibility.

I can think of the following scenarios that we wouldn't be able to support:
  - Where a single study config is used for multiple experiments to ensure
they're not overlapping. For example, Lightspeed perf experiments on canary or
the Omnibox search experiments. In those cases, different non-overlapping groups
could affect different features. And since we have real examples of such
studies, it's not just a hypothetical scenario.
  - Where a kill-switch mechanism needs to disable several things in the same
config. e.g. both feature A and B are using the same helper class that has a bug
and is DDOS'ing servers. We'd like to disable them both. It's easier to do this
in a single study config that in separate ones. Especially if you want to do a
ramp-up of the kill to be safe (e.g. disable both for 10% of users first).

I think both of the above scenarios are realistic and I'd like to ensure the
protocol is flexible enough to support them.

Alexei Svitkine (slow)

Some replies to your other comments. Haven't made any changes yet (will probably have to ...

5 years, 3 months ago (2015-09-16 20:54:08 UTC) #19

Some replies to your other comments. Haven't made any changes yet (will probably
have to wait till next week since I'm pretty busy with perf this week).

A number of the edge cases you mention are things that we don't expect the
server to send and we'll have server-side checks for (when that part of the
system is implemented). Not sure it's worth having separate checks for them here
- e.g. same reason we don't check what happens if all the string fields in the
proto have random non-printable binary data.

Sure, the protocol allows it, but our server won't send it - and we won't
process random corrupted data since the payload is signed.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:69: if (it->second.field_trial) {
On 2015/09/16 00:50:43, Ilya Sherman wrote:
> Maybe it's ok if the already set field trial matches the new one?

Is there a specific scenario you're imagining? In my mind, I can't think of a
case where we would want that.

i.e.
There shouldn't be two server-side configs trying to affect the same feature
(and this will be validated server-side in the analyzer per the design) for the
same population of users.
And within a single config, association with command-line flags takes precedence
over forcing the state via the experiment. (i.e. VariationsService first calls
all the Associate* functions and does an early return when one succeeds, before
calling Register* functions for that study).

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:74: FieldTrialList::CreateFieldTrial(field_trial_name,
group_name);
On 2015/09/16 00:50:43, Ilya Sherman wrote:
> Hmm, why create the field trial?  What guarantees that it doesn't already
exist?
>  That is, why doesn't this method accept a FieldTrial* param, same as the
other
> method?

The field trial should only be created through this mechanism if it's been
forced through the command-line. This is similar logic to "forcing_flag" logic
already in VariationsService.

The difference for why the trial is created here vs. for "forcing_flag" where
it's created in VariationsService is because VariationsService can test directly
if a flag was forced, but can't (without adding an API) do so for feature
forcing.

If we wanted the trial to be created in VariationsService, then we'd need to add
an API on FeatureList to tell if a trial was overridden by the command-line
(either one or two functions depending if you want to combine enable/disable
into a more complicated function or have separate one). Then, when you decide to
create the trial, you still need to associate it with the entry - so a separate
Associate() call is still needed.

Per the above, I believe the approach implemented in this CL is actually
simpler, because it encompasses the complexity in the single call, instead of
splitting this logic between here and VariationService and exposing more API
surface.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/variations_seed_processor_unittest.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor_unittest.cc:663: }
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> It would be nice to add tests for what happens in corner cases such as:
> 
>   * Two studies both try to activate the same feature.

This is meant to be enforced server-side. I guess I can add a test to document
what the client does in that case, but the intention is for this to never happen
through server-side enforcement.

>   * Two experiments within a study both try to activate the same feature.
There's no problem there. Only the chosen experiment should have effect. But if
they're both doing the same thing, then I'm not sure what this will be testing
for? That having a second experiment with the same config doesn't make the
chosen experiment do nothing for some reason?

>   * One study tries to activate a feature, while another study tries to
> deactivate it.
Again, should be enforced server-side.

>   * Two studies both try to associate with a single feature when it's turned
on.
Ditto.

>   * Two experiments within a study both try to associate with a single feature
> when it's turned on.
Again, server-side validated. Same thing as two forcing_flags for the same flag.

>   * Two studies try to associate with a single feature in opposite on/off
> states.
Again, similar to two studies trying to associate to same feature, not allowed
via server-side checking.

>   * Two experiments within a study try to associate with a single feature in
> opposite on/off states.
Since this is an expected use case, seems reasonable to add.

>   * An experiment tries to simultaneously associate with a feature both in its
> on and off state.
Again, shouldn't be allowed via server-side checks.

>   * An experiment tries to simultaneously associate with a feature in its on
> state, and activate that feature.
Seems reasonable to test, but note that the experiment is associating with the
"feature being turned on from command-line", not "in its on state", but worth
testing.

>   * An experiment tries to simultaneously associate with a feature in its on
> state, and deactivate that feature.
Ditto.

Ilya Sherman

Thanks, Alexei. On 2015/09/16 20:30:28, Alexei Svitkine (slow) wrote: > On 2015/09/16 01:18:43, Ilya Sherman ...

5 years, 3 months ago (2015-09-17 01:02:46 UTC) #20

Thanks, Alexei.

On 2015/09/16 20:30:28, Alexei Svitkine (slow) wrote:
> On 2015/09/16 01:18:43, Ilya Sherman wrote:
> > One thing that I think would help increase clarity would be to reduce the
> number
> > of repeated fields that we're adding.  Notably, a single study can only be
> > associated with a single feature being forced on or off via the
command-line. 
> > Otherwise, if both forcing flags are present on the command-line, it is not
> > obvious what group would be activated.  So, it probably makes sense for the
> > fields describing forcing flags to be top-level in the study config, rather
> than
> > associated with individual experiments.
> > 
> > It's also not super clear to me how to reason about the possibility that a
> > single study controls multiple features.  Suppose that those features are
> forced
> > via the command line.  What do we report for the study?  A 1:1 mapping
between
> > studies and features would make the whole system much easier to reason
about. 
> > What do we lose if we restrict things thusly?
> 
> I agree that a 1-1 mapping is easier to reason about, but there's less
> flexibility.
> 
> I can think of the following scenarios that we wouldn't be able to support:
>   - Where a single study config is used for multiple experiments to ensure
> they're not overlapping. For example, Lightspeed perf experiments on canary or
> the Omnibox search experiments. In those cases, different non-overlapping
groups
> could affect different features. And since we have real examples of such
> studies, it's not just a hypothetical scenario.

This use case is pretty compelling to me.  It also sounds like something we
could still support with a single feature per experiment, which is still easier
to reason about than multiple features per experiment.

>   - Where a kill-switch mechanism needs to disable several things in the same
> config. e.g. both feature A and B are using the same helper class that has a
bug
> and is DDOS'ing servers. We'd like to disable them both. It's easier to do
this
> in a single study config that in separate ones. Especially if you want to do a
> ramp-up of the kill to be safe (e.g. disable both for 10% of users first).

This sort of configuration seems unlikely enough to me that having separate
configs for each feature doesn't sound like an unreasonable tradeoff for
increased simplicity in the common case.

> I think both of the above scenarios are realistic and I'd like to ensure the
> protocol is flexible enough to support them.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:69: if (it->second.field_trial) {
On 2015/09/16 20:54:08, Alexei Svitkine (slow) wrote:
> On 2015/09/16 00:50:43, Ilya Sherman wrote:
> > Maybe it's ok if the already set field trial matches the new one?
> 
> Is there a specific scenario you're imagining? In my mind, I can't think of a
> case where we would want that.
> 
> i.e.
> There shouldn't be two server-side configs trying to affect the same feature
> (and this will be validated server-side in the analyzer per the design) for
the
> same population of users.
> And within a single config, association with command-line flags takes
precedence
> over forcing the state via the experiment. (i.e. VariationsService first calls
> all the Associate* functions and does an early return when one succeeds,
before
> calling Register* functions for that study).

I think you're right.  I was still trying to wrap my head around the interaction
of AssociateReportingFieldTrial() and RegisterFieldTrialOverride().

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:74: FieldTrialList::CreateFieldTrial(field_trial_name,
group_name);
On 2015/09/16 20:54:08, Alexei Svitkine (slow) wrote:
> On 2015/09/16 00:50:43, Ilya Sherman wrote:
> > Hmm, why create the field trial?  What guarantees that it doesn't already
> exist?
> >  That is, why doesn't this method accept a FieldTrial* param, same as the
> other
> > method?
> 
> The field trial should only be created through this mechanism if it's been
> forced through the command-line. This is similar logic to "forcing_flag" logic
> already in VariationsService.
> 
> The difference for why the trial is created here vs. for "forcing_flag" where
> it's created in VariationsService is because VariationsService can test
directly
> if a flag was forced, but can't (without adding an API) do so for feature
> forcing.
> 
> If we wanted the trial to be created in VariationsService, then we'd need to
add
> an API on FeatureList to tell if a trial was overridden by the command-line
> (either one or two functions depending if you want to combine enable/disable
> into a more complicated function or have separate one). Then, when you decide
to
> create the trial, you still need to associate it with the entry - so a
separate
> Associate() call is still needed.
> 
> Per the above, I believe the approach implemented in this CL is actually
> simpler, because it encompasses the complexity in the single call, instead of
> splitting this logic between here and VariationService and exposing more API
> surface.

I agree that it's simpler in a way, but it also seems to tightly couple the
VariationsService implementation with this one.  I had a lot of trouble
understanding the FeatureList API until I read the VariationsService code.  But,
I'm also not thinking of a great way to simplify this without introducing the
split API like you described.  Maybe the code would be clearer if the field
trial creation were delegated to a callback passed in by the caller, but maybe
that would actually just be even more confusing... :/

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/variations_seed_processor_unittest.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor_unittest.cc:663: }
On 2015/09/16 20:54:08, Alexei Svitkine (slow) wrote:
> On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > It would be nice to add tests for what happens in corner cases such as:
> > 
> >   * Two studies both try to activate the same feature.
> 
> This is meant to be enforced server-side. I guess I can add a test to document
> what the client does in that case, but the intention is for this to never
happen
> through server-side enforcement.

I guess for things that are strictly enforced server-side, it might be useful to
just include a comment at the bottom of this file listing those things as not
needing testing.  Would be even better to provide test coverage just for
completeness, but I realize that it's kind of a pain, and has little practical
value if we really do strictly enforce these checks server-side.

> >   * Two experiments within a study both try to activate the same feature.
> There's no problem there. Only the chosen experiment should have effect. But
if
> they're both doing the same thing, then I'm not sure what this will be testing
> for? That having a second experiment with the same config doesn't make the
> chosen experiment do nothing for some reason?

I think you're right that this case is fine.

> >   * One study tries to activate a feature, while another study tries to
> > deactivate it.
> Again, should be enforced server-side.
> 
> >   * Two studies both try to associate with a single feature when it's turned
> on.
> Ditto.
> 
> >   * Two experiments within a study both try to associate with a single
feature
> > when it's turned on.
> Again, server-side validated. Same thing as two forcing_flags for the same
flag.
> 
> >   * Two studies try to associate with a single feature in opposite on/off
> > states.
> Again, similar to two studies trying to associate to same feature, not allowed
> via server-side checking.
> 
> >   * Two experiments within a study try to associate with a single feature in
> > opposite on/off states.
> Since this is an expected use case, seems reasonable to add.
> 
> >   * An experiment tries to simultaneously associate with a feature both in
its
> > on and off state.
> Again, shouldn't be allowed via server-side checks.
> 
> >   * An experiment tries to simultaneously associate with a feature in its on
> > state, and activate that feature.
> Seems reasonable to test, but note that the experiment is associating with the
> "feature being turned on from command-line", not "in its on state", but worth
> testing.

Yep, I understood that.  Is this something that we want to support, or protect
against on the server-side?  It doesn't make sense to me for an experiment to be
set up this way.

> >   * An experiment tries to simultaneously associate with a feature in its on
> > state, and deactivate that feature.
> Ditto.
> 
>

Alexei Svitkine (slow)

Thanks! I've addressed some of Ilya's comments - but haven't gotten to some of the ...

5 years, 3 months ago (2015-09-22 21:20:00 UTC) #21

Thanks! I've addressed some of Ilya's comments - but haven't gotten to some of
the others (e.g. additional tests). I've left the unresolved ones un-answered so
I can address them in the next patch set.

Wanted to upload this patchset early, in case you have feedback on any of those
changes. Feel free to take a look. Otherwise, I'll be working on addressing the
remaining comments.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:70: NOTREACHED();
On 2015/09/16 00:50:43, Ilya Sherman wrote:
> nit: Probably helpful to print out both field trial names, or at least the
> feature name.

Done.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:74: FieldTrialList::CreateFieldTrial(field_trial_name,
group_name);
On 2015/09/17 01:02:46, Ilya Sherman wrote:
> On 2015/09/16 20:54:08, Alexei Svitkine (slow) wrote:
> > On 2015/09/16 00:50:43, Ilya Sherman wrote:
> > > Hmm, why create the field trial?  What guarantees that it doesn't already
> > exist?
> > >  That is, why doesn't this method accept a FieldTrial* param, same as the
> > other
> > > method?
> > 
> > The field trial should only be created through this mechanism if it's been
> > forced through the command-line. This is similar logic to "forcing_flag"
logic
> > already in VariationsService.
> > 
> > The difference for why the trial is created here vs. for "forcing_flag"
where
> > it's created in VariationsService is because VariationsService can test
> directly
> > if a flag was forced, but can't (without adding an API) do so for feature
> > forcing.
> > 
> > If we wanted the trial to be created in VariationsService, then we'd need to
> add
> > an API on FeatureList to tell if a trial was overridden by the command-line
> > (either one or two functions depending if you want to combine enable/disable
> > into a more complicated function or have separate one). Then, when you
decide
> to
> > create the trial, you still need to associate it with the entry - so a
> separate
> > Associate() call is still needed.
> > 
> > Per the above, I believe the approach implemented in this CL is actually
> > simpler, because it encompasses the complexity in the single call, instead
of
> > splitting this logic between here and VariationService and exposing more API
> > surface.
> 
> I agree that it's simpler in a way, but it also seems to tightly couple the
> VariationsService implementation with this one.  I had a lot of trouble
> understanding the FeatureList API until I read the VariationsService code. 
But,
> I'm also not thinking of a great way to simplify this without introducing the
> split API like you described.  Maybe the code would be clearer if the field
> trial creation were delegated to a callback passed in by the caller, but maybe
> that would actually just be even more confusing... :/

Yeah, I feel like the callback would be even harder to follow. The good thing
about this API is it's not one that actually users of FeatureList API will need
to use - it's only for variations service, so even if it's hard to follow, not a
lot of people need to follow it.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:131: make_pair(feature_name,
OverrideEntry(overridden_state, field_trial)));
On 2015/09/16 00:50:43, Ilya Sherman wrote:
> It's pretty subtle that this does not overwrite in the case that the map key
> already exists.  Probably worth documenting in a quick comment.

Done.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.h
File base/feature_list.h (right):

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.h#ne...
base/feature_list.h:171: // state of the feature is queried for the first time.
On 2015/09/16 00:50:43, Ilya Sherman wrote:
> Please document ownership and lifetime expectations.

Done.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.h#ne...
base/feature_list.h:174: // TODO(asvitkine): Expand this as more support is
added.
On 2015/09/16 00:50:43, Ilya Sherman wrote:
> nit: Ok to remove this?  You removed a similar comment in the .cc file.

There's still the kill-switch semantics that are planned to be added. I've
restored the comment in the .cc file.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list_unit...
File base/feature_list_unittest.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list_unit...
base/feature_list_unittest.cc:137: for (size_t i = 0; i < arraysize(test_cases);
i++) {
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> nit: Add a SCOPED_TRACE or equivalent somewhere?

Done.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list_unit...
base/feature_list_unittest.cc:137: for (size_t i = 0; i < arraysize(test_cases);
i++) {
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> nit: ++i

Done.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list_unit...
base/feature_list_unittest.cc:216: for (size_t i = 0; i < arraysize(test_cases);
i++) {
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> nit: ++i

Done.

https://codereview.chromium.org/1306653004/diff/230001/base/metrics/field_tri...
File base/metrics/field_trial_unittest.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/base/metrics/field_tri...
base/metrics/field_trial_unittest.cc:402: // The trial starts inactive, so
|GetActiveGroup()| should return false.
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> This comment seems like it was probably written before you factored out
> IsFieldTrialActive -- please update it =)

Done.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor.cc:125: }
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> It might be nice to verify that no feature is simultaneously overridden to be
on
> and to be off, either by one study or by multiple.

This will be enforced server-side. Not sure what kind of verification you were
thinking of. The behavior is such that the first override take precedence.

Anyway, I've modified this to make RegisterOverride() internally in FeatureList
to return the result of the insert, and made RegisterFieldTrialOverride() check
that result and hit a NOTREACHED() if two trials tried to override the same
feature's state. Let me know if this is what you were thinking of.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor.cc:182: <<
experiment.forcing_flag();
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> Probably makes sense to either introduce similar logging below, or to remove
> this logging.

Done. Removed the logging, since don't remember a case when this was useful.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor.cc:262: if
(controls_feature_state)
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> nit: RegisterFeatureOverrides is safe to call even if there aren't any
features
> to override -- right?  If so, let's skip the boolean and this conditional, and
> just always call RegisterFeatureOverrides()

RegisterFeatureOverrides() uses GetExperimentIndexByName() which does a linear
iteration over experiments in the study to compare their names.

Sure, it's not a lot of work, but it's O(#studies * #experimentsInStudy *
#nameLength) operations. I think it's worth avoiding the extra work with the
simple if, given this is on Chrome's critical start up path.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/variations_seed_processor_unittest.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor_unittest.cc:546:
base::FeatureList::ClearInstanceForTesting();
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> Should this also be called at the end of each test, to avoid munging global
> state?

Good point. Put in the dtor of the harness.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor_unittest.cc:619: };
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> This test is pretty dense, making it hard to read.  It might be significantly
> clearer to just write 10 different, brief test cases; rather than trying to
> share code in this way.  (You could factor out some of the boilerplate-y
shared
> setup code into a helper function to keep the tests short and clear.)

Let me think about. I started it as individual tests, but made a table test
given the amount of test coverage I wanted. Plus, now you've given some
suggestions for additional tests.

Will save the thought for next patch set which will add extra tests.

Alexei Svitkine (slow)

https://codereview.chromium.org/1306653004/diff/230001/base/metrics/field_trial_unittest.cc File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/1306653004/diff/230001/base/metrics/field_trial_unittest.cc#newcode51 base/metrics/field_trial_unittest.cc:51: } On 2015/09/16 00:50:44, Ilya Sherman wrote: > nit: ...

5 years, 3 months ago (2015-09-23 17:33:32 UTC) #22

Alexei Svitkine (slow)

Addressed some more comments. There is still some stuff from isherman@'s review that I haven't ...

5 years, 3 months ago (2015-09-23 22:01:28 UTC) #23

Addressed some more comments.

There is still some stuff from isherman@'s review that I haven't gotten to,
mainly:
  - The bulleted-point list of edge cases, most of which are meant to be server
enforced. Haven't decided to what granularity I'd like to test them here.
Specifically, there are no tests for "illegal" scenarios - such as an experiment
usually multiple mutually exclusive fields or two studies trying to control a
feature at the same time. Or two experiments trying to do associate at the same
time. (All of these will be enforced server-side.)

Then, there's this discussion:

"
>   - Where a kill-switch mechanism needs to disable several things in the same
> config. e.g. both feature A and B are using the same helper class that has a
bug
> and is DDOS'ing servers. We'd like to disable them both. It's easier to do
this
> in a single study config that in separate ones. Especially if you want to do a
> ramp-up of the kill to be safe (e.g. disable both for 10% of users first).

This sort of configuration seems unlikely enough to me that having separate
configs for each feature doesn't sound like an unreasonable tradeoff for
increased simplicity in the common case.
"

First, as I mentioned above, separate configs don't support coordinated rollout.
So we would be losing functionality. Additionally, I feel like a "list of
features"
to enable, vs. "one feature to enable" is not much added mental complexity,
and gives us more flexibility.

Sure, if we switch the model around to something where we associate one feature
with one experiment, and have a mode of "enable_it, disable_it,
associate_with_enable_flag, associate_with_disable_flag",
then it's a different model and not just moving from a non-repeated field to a
repeated field. But I'm not convinced that mode is actually simpler.  It's just
different - and actually a bit more complicated at the config level imo
(assuming we set that as the how people define things on the server-side - as I
don't want to have conflicting format between the JSON and the proto.) Happy to
discuss more.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/proto/study.proto:103: // will cause this experiment to be
forced off, if that feature is
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> It's weird to say "will cause this experiment to be forced off", because the
> experiment (a.k.a. "group") will be reported -- which I would describe as "on"
> -- in the case that the given disable flag is present.

Good catch, you're right - the wording is incorrect. I think it was correct in
the original patch set but it looked like a typo since it seemed like it should
mimic the name of the field, i.e. the above should "on" and this one should be
"off". I've reworded it now to be less confusing. :)

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/proto/study.proto:106: optional string forcing_feature_off
= 4;
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> Am I understanding correctly that only one of forcing_feature_on and
> forcing_feature_off can be set?  If so, it would be nice to just have a single
> forcing_feature entry, which is itself a struct containing the feature name
and
> the feature's state.

I considered this approach earlier but didn't like it. Yes, it's true that only
one of these should be set - and we can use a sub-message with a state field.

But this seems a lot of extra boilerplate - especially since at the config level
server-side, I was planning to have the simple two fields (to make the JSON more
concise).

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/proto/study.proto:106: optional string forcing_feature_off
= 4;
On 2015/09/16 00:50:44, Ilya Sherman wrote:
> Am I understanding correctly that for a single experiment, either a
> forcing_feature or a set of enable/disable_features should be set, but not
both?

Correct. The following are mutually exclusive:

{forcing_flag, forcing_feature_on, forcing_feature_off, a non-zero
probability_weight}

Expanded comments on these fields.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/variations_seed_processor_unittest.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor_unittest.cc:619: };
On 2015/09/22 21:19:59, Alexei Svitkine (slow) wrote:
> On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > This test is pretty dense, making it hard to read.  It might be
significantly
> > clearer to just write 10 different, brief test cases; rather than trying to
> > share code in this way.  (You could factor out some of the boilerplate-y
> shared
> > setup code into a helper function to keep the tests short and clear.)
> 
> Let me think about. I started it as individual tests, but made a table test
> given the amount of test coverage I wanted. Plus, now you've given some
> suggestions for additional tests.
> 
> Will save the thought for next patch set which will add extra tests.

Thinking about this more, I agree that this test is hard to follow, but I also
think that splitting the cases into separate tests which just be way too much
boilerplate. Additionally, the test wasn't covering as many cases as we wanted.

So I went and re-wrote this test with a slightly different structure. The new
test is still table-driven, but I believe more clear and covers a lot more
cases. I've also added comments and grouped the entries in the table together to
make it clear what each section is trying to cover. I think it's much better
now, PTAL.

Ilya Sherman

Thanks, Alexei =) https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#newcode74 base/feature_list.cc:74: FieldTrialList::CreateFieldTrial(field_trial_name, group_name); On 2015/09/22 21:19:59, Alexei ...

5 years, 3 months ago (2015-09-24 01:11:33 UTC) #24

Thanks, Alexei =)

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:74: FieldTrialList::CreateFieldTrial(field_trial_name,
group_name);
On 2015/09/22 21:19:59, Alexei Svitkine (slow) wrote:
> On 2015/09/17 01:02:46, Ilya Sherman wrote:
> > On 2015/09/16 20:54:08, Alexei Svitkine (slow) wrote:
> > > On 2015/09/16 00:50:43, Ilya Sherman wrote:
> > > > Hmm, why create the field trial?  What guarantees that it doesn't
already
> > > exist?
> > > >  That is, why doesn't this method accept a FieldTrial* param, same as
the
> > > other
> > > > method?
> > > 
> > > The field trial should only be created through this mechanism if it's been
> > > forced through the command-line. This is similar logic to "forcing_flag"
> logic
> > > already in VariationsService.
> > > 
> > > The difference for why the trial is created here vs. for "forcing_flag"
> where
> > > it's created in VariationsService is because VariationsService can test
> > directly
> > > if a flag was forced, but can't (without adding an API) do so for feature
> > > forcing.
> > > 
> > > If we wanted the trial to be created in VariationsService, then we'd need
to
> > add
> > > an API on FeatureList to tell if a trial was overridden by the
command-line
> > > (either one or two functions depending if you want to combine
enable/disable
> > > into a more complicated function or have separate one). Then, when you
> decide
> > to
> > > create the trial, you still need to associate it with the entry - so a
> > separate
> > > Associate() call is still needed.
> > > 
> > > Per the above, I believe the approach implemented in this CL is actually
> > > simpler, because it encompasses the complexity in the single call, instead
> of
> > > splitting this logic between here and VariationService and exposing more
API
> > > surface.
> > 
> > I agree that it's simpler in a way, but it also seems to tightly couple the
> > VariationsService implementation with this one.  I had a lot of trouble
> > understanding the FeatureList API until I read the VariationsService code. 
> But,
> > I'm also not thinking of a great way to simplify this without introducing
the
> > split API like you described.  Maybe the code would be clearer if the field
> > trial creation were delegated to a callback passed in by the caller, but
maybe
> > that would actually just be even more confusing... :/
> 
> Yeah, I feel like the callback would be even harder to follow. The good thing
> about this API is it's not one that actually users of FeatureList API will
need
> to use - it's only for variations service, so even if it's hard to follow, not
a
> lot of people need to follow it.

After thinking about this more, I think I actually prefer exposing two separate
methods in the API.  It does increase the API surface area slightly, but it also
makes it possible to better separate the VariationsService logic from the
FeatureList logic, and makes the FeatureList logic clearer IMO.  Something like
this:

bool FeatureList::HasOverriddenFeature(const std::string& feature_name,
                                       OverrideState state) {
  auto it = overrides_.find(feature_name);
  return it != overrides_.end() && it->second.overridden_state == state;
}

FieldTrial* FeatureList::AssociateReportingFieldTrial(
    const std::string& feature_name,
    OverrideState for_overridden_state,
    FieldTrial* field_trial) {
  DCHECK(HasOverriddenFeature(feature_name, for_overridden_state));
  
  // Only one associated field trial is supported per feature. This is generally
  // enforced server-side.
  OverrideEntry& entry = overrides_[feature_name];
  if (entry.field_trial) {
    NOTREACHED() << "Feature = " << feature_name
                 << ", has trial = " << entry.field_trial->trial_name()
                 << ", associating trial = " << field_trial_name;
    return;
  }
  
  entry.field_trial = field_trial;
}

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/proto/study.proto:106: optional string forcing_feature_off
= 4;
On 2015/09/23 22:01:27, Alexei Svitkine (slow) wrote:
> On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > Am I understanding correctly that only one of forcing_feature_on and
> > forcing_feature_off can be set?  If so, it would be nice to just have a
single
> > forcing_feature entry, which is itself a struct containing the feature name
> and
> > the feature's state.
> 
> I considered this approach earlier but didn't like it. Yes, it's true that
only
> one of these should be set - and we can use a sub-message with a state field.
> 
> But this seems a lot of extra boilerplate - especially since at the config
level
> server-side, I was planning to have the simple two fields (to make the JSON
more
> concise).

Hmm.  Any experiment that has a forcing_feature_on will presumably have a sister
experiment that has a forcing_feature_off.  I'd think that the most concise way
to represent this would be a single top-level entry in the study config.  We
could probably just have users specify the forcing feature name, and we can
autogenerate the group names for them.  WDYT?

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor.cc:125: }
On 2015/09/22 21:19:59, Alexei Svitkine (slow) wrote:
> On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > It might be nice to verify that no feature is simultaneously overridden to
be
> on
> > and to be off, either by one study or by multiple.
> 
> This will be enforced server-side. Not sure what kind of verification you were
> thinking of. The behavior is such that the first override take precedence.
> 
> Anyway, I've modified this to make RegisterOverride() internally in
FeatureList
> to return the result of the insert, and made RegisterFieldTrialOverride()
check
> that result and hit a NOTREACHED() if two trials tried to override the same
> feature's state. Let me know if this is what you were thinking of.

Yep, I like that.  That way, if we ever fail to catch something on the
server-side, at least we'll have a record of it client-side.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor.cc:262: if
(controls_feature_state)
On 2015/09/22 21:19:59, Alexei Svitkine (slow) wrote:
> On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > nit: RegisterFeatureOverrides is safe to call even if there aren't any
> features
> > to override -- right?  If so, let's skip the boolean and this conditional,
and
> > just always call RegisterFeatureOverrides()
> 
> RegisterFeatureOverrides() uses GetExperimentIndexByName() which does a linear
> iteration over experiments in the study to compare their names.
> 
> Sure, it's not a lot of work, but it's O(#studies * #experimentsInStudy *
> #nameLength) operations. I think it's worth avoiding the extra work with the
> simple if, given this is on Chrome's critical start up path.

That sounds like we're saving on the order of thousands of ops, which seems
marginal compared to everything else we do even in just this function alone. 
But, I don't feel strongly about this.

If we do keep the if-stmt, I'd rename the variable to
"does_control_feature_state", because "controls" can be read as either a verb or
as a plural noun, and that garden path throws me off every time I read it :P

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/variations_seed_processor_unittest.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor_unittest.cc:619: };
On 2015/09/23 22:01:27, Alexei Svitkine (slow) wrote:
> On 2015/09/22 21:19:59, Alexei Svitkine (slow) wrote:
> > On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > > This test is pretty dense, making it hard to read.  It might be
> significantly
> > > clearer to just write 10 different, brief test cases; rather than trying
to
> > > share code in this way.  (You could factor out some of the boilerplate-y
> > shared
> > > setup code into a helper function to keep the tests short and clear.)
> > 
> > Let me think about. I started it as individual tests, but made a table test
> > given the amount of test coverage I wanted. Plus, now you've given some
> > suggestions for additional tests.
> > 
> > Will save the thought for next patch set which will add extra tests.
> 
> Thinking about this more, I agree that this test is hard to follow, but I also
> think that splitting the cases into separate tests which just be way too much
> boilerplate. Additionally, the test wasn't covering as many cases as we
wanted.
> 
> So I went and re-wrote this test with a slightly different structure. The new
> test is still table-driven, but I believe more clear and covers a lot more
> cases. I've also added comments and grouped the entries in the table together
to
> make it clear what each section is trying to cover. I think it's much better
> now, PTAL.

The comments help a bunch -- thanks!

I think my main frustration with these table-based tests is not that they're
table based, but that they use a relatively complex backing struct, with several
boolean fields and string fields that are frequently empty.  So, when reading
the code, I have to do a lot of cross referencing to understand what an
individual "" means or what "false, true" means.  Maybe as a compromise, we
could have one TEST_F per grouped block of tests?  I think that might make the
logic easier to follow, since each test would have a simpler struct needed to
define the inputs.

FWIW, here are some TOTT articles that come to mind for me:
go/tott/2014/07/episode-338-dont-put-logic-in-tests.html
go/tott/2014/07/episode-339-writing-descriptive-test.html
go/tott/2014/03/episode-322-test-behaviors-not-methods.html

https://codereview.chromium.org/1306653004/diff/270001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/270001/base/feature_list.cc#n...
base/feature_list.cc:63: }
I think that rather than returning a pair from RegisterOverride(), it might be
simpler to write this method as something like

  // It's not valid for two field trials to try to override the state of the
  // same feature.
  DCHECK(!ContainsKey(overrides_, feature_name) ||
         !overrides_[feature_name].field_trial)
      << "Feature " << feature_name
      << " has conflicting field trial overrides: "
      << result.first.field_trial->trial_name() << " / "
      << field_trial->trial_name();

  RegisterOverride(feature_name, override_state, field_trial);

Alexei Svitkine (slow)

Thanks Ilya. PTAL. https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc File base/feature_list.cc (right): https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#newcode74 base/feature_list.cc:74: FieldTrialList::CreateFieldTrial(field_trial_name, group_name); On 2015/09/24 01:11:32, Ilya ...

5 years, 3 months ago (2015-09-24 20:49:21 UTC) #26

Thanks Ilya. PTAL.

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:74: FieldTrialList::CreateFieldTrial(field_trial_name,
group_name);
On 2015/09/24 01:11:32, Ilya Sherman wrote:
> On 2015/09/22 21:19:59, Alexei Svitkine (slow) wrote:
> > On 2015/09/17 01:02:46, Ilya Sherman wrote:
> > > On 2015/09/16 20:54:08, Alexei Svitkine (slow) wrote:
> > > > On 2015/09/16 00:50:43, Ilya Sherman wrote:
> > > > > Hmm, why create the field trial?  What guarantees that it doesn't
> already
> > > > exist?
> > > > >  That is, why doesn't this method accept a FieldTrial* param, same as
> the
> > > > other
> > > > > method?
> > > > 
> > > > The field trial should only be created through this mechanism if it's
been
> > > > forced through the command-line. This is similar logic to "forcing_flag"
> > logic
> > > > already in VariationsService.
> > > > 
> > > > The difference for why the trial is created here vs. for "forcing_flag"
> > where
> > > > it's created in VariationsService is because VariationsService can test
> > > directly
> > > > if a flag was forced, but can't (without adding an API) do so for
feature
> > > > forcing.
> > > > 
> > > > If we wanted the trial to be created in VariationsService, then we'd
need
> to
> > > add
> > > > an API on FeatureList to tell if a trial was overridden by the
> command-line
> > > > (either one or two functions depending if you want to combine
> enable/disable
> > > > into a more complicated function or have separate one). Then, when you
> > decide
> > > to
> > > > create the trial, you still need to associate it with the entry - so a
> > > separate
> > > > Associate() call is still needed.
> > > > 
> > > > Per the above, I believe the approach implemented in this CL is actually
> > > > simpler, because it encompasses the complexity in the single call,
instead
> > of
> > > > splitting this logic between here and VariationService and exposing more
> API
> > > > surface.
> > > 
> > > I agree that it's simpler in a way, but it also seems to tightly couple
the
> > > VariationsService implementation with this one.  I had a lot of trouble
> > > understanding the FeatureList API until I read the VariationsService code.

> > But,
> > > I'm also not thinking of a great way to simplify this without introducing
> the
> > > split API like you described.  Maybe the code would be clearer if the
field
> > > trial creation were delegated to a callback passed in by the caller, but
> maybe
> > > that would actually just be even more confusing... :/
> > 
> > Yeah, I feel like the callback would be even harder to follow. The good
thing
> > about this API is it's not one that actually users of FeatureList API will
> need
> > to use - it's only for variations service, so even if it's hard to follow,
not
> a
> > lot of people need to follow it.
> 
> After thinking about this more, I think I actually prefer exposing two
separate
> methods in the API.  It does increase the API surface area slightly, but it
also
> makes it possible to better separate the VariationsService logic from the
> FeatureList logic, and makes the FeatureList logic clearer IMO.  Something
like
> this:
> 
> bool FeatureList::HasOverriddenFeature(const std::string& feature_name,
>                                        OverrideState state) {
>   auto it = overrides_.find(feature_name);
>   return it != overrides_.end() && it->second.overridden_state == state;
> }
> 
> FieldTrial* FeatureList::AssociateReportingFieldTrial(
>     const std::string& feature_name,
>     OverrideState for_overridden_state,
>     FieldTrial* field_trial) {
>   DCHECK(HasOverriddenFeature(feature_name, for_overridden_state));
>   
>   // Only one associated field trial is supported per feature. This is
generally
>   // enforced server-side.
>   OverrideEntry& entry = overrides_[feature_name];
>   if (entry.field_trial) {
>     NOTREACHED() << "Feature = " << feature_name
>                  << ", has trial = " << entry.field_trial->trial_name()
>                  << ", associating trial = " << field_trial_name;
>     return;
>   }
>   
>   entry.field_trial = field_trial;
> }

I'm ok with this approach. It does let some code to be slightly simplified in
variations_seed_processor.cc too.

One thing I'm a little bit iffy on is HasOverriddenFeature() semantics. In your
suggestion, it will return true even if the feature is overridden by something
other than the command-line. And will return different results as more override
methods are called during initialization.

I understand that this helps detect overlap issues in practice (i.e. we don't
want two trials affecting the same feature), by having a DCHECK in the Associate
function, but it still feels like the logic is wrong for VariationsSeedProcessor
using it.

In fact, it doesn't just feel wrong, it is wrong. Because with this approach,
the check flow will be:

if (HasOverriddenFeature()) {
  trial = CreateFieldTrial();
  AssociateFieldTrial(trial);
}

In a debug build, it will just crash. But in prod, it will result in weirdness
if ever it does happen. Specifically, the trial will still be created, although
I guess not associated.

I'd rather avoid that weird behavior. So, we can make the function
IsFeatureOverriddenFromCommandLine() which specifically checks that field_trial
is null on the override entry. This makes the semantics much clearer - i.e.
makes the variations code associate only with command-line overridden things,
instead of all overridden things - which is what it's supposed to be doing,

(It does lose the NOTREACHED() firing in this case, but I think that's OK
because likely in such a case, the dice-rolled group of the experiment might try
to force the feature, which will trigger a NOTREACHED() in
RegisterFieldTrialOverride()).

Anyway, I've implemented the above suggestion % making the new function
IsFeatureOverriddenFromCommandLine() - which required an extra book-keeping
variable on the OverrideEntry to track whether the override is from a trial (or
otherwise from a command-line). Added a test for the new function as well.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/proto/study.proto:106: optional string forcing_feature_off
= 4;
On 2015/09/24 01:11:32, Ilya Sherman wrote:
> On 2015/09/23 22:01:27, Alexei Svitkine (slow) wrote:
> > On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > > Am I understanding correctly that only one of forcing_feature_on and
> > > forcing_feature_off can be set?  If so, it would be nice to just have a
> single
> > > forcing_feature entry, which is itself a struct containing the feature
name
> > and
> > > the feature's state.
> > 
> > I considered this approach earlier but didn't like it. Yes, it's true that
> only
> > one of these should be set - and we can use a sub-message with a state
field.
> > 
> > But this seems a lot of extra boilerplate - especially since at the config
> level
> > server-side, I was planning to have the simple two fields (to make the JSON
> more
> > concise).
> 
> Hmm.  Any experiment that has a forcing_feature_on will presumably have a
sister
> experiment that has a forcing_feature_off.  I'd think that the most concise
way
> to represent this would be a single top-level entry in the study config.  We
> could probably just have users specify the forcing feature name, and we can
> autogenerate the group names for them.  WDYT?

Again, I think that reduces flexibility. We've seen people use multiple
forcing_flag entries in a study config - for example in QuicStable.json and I
suspect there will be cases where you don't want to do the dice roll experiments
if multiple different things are forced.

Note that this still allows us to have syntax sugar at the config level, if
desired, but the point is it makes the protocol support the extra flexibility,
which is desirable in some cases.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/variations_seed_processor.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor.cc:262: if
(controls_feature_state)
On 2015/09/24 01:11:32, Ilya Sherman wrote:
> On 2015/09/22 21:19:59, Alexei Svitkine (slow) wrote:
> > On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > > nit: RegisterFeatureOverrides is safe to call even if there aren't any
> > features
> > > to override -- right?  If so, let's skip the boolean and this conditional,
> and
> > > just always call RegisterFeatureOverrides()
> > 
> > RegisterFeatureOverrides() uses GetExperimentIndexByName() which does a
linear
> > iteration over experiments in the study to compare their names.
> > 
> > Sure, it's not a lot of work, but it's O(#studies * #experimentsInStudy *
> > #nameLength) operations. I think it's worth avoiding the extra work with the
> > simple if, given this is on Chrome's critical start up path.
> 
> That sounds like we're saving on the order of thousands of ops, which seems
> marginal compared to everything else we do even in just this function alone. 
> But, I don't feel strongly about this.
> 
> If we do keep the if-stmt, I'd rename the variable to
> "does_control_feature_state", because "controls" can be read as either a verb
or
> as a plural noun, and that garden path throws me off every time I read it :P

Changed to |enables_or_disables_features|, as I found "does_" wording awkward.
Let me know if this works for you.

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/variations_seed_processor_unittest.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/variations_seed_processor_unittest.cc:619: };
On 2015/09/24 01:11:32, Ilya Sherman wrote:
> On 2015/09/23 22:01:27, Alexei Svitkine (slow) wrote:
> > On 2015/09/22 21:19:59, Alexei Svitkine (slow) wrote:
> > > On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > > > This test is pretty dense, making it hard to read.  It might be
> > significantly
> > > > clearer to just write 10 different, brief test cases; rather than trying
> to
> > > > share code in this way.  (You could factor out some of the boilerplate-y
> > > shared
> > > > setup code into a helper function to keep the tests short and clear.)
> > > 
> > > Let me think about. I started it as individual tests, but made a table
test
> > > given the amount of test coverage I wanted. Plus, now you've given some
> > > suggestions for additional tests.
> > > 
> > > Will save the thought for next patch set which will add extra tests.
> > 
> > Thinking about this more, I agree that this test is hard to follow, but I
also
> > think that splitting the cases into separate tests which just be way too
much
> > boilerplate. Additionally, the test wasn't covering as many cases as we
> wanted.
> > 
> > So I went and re-wrote this test with a slightly different structure. The
new
> > test is still table-driven, but I believe more clear and covers a lot more
> > cases. I've also added comments and grouped the entries in the table
together
> to
> > make it clear what each section is trying to cover. I think it's much better
> > now, PTAL.
> 
> The comments help a bunch -- thanks!
> 
> I think my main frustration with these table-based tests is not that they're
> table based, but that they use a relatively complex backing struct, with
several
> boolean fields and string fields that are frequently empty.  So, when reading
> the code, I have to do a lot of cross referencing to understand what an
> individual "" means or what "false, true" means.  Maybe as a compromise, we
> could have one TEST_F per grouped block of tests?  I think that might make the
> logic easier to follow, since each test would have a simpler struct needed to
> define the inputs.
> 
> FWIW, here are some TOTT articles that come to mind for me:
> go/tott/2014/07/episode-338-dont-put-logic-in-tests.html
> go/tott/2014/07/episode-339-writing-descriptive-test.html
> go/tott/2014/03/episode-322-test-behaviors-not-methods.html

I agree that complicated table-driven tests are hard to read. However, I think
my updated code actually has a good balance between the two. Sure, you do have
to go up to figure out what "" is, but hopefully this is not something too many
people have to do - other than during this code review and if the logic needs
updating.

I don't think the splitting the different blocks of tests in the table are super
useful in this case. Particularly, because this test basically is a sort of
"brute force" case where it tries all the different combinations of
{default_feature_state, enabled_by_cli, disable_by_cli, dice_rolled_group} for
the config.

 And splitting some of those off just makes the test seem incomplete - and also
would result in more boilerplate (i.e. either the code from 622 to 661 would
essentially be duplicated, or we have to make a helper method which makes the
test harder to follow).

So I don't find splitting this specific test compelling. On the other hand, I
agree that it's worth having a few more tests for this general functionality and
plan to add some, to cover some of your bullet point cases from earlier
comments. Haven't gotten to that yet, though.

https://codereview.chromium.org/1306653004/diff/270001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/270001/base/feature_list.cc#n...
base/feature_list.cc:63: }
On 2015/09/24 01:11:32, Ilya Sherman wrote:
> I think that rather than returning a pair from RegisterOverride(), it might be
> simpler to write this method as something like
> 
>   // It's not valid for two field trials to try to override the state of the
>   // same feature.
>   DCHECK(!ContainsKey(overrides_, feature_name) ||
>          !overrides_[feature_name].field_trial)
>       << "Feature " << feature_name
>       << " has conflicting field trial overrides: "
>       << result.first.field_trial->trial_name() << " / "
>       << field_trial->trial_name();
> 
>   RegisterOverride(feature_name, override_state, field_trial);

Thanks, that does simplify things a lot! Done.

Ilya Sherman

Thanks! I think the API exposed by the latest revision makes the code much more ...

5 years, 3 months ago (2015-09-25 03:47:22 UTC) #27

Thanks!  I think the API exposed by the latest revision makes the code much more
straightforward =)

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/230001/base/feature_list.cc#n...
base/feature_list.cc:74: FieldTrialList::CreateFieldTrial(field_trial_name,
group_name);
On 2015/09/24 20:49:20, Alexei Svitkine (slow) wrote:
> On 2015/09/24 01:11:32, Ilya Sherman wrote:
> > On 2015/09/22 21:19:59, Alexei Svitkine (slow) wrote:
> > > On 2015/09/17 01:02:46, Ilya Sherman wrote:
> > > > On 2015/09/16 20:54:08, Alexei Svitkine (slow) wrote:
> > > > > On 2015/09/16 00:50:43, Ilya Sherman wrote:
> > > > > > Hmm, why create the field trial?  What guarantees that it doesn't
> > already
> > > > > exist?
> > > > > >  That is, why doesn't this method accept a FieldTrial* param, same
as
> > the
> > > > > other
> > > > > > method?
> > > > > 
> > > > > The field trial should only be created through this mechanism if it's
> been
> > > > > forced through the command-line. This is similar logic to
"forcing_flag"
> > > logic
> > > > > already in VariationsService.
> > > > > 
> > > > > The difference for why the trial is created here vs. for
"forcing_flag"
> > > where
> > > > > it's created in VariationsService is because VariationsService can
test
> > > > directly
> > > > > if a flag was forced, but can't (without adding an API) do so for
> feature
> > > > > forcing.
> > > > > 
> > > > > If we wanted the trial to be created in VariationsService, then we'd
> need
> > to
> > > > add
> > > > > an API on FeatureList to tell if a trial was overridden by the
> > command-line
> > > > > (either one or two functions depending if you want to combine
> > enable/disable
> > > > > into a more complicated function or have separate one). Then, when you
> > > decide
> > > > to
> > > > > create the trial, you still need to associate it with the entry - so a
> > > > separate
> > > > > Associate() call is still needed.
> > > > > 
> > > > > Per the above, I believe the approach implemented in this CL is
actually
> > > > > simpler, because it encompasses the complexity in the single call,
> instead
> > > of
> > > > > splitting this logic between here and VariationService and exposing
more
> > API
> > > > > surface.
> > > > 
> > > > I agree that it's simpler in a way, but it also seems to tightly couple
> the
> > > > VariationsService implementation with this one.  I had a lot of trouble
> > > > understanding the FeatureList API until I read the VariationsService
code.
> 
> > > But,
> > > > I'm also not thinking of a great way to simplify this without
introducing
> > the
> > > > split API like you described.  Maybe the code would be clearer if the
> field
> > > > trial creation were delegated to a callback passed in by the caller, but
> > maybe
> > > > that would actually just be even more confusing... :/
> > > 
> > > Yeah, I feel like the callback would be even harder to follow. The good
> thing
> > > about this API is it's not one that actually users of FeatureList API will
> > need
> > > to use - it's only for variations service, so even if it's hard to follow,
> not
> > a
> > > lot of people need to follow it.
> > 
> > After thinking about this more, I think I actually prefer exposing two
> separate
> > methods in the API.  It does increase the API surface area slightly, but it
> also
> > makes it possible to better separate the VariationsService logic from the
> > FeatureList logic, and makes the FeatureList logic clearer IMO.  Something
> like
> > this:
> > 
> > bool FeatureList::HasOverriddenFeature(const std::string& feature_name,
> >                                        OverrideState state) {
> >   auto it = overrides_.find(feature_name);
> >   return it != overrides_.end() && it->second.overridden_state == state;
> > }
> > 
> > FieldTrial* FeatureList::AssociateReportingFieldTrial(
> >     const std::string& feature_name,
> >     OverrideState for_overridden_state,
> >     FieldTrial* field_trial) {
> >   DCHECK(HasOverriddenFeature(feature_name, for_overridden_state));
> >   
> >   // Only one associated field trial is supported per feature. This is
> generally
> >   // enforced server-side.
> >   OverrideEntry& entry = overrides_[feature_name];
> >   if (entry.field_trial) {
> >     NOTREACHED() << "Feature = " << feature_name
> >                  << ", has trial = " << entry.field_trial->trial_name()
> >                  << ", associating trial = " << field_trial_name;
> >     return;
> >   }
> >   
> >   entry.field_trial = field_trial;
> > }
> 
> I'm ok with this approach. It does let some code to be slightly simplified in
> variations_seed_processor.cc too.
> 
> One thing I'm a little bit iffy on is HasOverriddenFeature() semantics. In
your
> suggestion, it will return true even if the feature is overridden by something
> other than the command-line. And will return different results as more
override
> methods are called during initialization.
> 
> I understand that this helps detect overlap issues in practice (i.e. we don't
> want two trials affecting the same feature), by having a DCHECK in the
Associate
> function, but it still feels like the logic is wrong for
VariationsSeedProcessor
> using it.
> 
> In fact, it doesn't just feel wrong, it is wrong. Because with this approach,
> the check flow will be:
> 
> if (HasOverriddenFeature()) {
>   trial = CreateFieldTrial();
>   AssociateFieldTrial(trial);
> }
> 
> In a debug build, it will just crash. But in prod, it will result in weirdness
> if ever it does happen. Specifically, the trial will still be created,
although
> I guess not associated.
> 
> I'd rather avoid that weird behavior. So, we can make the function
> IsFeatureOverriddenFromCommandLine() which specifically checks that
field_trial
> is null on the override entry. This makes the semantics much clearer - i.e.
> makes the variations code associate only with command-line overridden things,
> instead of all overridden things - which is what it's supposed to be doing,
> 
> (It does lose the NOTREACHED() firing in this case, but I think that's OK
> because likely in such a case, the dice-rolled group of the experiment might
try
> to force the feature, which will trigger a NOTREACHED() in
> RegisterFieldTrialOverride()).
> 
> Anyway, I've implemented the above suggestion % making the new function
> IsFeatureOverriddenFromCommandLine() - which required an extra book-keeping
> variable on the OverrideEntry to track whether the override is from a trial
(or
> otherwise from a command-line). Added a test for the new function as well.

Nice, I like the explicit command line check even better than what I suggested
=)

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/proto/study.proto:106: optional string forcing_feature_off
= 4;
On 2015/09/24 20:49:20, Alexei Svitkine (slow) wrote:
> On 2015/09/24 01:11:32, Ilya Sherman wrote:
> > On 2015/09/23 22:01:27, Alexei Svitkine (slow) wrote:
> > > On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > > > Am I understanding correctly that only one of forcing_feature_on and
> > > > forcing_feature_off can be set?  If so, it would be nice to just have a
> > single
> > > > forcing_feature entry, which is itself a struct containing the feature
> name
> > > and
> > > > the feature's state.
> > > 
> > > I considered this approach earlier but didn't like it. Yes, it's true that
> > only
> > > one of these should be set - and we can use a sub-message with a state
> field.
> > > 
> > > But this seems a lot of extra boilerplate - especially since at the config
> > level
> > > server-side, I was planning to have the simple two fields (to make the
JSON
> > more
> > > concise).
> > 
> > Hmm.  Any experiment that has a forcing_feature_on will presumably have a
> sister
> > experiment that has a forcing_feature_off.  I'd think that the most concise
> way
> > to represent this would be a single top-level entry in the study config.  We
> > could probably just have users specify the forcing feature name, and we can
> > autogenerate the group names for them.  WDYT?
> 
> Again, I think that reduces flexibility. We've seen people use multiple
> forcing_flag entries in a study config - for example in QuicStable.json and I
> suspect there will be cases where you don't want to do the dice roll
experiments
> if multiple different things are forced.
> 
> Note that this still allows us to have syntax sugar at the config level, if
> desired, but the point is it makes the protocol support the extra flexibility,
> which is desirable in some cases.

Well, the field could be repeated, if the concern is just that people might want
to associate with multiple features.  Was that the concern, or was there an
additional part that I misunderstood?

My main point here is that I'd expect these experiments to always come in pairs:
One for the feature being forced on, and one for being forced off.  Since the
same feature name controls both cases, it seems silly for the protocol to need
to repeat the feature name for the enabled and disabled cases.

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:53: !it->second.overriden_by_field_trial;
nit: s/riden/ridden

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:61: !overrides_.at(feature_name).field_trial)
nit: Please use operator[] (or .find()) rather than .at(), as .at() can throw an
exception.  Note that the ContainsKey call ensures that operator[] is
side-effect free.

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:64: <<
overrides_.at(feature_name).field_trial->trial_name() << " / "
(here too)

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:79: OverrideEntry* entry = &overrides_.at(feature_name);
nit: Here too.

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:147: make_pair(feature_name,
OverrideEntry(overridden_state, field_trial)));
nit: Missing "std::" in front of make_pair?

Alexei Svitkine (slow)

https://codereview.chromium.org/1306653004/diff/230001/components/variations/proto/study.proto File components/variations/proto/study.proto (right): https://codereview.chromium.org/1306653004/diff/230001/components/variations/proto/study.proto#newcode106 components/variations/proto/study.proto:106: optional string forcing_feature_off = 4; On 2015/09/25 03:47:22, Ilya ...

5 years, 3 months ago (2015-09-25 03:56:52 UTC) #28

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/proto/study.proto:106: optional string forcing_feature_off
= 4;
On 2015/09/25 03:47:22, Ilya Sherman wrote:
> On 2015/09/24 20:49:20, Alexei Svitkine (slow) wrote:
> > On 2015/09/24 01:11:32, Ilya Sherman wrote:
> > > On 2015/09/23 22:01:27, Alexei Svitkine (slow) wrote:
> > > > On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > > > > Am I understanding correctly that only one of forcing_feature_on and
> > > > > forcing_feature_off can be set?  If so, it would be nice to just have
a
> > > single
> > > > > forcing_feature entry, which is itself a struct containing the feature
> > name
> > > > and
> > > > > the feature's state.
> > > > 
> > > > I considered this approach earlier but didn't like it. Yes, it's true
that
> > > only
> > > > one of these should be set - and we can use a sub-message with a state
> > field.
> > > > 
> > > > But this seems a lot of extra boilerplate - especially since at the
config
> > > level
> > > > server-side, I was planning to have the simple two fields (to make the
> JSON
> > > more
> > > > concise).
> > > 
> > > Hmm.  Any experiment that has a forcing_feature_on will presumably have a
> > sister
> > > experiment that has a forcing_feature_off.  I'd think that the most
concise
> > way
> > > to represent this would be a single top-level entry in the study config. 
We
> > > could probably just have users specify the forcing feature name, and we
can
> > > autogenerate the group names for them.  WDYT?
> > 
> > Again, I think that reduces flexibility. We've seen people use multiple
> > forcing_flag entries in a study config - for example in QuicStable.json and
I
> > suspect there will be cases where you don't want to do the dice roll
> experiments
> > if multiple different things are forced.
> > 
> > Note that this still allows us to have syntax sugar at the config level, if
> > desired, but the point is it makes the protocol support the extra
flexibility,
> > which is desirable in some cases.
> 
> Well, the field could be repeated, if the concern is just that people might
want
> to associate with multiple features.  Was that the concern, or was there an
> additional part that I misunderstood?

Groups have other features though, such as associated experiment ids and other
things. So if we move everything top-level to study and make things repeated, we
either lose those functionalities (which people have actually asked for/used -
it used to be that we didn't support them for forcing_flags) or we end up having
to add more fields to support those things, which is not very maintainable and
doesn't allow code sharing. The other option is to have a top-level list of
strings and the protocol to instead specify indexes into that array. But I don't
feel that's worth it - see also my note about gzip below.

> 
> My main point here is that I'd expect these experiments to always come in
pairs:
> One for the feature being forced on, and one for being forced off.  Since the
> same feature name controls both cases, it seems silly for the protocol to need
> to repeat the feature name for the enabled and disabled cases.

I agree that repeating strings is not ideal. However, probably this sort of
thing becomes mostly moot if the data format is gzip-compressed. (I think right
now we don't do this for the server payload, but there's a bug on file to start
doing this.)

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:61: !overrides_.at(feature_name).field_trial)
On 2015/09/25 03:47:22, Ilya Sherman wrote:
> nit: Please use operator[] (or .find()) rather than .at(), as .at() can throw
an
> exception.  Note that the ContainsKey call ensures that operator[] is
> side-effect free.

I used at() because [] requires a default ctor. I suppose there's no "harm" in
adding one, since our access with [] ensures we don't accidentally add extra
entries into the map. Didn't realise that at() throws though..

Ilya Sherman

https://codereview.chromium.org/1306653004/diff/230001/components/variations/proto/study.proto File components/variations/proto/study.proto (right): https://codereview.chromium.org/1306653004/diff/230001/components/variations/proto/study.proto#newcode106 components/variations/proto/study.proto:106: optional string forcing_feature_off = 4; On 2015/09/25 03:56:51, Alexei ...

5 years, 3 months ago (2015-09-25 04:33:36 UTC) #29

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
File components/variations/proto/study.proto (right):

https://codereview.chromium.org/1306653004/diff/230001/components/variations/...
components/variations/proto/study.proto:106: optional string forcing_feature_off
= 4;
On 2015/09/25 03:56:51, Alexei Svitkine (slow) wrote:
> On 2015/09/25 03:47:22, Ilya Sherman wrote:
> > On 2015/09/24 20:49:20, Alexei Svitkine (slow) wrote:
> > > On 2015/09/24 01:11:32, Ilya Sherman wrote:
> > > > On 2015/09/23 22:01:27, Alexei Svitkine (slow) wrote:
> > > > > On 2015/09/16 00:50:44, Ilya Sherman wrote:
> > > > > > Am I understanding correctly that only one of forcing_feature_on and
> > > > > > forcing_feature_off can be set?  If so, it would be nice to just
have
> a
> > > > single
> > > > > > forcing_feature entry, which is itself a struct containing the
feature
> > > name
> > > > > and
> > > > > > the feature's state.
> > > > > 
> > > > > I considered this approach earlier but didn't like it. Yes, it's true
> that
> > > > only
> > > > > one of these should be set - and we can use a sub-message with a state
> > > field.
> > > > > 
> > > > > But this seems a lot of extra boilerplate - especially since at the
> config
> > > > level
> > > > > server-side, I was planning to have the simple two fields (to make the
> > JSON
> > > > more
> > > > > concise).
> > > > 
> > > > Hmm.  Any experiment that has a forcing_feature_on will presumably have
a
> > > sister
> > > > experiment that has a forcing_feature_off.  I'd think that the most
> concise
> > > way
> > > > to represent this would be a single top-level entry in the study config.

> We
> > > > could probably just have users specify the forcing feature name, and we
> can
> > > > autogenerate the group names for them.  WDYT?
> > > 
> > > Again, I think that reduces flexibility. We've seen people use multiple
> > > forcing_flag entries in a study config - for example in QuicStable.json
and
> I
> > > suspect there will be cases where you don't want to do the dice roll
> > experiments
> > > if multiple different things are forced.
> > > 
> > > Note that this still allows us to have syntax sugar at the config level,
if
> > > desired, but the point is it makes the protocol support the extra
> flexibility,
> > > which is desirable in some cases.
> > 
> > Well, the field could be repeated, if the concern is just that people might
> want
> > to associate with multiple features.  Was that the concern, or was there an
> > additional part that I misunderstood?
> 
> Groups have other features though, such as associated experiment ids and other
> things. So if we move everything top-level to study and make things repeated,
we
> either lose those functionalities (which people have actually asked for/used -
> it used to be that we didn't support them for forcing_flags) or we end up
having
> to add more fields to support those things, which is not very maintainable and
> doesn't allow code sharing. The other option is to have a top-level list of
> strings and the protocol to instead specify indexes into that array. But I
don't
> feel that's worth it - see also my note about gzip below.

Alright, you've convinced me :)

> > My main point here is that I'd expect these experiments to always come in
> pairs:
> > One for the feature being forced on, and one for being forced off.  Since
the
> > same feature name controls both cases, it seems silly for the protocol to
need
> > to repeat the feature name for the enabled and disabled cases.
> 
> I agree that repeating strings is not ideal. However, probably this sort of
> thing becomes mostly moot if the data format is gzip-compressed. (I think
right
> now we don't do this for the server payload, but there's a bug on file to
start
> doing this.)

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:61: !overrides_.at(feature_name).field_trial)
On 2015/09/25 03:56:52, Alexei Svitkine (slow) wrote:
> On 2015/09/25 03:47:22, Ilya Sherman wrote:
> > nit: Please use operator[] (or .find()) rather than .at(), as .at() can
throw
> an
> > exception.  Note that the ContainsKey call ensures that operator[] is
> > side-effect free.
> 
> I used at() because [] requires a default ctor. I suppose there's no "harm" in
> adding one, since our access with [] ensures we don't accidentally add extra
> entries into the map. Didn't realise that at() throws though..

Hrm.  I guess we could also just use .find() instead of either .at() or
operator[], since .find() neither throws nor requires a default constructor.

Alexei Svitkine (slow)

PTAL. I still plan to add a few more tests to variations seed processor for ...

5 years, 2 months ago (2015-09-25 15:27:18 UTC) #30

PTAL.

I still plan to add a few more tests to variations seed processor for the
different edge cases mentioned. Will do that Monday.

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc
File base/feature_list.cc (right):

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:53: !it->second.overriden_by_field_trial;
On 2015/09/25 03:47:22, Ilya Sherman wrote:
> nit: s/riden/ridden

Done.

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:61: !overrides_.at(feature_name).field_trial)
On 2015/09/25 04:33:36, Ilya Sherman wrote:
> On 2015/09/25 03:56:52, Alexei Svitkine (slow) wrote:
> > On 2015/09/25 03:47:22, Ilya Sherman wrote:
> > > nit: Please use operator[] (or .find()) rather than .at(), as .at() can
> throw
> > an
> > > exception.  Note that the ContainsKey call ensures that operator[] is
> > > side-effect free.
> > 
> > I used at() because [] requires a default ctor. I suppose there's no "harm"
in
> > adding one, since our access with [] ensures we don't accidentally add extra
> > entries into the map. Didn't realise that at() throws though..
> 
> Hrm.  I guess we could also just use .find() instead of either .at() or
> operator[], since .find() neither throws nor requires a default constructor.

Done.

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:64: <<
overrides_.at(feature_name).field_trial->trial_name() << " / "
On 2015/09/25 03:47:22, Ilya Sherman wrote:
> (here too)

Done.

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:79: OverrideEntry* entry = &overrides_.at(feature_name);
On 2015/09/25 03:47:22, Ilya Sherman wrote:
> nit: Here too.

Done.

https://codereview.chromium.org/1306653004/diff/310001/base/feature_list.cc#n...
base/feature_list.cc:147: make_pair(feature_name,
OverrideEntry(overridden_state, field_trial)));
On 2015/09/25 03:47:22, Ilya Sherman wrote:
> nit: Missing "std::" in front of make_pair?

Done. Not sure why it worked before.

Alexei Svitkine (slow)

asvitkine@chromium.org changed reviewers: + brettw@chromium.org

5 years, 2 months ago (2015-09-28 15:02:39 UTC) #32

Alexei Svitkine (slow)

+brettw, please review changes to chrome/browser/chrome_browser_main.cc

5 years, 2 months ago (2015-09-28 15:02:39 UTC) #33

brettw

chrome_browser_main lgtm https://codereview.chromium.org/1306653004/diff/370001/base/feature_list.h File base/feature_list.h (right): https://codereview.chromium.org/1306653004/diff/370001/base/feature_list.h#newcode10 base/feature_list.h:10: #include <utility> I don't see what this ...

5 years, 2 months ago (2015-09-28 20:40:55 UTC) #34

Alexei Svitkine (slow)

Thanks! https://codereview.chromium.org/1306653004/diff/370001/base/feature_list.h File base/feature_list.h (right): https://codereview.chromium.org/1306653004/diff/370001/base/feature_list.h#newcode10 base/feature_list.h:10: #include <utility> On 2015/09/28 20:40:55, brettw wrote: > ...

5 years, 2 months ago (2015-09-28 20:54:59 UTC) #35

Alexei Svitkine (slow)

The CQ bit was checked by asvitkine@chromium.org

5 years, 2 months ago (2015-09-28 20:55:04 UTC) #36

Alexei Svitkine (slow)

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, brettw@chromium.org, isherman@chromium.org Link ...

5 years, 2 months ago (2015-09-28 20:55:04 UTC) #37

commit-bot: I haz the power

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

5 years, 2 months ago (2015-09-28 20:55:28 UTC) #38

commit-bot: I haz the power

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

5 years, 2 months ago (2015-09-28 21:00:41 UTC) #39

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_32_ng/builds/102852) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, ...

5 years, 2 months ago (2015-09-28 21:00:41 UTC) #40

Alexei Svitkine (slow)

The CQ bit was checked by asvitkine@chromium.org

5 years, 2 months ago (2015-09-28 21:41:57 UTC) #41

Alexei Svitkine (slow)

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, brettw@chromium.org, isherman@chromium.org Link ...

5 years, 2 months ago (2015-09-28 21:41:57 UTC) #42

commit-bot: I haz the power

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

5 years, 2 months ago (2015-09-28 21:43:22 UTC) #43

commit-bot: I haz the power

5 years, 2 months ago (2015-09-28 23:25:31 UTC) #45

Message was sent while issue was closed.

Patchset 12 (id:??) landed as
https://crrev.com/8423d1721a4cf444a0dc5935b78bbf38ea82d834
Cr-Commit-Position: refs/heads/master@{#351199}

Issue 1306653004: Expand FeatureList to support FieldTrial association. (Closed)

Description

Patch Set 1 : #

Patch Set 2 : Rebase #

Patch Set 3 : Rebase. #

Patch Set 4 : Address Rob's comments. #

Patch Set 5 : Address some of isherman@'s comments. #

Patch Set 6 : More of isherman@'s comments addressed. #

Patch Set 7 : Address latest comments from isherman@. #

Patch Set 8 : Rebase. #

Patch Set 9 : Adopt FieldTrialList::IsTrialActive() API. #

Patch Set 10 : overriden -> overridden; at() -> find() #

Patch Set 11 : Move <utility> include to .cc. #

Patch Set 12 : Rebase. #

Messages