Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9705074:
Supporting command line argument to force field trials (Closed)
|
Created:
8 years, 9 months ago by MAD Modified:
8 years, 8 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, jar (doing other things), tburkard+watch_chromium.org, jam, cbentzel+watch_chromium.org, joi+watch-content_chromium.org, dominich+watch_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, estade+watch_chromium.org, James Su, Ilya Sherman, SteveT Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionField Trials choices can now be forced from a command line argument, and yet still behave as if a coin was tossed, except the coin is tricked :-).
To do this, we needed to change the usage from a regular constructor (which is not private) to a static CreateInstance method (which is why there is a bunch of TBR'd owners, those changes were trivial to existing users of FieldTrials).
OWNERs of trivially changed files:
TBR=sky,jamesr,cpu,joi,
BUG=119726
TEST=base_unittests.exe --gtest_filter=FieldTrialTest.*
You can also find an active field trial name and force it to a given group by passing the following command line argument "--force-fieldtest=<trial_name>/<default_group_name>/<group_name>/"
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131948
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #
Total comments: 16
Patch Set 3 : Addressed CR Comments added a unit test. #
Total comments: 28
Patch Set 4 : Addressed OWNER's cooments... #
Total comments: 11
Patch Set 5 : No more static default group number #
Total comments: 20
Patch Set 6 : Addressed second round of CR comments from asvitkine #
Total comments: 31
Patch Set 7 : Addressed second round of OWNER comments. #
Total comments: 1
Patch Set 8 : Fixed some ooopss... #Messages
Total messages: 35 (0 generated)
Please... Thanks! BYE MAD
Oupssss... Wait... I have some more changes locally that didn't get uploaded... I'll send another patch ASAP... Sorry about that...
Hey MAD, Mostly looks good with some nits. Could you also add a unit test for this? (i.e. creating a forced trial then calling CreateInstance() and making sure the same one is returned with the correct group value) https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... base/metrics/field_trial.cc:35: // First check if the field trial has already been created in some other ways. Nit: "some other ways" -> "some other way" https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... base/metrics/field_trial.cc:39: } else { Nit: Since you're early returning, don't need an else block. https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... base/metrics/field_trial.h:119: // Using this constructor creates a startup-randomized FieldTrial. If you Update this comment, since this is no longer a constructor. Also, please move the last paragraph to the beginning of the comment since that introduces what the function does. https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.cc:42: name, total_probability, default_group_name, year, month, day_of_month); Nit: just return the new FieldTrial(...) directly. https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.cc:119: if (forced_) { This block needs a comment explaining why this code is doing this. https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.cc:365: FieldTrial* field_trial(FieldTrialList::Find(name)); Nit: Can you change this to use = syntax, since it's a pointer? https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.cc:377: field_trial->forced_ = true; Are you sure this is okay to do for all trials created this way? For example, for trials created in the renderer, or for some of the other existing callers of this function? https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.h:121: // after construction. Please also mention that this may returned an existing field trial, if it had been forced. https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.h:254: // When forced_ is true, we return chosen group from AppendGroup when Nit: grammar - "we return chosen group" -> "we return the chosen group" https://chromiumcodereview.appspot.com/9705074/diff/8002/chrome/browser/chrom... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/8002/chrome/browser/chrom... chrome/browser/chrome_browser_main.cc:621: // and stop the metrics service so that we don't pollute UMA. Nit: Split this into two sentences. https://chromiumcodereview.appspot.com/9705074/diff/8002/chrome/browser/prere... File chrome/browser/prerender/prerender_field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/8002/chrome/browser/prere... chrome/browser/prerender/prerender_field_trial.cc:194: kOmniboxTrialName, kDivisor, "OmniboxPrerenderEnabled", 2012, 8, 30)); Nit: can you wrap this consistently with the one below?
Thanks... Addressed comments (except one which I answered) and also added a new test (forgot about it, thanks for the reminder)... PTAL... BYE MAD https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... base/metrics/field_trial.cc:35: // First check if the field trial has already been created in some other ways. On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Nit: "some other ways" -> "some other way" Done. https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... base/metrics/field_trial.cc:39: } else { On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Nit: Since you're early returning, don't need an else block. Done. https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/6001/base/metrics/field_t... base/metrics/field_trial.h:119: // Using this constructor creates a startup-randomized FieldTrial. If you On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Update this comment, since this is no longer a constructor. Also, please move > the last paragraph to the beginning of the comment since that introduces what > the function does. Done. https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.cc:42: name, total_probability, default_group_name, year, month, day_of_month); On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Nit: just return the new FieldTrial(...) directly. Done. https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.cc:119: if (forced_) { On 2012/03/23 18:41:55, Alexei Svitkine wrote: > This block needs a comment explaining why this code is doing this. Done. https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.cc:365: FieldTrial* field_trial(FieldTrialList::Find(name)); On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Nit: Can you change this to use = syntax, since it's a pointer? Done. https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.cc:377: field_trial->forced_ = true; On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Are you sure this is okay to do for all trials created this way? For example, > for trials created in the renderer, or for some of the other existing callers of > this function? Yes, it should be fine since this function essentially forces the group choice anyway... Even if it is already used elsewhere, the expected behavior is the same... Unless I'm missing something... https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.h:121: // after construction. On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Please also mention that this may returned an existing field trial, if it had > been forced. Done. https://chromiumcodereview.appspot.com/9705074/diff/8002/base/metrics/field_t... base/metrics/field_trial.h:254: // When forced_ is true, we return chosen group from AppendGroup when On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Nit: grammar - "we return chosen group" -> "we return the chosen group" Done. https://chromiumcodereview.appspot.com/9705074/diff/8002/chrome/browser/chrom... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/8002/chrome/browser/chrom... chrome/browser/chrome_browser_main.cc:621: // and stop the metrics service so that we don't pollute UMA. On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Nit: Split this into two sentences. Done. https://chromiumcodereview.appspot.com/9705074/diff/8002/chrome/browser/prere... File chrome/browser/prerender/prerender_field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/8002/chrome/browser/prere... chrome/browser/prerender/prerender_field_trial.cc:194: kOmniboxTrialName, kDivisor, "OmniboxPrerenderEnabled", 2012, 8, 30)); On 2012/03/23 18:41:55, Alexei Svitkine wrote: > Nit: can you wrap this consistently with the one below? Done.
Thanks! LGTM.
Thanks Alexei...
Now I need a OWNERS review from jar@ for stuff under
base\metrics
Where I changed the Field Trials so that we can force them from the command line
and make sure they still work as expected (except for the forced choice as
opposed to the coin toss).
For the rest, I added the following ldaps as TBR since I only changed the usage
of constructor to a new static CreateInstance method, and some comments in
content/public.
chrome\browser\ui\webui (sky)
chrome\browser\net (jar is part of these OWNERS too)
content\renderer (jamesr)
content\public (joi)
chrome\browser\component_updater (cpu)
https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.h:390: friend class base::FieldTrial; nit: No need for "base::"
The cl appears to break most all (revealed) uses of non-binary field trials. You significantly changed the semantics of the field trial constructor as you switched over to a factory, and are returning only two distinct group, breaking most common usages. I suggested a fix (below, in the comment), but it is not quite right. You really need to change a "force" to establishing a new default group. This way, you'll be able to return group numbers for any additional appends. You can't quite get away with what I suggested (below)... but it should at least make it clear where you were going wrong. https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.cc:125: return group_ + 1; This appears to be new semantics, and will potentially break some uses of code. The AppendGroup is supposed to return distinct group numbers for each call, but it appears that you only return two numbers. Some folks do tests such as: foo = trial.AppendGroup("foo", ...); goo = trial.AppendGroup("goo", ...); disable = trial.AppendGroup("disable", ...); ... if (trial.group() != disable) do_some_initialization(); IMO, the more more "standard" code, continuing with current semantics, might look like: if (name == group_name_) probability == divisor_; else probability == 0; You also have to be sure that lines 131 and 132 are in the "else" clause of "if (forced)" so that you continue to override their semantics. https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.cc:379: field_trial->forced_ = true; I would have expected this setting to only appear when you force the value from the command line. The code here suggests that you do it in all child processes as well. What was the intention? https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... File base/metrics/field_trial.h (left): https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.h:297: static void Register(FieldTrial* trial); Why were you psyched to make this private? It ended up meaning that you had to make a friend class... but that seems worse than having a method that is public that probably shouldn't be called by everyone. https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.h:390: friend class base::FieldTrial; Friendship is always discouraged... but I need to see why you opened up the class this way. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:680: } else if (connect_trial_group == connect_6) { All this code appears broken by the forcing function as implemented. This code assumes distinct groups. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:724: if (idle_to_trial_group == socket_timeout_5) { This code is broken by the new forcing function, as we assume we get distinct groups. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:768: if (proxy_connections_trial_group == proxy_connections_16) { This code is broken by this new CL forcing function, as it assumes distinct groups. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:810: if (trial_grp == npn_http_grp) { This code is also broken by this CL. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:917: trial_group == connect_backup_jobs_enabled); This is potentially broken by this CL. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:964: if (trial->group() != disabled_prefetch) { This code is broken by the CL. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/inst... File chrome/browser/instant/instant_field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/inst... chrome/browser/instant/instant_field_trial.cc:65: group == base::FieldTrial::kDefaultGroupNumber) { This code is broken by this CL. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/prer... File chrome/browser/prerender/prerender_field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:97: trial_group == kExperiment2Group) { This is broken by this CL. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:213: if (group == kOmniboxWeightFourGroup) Broken by this CL.
Thanks for the comments Jim... I don't think returning the same group value for non-chosen groups is a problem (more details below). But I don't mind changing it anyway, just in case some new FieldTrial code decides to do something different where this may be a problem (although I really don't see how)... It's easy to return different group numbers for every call to AppendGroup though, so might as well. But while I was trying to confirm that, I realized something worst. Something that is already broken in the render process (although hopefully not exercised yet). The problem is that when we force the default value, we don't set the chosen group id to be equal to the static value that consumers use to recognize it. I'll fix that by adding the default group name to the string we use to force trials, unless someone has a better solution. More details below. https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.cc:125: return group_ + 1; On 2012/03/23 23:03:01, jar wrote: > This appears to be new semantics, and will potentially break some uses of code. > The AppendGroup is supposed to return distinct group numbers for each call, but > it appears that you only return two numbers. Some folks do tests such as: > foo = trial.AppendGroup("foo", ...); > goo = trial.AppendGroup("goo", ...); > disable = trial.AppendGroup("disable", ...); > ... > if (trial.group() != disable) > do_some_initialization(); > > IMO, the more more "standard" code, continuing with current semantics, might > look like: > if (name == group_name_) > probability == divisor_; > else > probability == 0; > > You also have to be sure that lines 131 and 132 are in the "else" clause of "if > (forced)" so that you continue to override their semantics. > trial.group() would still be !=disable if a choice was forced to one of the other (not-disabled) groups. I don't see what would be the problem... If disable is forced, then the trial.group() != disable condition won't be met, as usual, so this is fine. If any of the other groups is chosen, then the trial.group() != disable condition will be met, which is what we want. I don't mind changing it anyway, just in case... But I don't think we should let the rest of the code in the AppendGroup method be executed. Most of it will be ignored by the "if (group_ == kNotFinalized ..." condition anyway... So I prefer to simply do something like this: if (forced_) { DCHECK(!group_name_.empty()); if (name == group_name_) { return group_; } else { DCHECK_NE(next_group_number_, group_); return next_group_number_++; } } https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.cc:379: field_trial->forced_ = true; On 2012/03/23 23:03:01, jar wrote: > I would have expected this setting to only appear when you force the value from > the command line. The code here suggests that you do it in all child processes > as well. What was the intention? Semantically, the group is also forced in child processes too. And for those that are forced from one process to another, AppendGroup won't be called anyway. Otherwise we would have seen the following DCHECK appear. DCHECK_LE(accumulated_group_probability_, divisor_); And actually, you made me realize that there currently is a problem with that... All field trials we push to the child processes that have the default group chosen won't be identified with the constant group id as some pieces of code you pointed me to do... This is bad... I'll need to fix this too... Thanks! https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... File base/metrics/field_trial.h (left): https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.h:297: static void Register(FieldTrial* trial); On 2012/03/23 23:03:01, jar wrote: > Why were you psyched to make this private? It ended up meaning that you had to > make a friend class... but that seems worse than having a method that is public > that probably shouldn't be called by everyone. I think it's bad that this is exposed, especially by the fact that it is called from the constructor of the FieldTrial... I thought it was OK to have the FieldTrial be friends with the FieldTrialList, since we already had the friendship in the other way, and these two classes are very close to each other... But I'm fine with removing the new friendship. Then, I'll move FieldTrial::CreateInstance to FieldTrialList::GetFieldTrialInstance (The Get prefix being more appropriate than Create since it's not always creating a new one). Then I need to move the call to Register out of the FieldTrial constructor and put it after the two places where we create a new FieldTrial, which can only be done from within FieldTrialList anyway, since the FieldTrial constructor is now private... https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.h:390: friend class base::FieldTrial; On 2012/03/23 23:03:01, jar wrote: > Friendship is always discouraged... but I need to see why you opened up the > class this way. Answered above... https://chromiumcodereview.appspot.com/9705074/diff/13001/base/metrics/field_... base/metrics/field_trial.h:390: friend class base::FieldTrial; On 2012/03/23 21:35:53, Ilya Sherman wrote: > nit: No need for "base::" Done. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:680: } else if (connect_trial_group == connect_6) { On 2012/03/23 23:03:01, jar wrote: > All this code appears broken by the forcing function as implemented. This code > assumes distinct groups. Actually the code doesn't expect any of the non-chosen group to be different from one another, all they need is to be different from the chosen group itself. If "IdleSktToImpact" is forced to the conn_count_5 gtoup, then idle_to_trial_group is equal to socket_timeout_5 and the proper branch will be executed. It doesn't really matter if socket_timeout_10 and socket_timeout_20 have the same value... But while making sure this was the case, I realized that if code like would run in the render process, it would fail to recognize the default group because it uses the hard coded kDefaultGroupNumber which will not be the one returned by the group() call, since FieldTrialList::CreateFieldTrial doesn't take the default group into account... I'll fix that in my subsequent upload. Thanks! https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:724: if (idle_to_trial_group == socket_timeout_5) { On 2012/03/23 23:03:01, jar wrote: > This code is broken by the new forcing function, as we assume we get distinct > groups. Again, wouldn't be broken if it wasn't for the usage of the kDefaultGroupNumber static value. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:768: if (proxy_connections_trial_group == proxy_connections_16) { On 2012/03/23 23:03:01, jar wrote: > This code is broken by this new CL forcing function, as it assumes distinct > groups. Done. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:810: if (trial_grp == npn_http_grp) { On 2012/03/23 23:03:01, jar wrote: > This code is also broken by this CL. Done. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:917: trial_group == connect_backup_jobs_enabled); On 2012/03/23 23:03:01, jar wrote: > This is potentially broken by this CL. Done. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:964: if (trial->group() != disabled_prefetch) { On 2012/03/23 23:03:01, jar wrote: > This code is broken by the CL. No it's not. It's not using kDefaultGroupNumber. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/inst... File chrome/browser/instant/instant_field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/inst... chrome/browser/instant/instant_field_trial.cc:65: group == base::FieldTrial::kDefaultGroupNumber) { On 2012/03/23 23:03:01, jar wrote: > This code is broken by this CL. Done. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/prer... File chrome/browser/prerender/prerender_field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:213: if (group == kOmniboxWeightFourGroup) On 2012/03/23 23:03:01, jar wrote: > Broken by this CL. Not using kDefaultGroupNumber. https://chromiumcodereview.appspot.com/9705074/diff/13001/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:244: group == base::FieldTrial::kDefaultGroupNumber; Although this here would be broken by this CL... But only in DEBUG mode with the command line argument choosing the default group.
Alexei, please take another look now that I made some changes to address Jim's comments... We'll then ask Jim to look at it again once we both agree on the changes... Thanks! BYE MAD
Some code comments below, but a general comment: I feel this makes things a bit cumbersome, especially for command-line users - where they now have to always know and pass the default group name. I wonder if we can make this better. What if we make a convention that the default group will always have the name "default"? We'd need to convert existing field trials to this and I've not checked what most of them look like... but that would make things much cleaner, IMHO. I guess we'd need to ask Jim what he thinks about that. https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.cc:347: while (next_item < trials_string.length()) { I wonder if it would be better to replace this whole thing with base::SplitString() on "/". Though I'm not going to insist that you do it in this CL... https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.cc:354: name_end + 1 == default_group_name_end) { Nit: put default_group_name_end on the left side of the == to be consistent with the previous line. https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.cc:360: default_group_name_end + 1 == group_name_end) { Nit: put group_name_end on the left side of the == to be consistent with the previous line. https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.cc:365: default_group_name_end - name_end - 1); Nit: Align this better. https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.cc:398: FieldTrialList::Register(field_trial); Nit: Call Register() at the bottom after the trial has been fully initialized. https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.cc:400: field_trial->AppendGroup(group_name, kTotalProbability); Can you add a DCHECKs that after calling AppendGroup(), the trial's group is not kNotFinalized and not kDefaultGroupNumber. https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.cc:402: // Force the choice of the default group Nit: add a . https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.h:270: const std::string& trial_name, const std::string& default_group_name, One param per line, please. https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.h:292: // name). |name| and |default_group_name| may not be empty. Nit: Use a single space between sentences here to be consistent with the rest of the comment. Also, either remove the |'s around |name| and |default_group_name| (And start the sentence with "Arguments name and default_group_name ..", or put |'s around the other ones in the next paragraph, for consistency. https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.h:366: const std::string& name, const std::string& default_group_name, One arg per line.
On Tue, Mar 27, 2012 at 10:29 AM, <asvitkine@chromium.org> wrote: > Some code comments below, but a general comment: > > I feel this makes things a bit cumbersome, especially for command-line > users - > where they now have to always know and pass the default group name. I > wonder if > we can make this better. > > What if we make a convention that the default group will always have the > name > "default"? We'd need to convert existing field trials to this and I've not > checked what most of them look like... but that would make things much > cleaner, > IMHO. I guess we'd need to ask Jim what he thinks about that. > Interesting idea... In fact, we could easily force it by having a specific static variable for that default group name and remove the argument from the constructor and factory to make sure that consumers of the interface don't use another name... The problem with existing FieldTrials is that it will change their histogram data because the MakeName method will now return a different string starting with this patch... I'm not sure we are ready for that... I personally don't think it's a big deal if the command line argument is more cumbersome... It's for developers testing their FieldTrials code, I don't think it matters that much if it's a bit more work to specify the default group name... But I could be convinced otherwise... I'll go fix your nits now.... Thanks! :-) > > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.cc<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.cc> > File base/metrics/field_trial.cc (right): > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.cc#newcode347<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.cc#newcode347> > base/metrics/field_trial.cc:**347: while (next_item < > trials_string.length()) { > I wonder if it would be better to replace this whole thing with > base::SplitString() on "/". Though I'm not going to insist that you do > it in this CL... > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.cc#newcode354<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.cc#newcode354> > base/metrics/field_trial.cc:**354: name_end + 1 == default_group_name_end) > { > Nit: put default_group_name_end on the left side of the == to be > consistent with the previous line. > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.cc#newcode360<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.cc#newcode360> > base/metrics/field_trial.cc:**360: default_group_name_end + 1 == > group_name_end) { > Nit: put group_name_end on the left side of the == to be consistent with > the previous line. > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.cc#newcode365<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.cc#newcode365> > base/metrics/field_trial.cc:**365: default_group_name_end - name_end - 1); > Nit: Align this better. > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.cc#newcode398<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.cc#newcode398> > base/metrics/field_trial.cc:**398: FieldTrialList::Register(** > field_trial); > Nit: Call Register() at the bottom after the trial has been fully > initialized. > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.cc#newcode400<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.cc#newcode400> > base/metrics/field_trial.cc:**400: field_trial->AppendGroup(**group_name, > kTotalProbability); > Can you add a DCHECKs that after calling AppendGroup(), the trial's > group is not kNotFinalized and not kDefaultGroupNumber. > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.cc#newcode402<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.cc#newcode402> > base/metrics/field_trial.cc:**402: // Force the choice of the default > group > Nit: add a . > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.h<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.h> > File base/metrics/field_trial.h (right): > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.h#newcode270<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.h#newcode270> > base/metrics/field_trial.h:**270: const std::string& trial_name, const > std::string& default_group_name, > One param per line, please. > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.h#newcode292<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.h#newcode292> > base/metrics/field_trial.h:**292: // name). |name| and > |default_group_name| may not be empty. > Nit: Use a single space between sentences here to be consistent with the > rest of the comment. > > Also, either remove the |'s around |name| and |default_group_name| (And > start the sentence with "Arguments name and default_group_name ..", or > put |'s around the other ones in the next paragraph, for consistency. > > https://chromiumcodereview.**appspot.com/9705074/diff/** > 24001/base/metrics/field_**trial.h#newcode366<https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_trial.h#newcode366> > base/metrics/field_trial.h:**366: const std::string& name, const > std::string& default_group_name, > One arg per line. > > https://chromiumcodereview.**appspot.com/9705074/<https://chromiumcodereview.... >
Please take another look... I changed the scheme of "specifying the default group name when forcing a choice" to "not always using a public static const value for the default group name anymore" instead... I first tried to isolate this change and do it in it's own CL, but it ended up overlapping with most of the changes in this current CL, so I decided to keep them together. Please let me know if you disagree or find anything wrong in this new CL... Thank! BYE MAD...
https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:98: if (group_name_ != default_group_name_) { Hmm. If group_name_ == default_group_name_, then we don't know whether it's at the default value or not. :/ So this effectively makes Disable() not work for forced trials. I am okay with this as long as its clear to callers. Document this in the header. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:148: // since we can't be forced and not finalized. Add a DCHECK(!forced_); https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:270: // If the default group was forced, it won't have kDefaultGroupNumber I don't think this comment is clear. How about: "If the field trial has already been forced, check whether it was forced to the default group. Return the chosen group number, in that case." https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:271: // as a group number, so we must return the chosen group number. Add a test for this logic. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:284: *default_group_number = FieldTrial::kDefaultGroupNumber; Nit: Move this to the top of the function. Then, in the "existing trial" block, you can just do: if (default_group_number && default_group_name == existing_trial->default_group_name()) { *default_group_number = existing_trial->group(); } https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:367: name_end + 1); Nit: Align this with kPersistentStringSeparator, so that there's no diff with the original. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:372: group_name_end - name_end - 1); Nit: Align this with trials_string, so that there's no diff with the original. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.h:143: std::string default_group_name() const { return default_group_name_; } Can you also remove this/make this private for the same reason as making kDefaultGroupNumber private? https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.h:291: // name). |name| and |default_group_name| may not be empty but Nit: Either remove the |'s around |name| and |default_group_name| (And start the sentence with "Arguments name and default_group_name .."), or put |'s around the other uses of variable names in this comment, for consistency. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.h:292: // default_group_number can be NULL if the value is not needed. Please explain what |default_group_number| is for, rather than just mentioning it can be NULL.
I think I addressed all comments... Please Take Another Look! Thanks! BYE MAD https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:98: if (group_name_ != default_group_name_) { On 2012/03/29 19:19:07, Alexei Svitkine wrote: > Hmm. If group_name_ == default_group_name_, then we don't know whether it's at > the default value or not. :/ > > So this effectively makes Disable() not work for forced trials. I am okay with > this as long as its clear to callers. Document this in the header. Filed a bug for this since it's already broken without this change (although just for the group name). And added a TODO comment in the header file. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:148: // since we can't be forced and not finalized. On 2012/03/29 19:19:07, Alexei Svitkine wrote: > Add a DCHECK(!forced_); Done. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:270: // If the default group was forced, it won't have kDefaultGroupNumber On 2012/03/29 19:19:07, Alexei Svitkine wrote: > I don't think this comment is clear. How about: > > "If the field trial has already been forced, check whether it was forced to the > default group. Return the chosen group number, in that case." Done. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:271: // as a group number, so we must return the chosen group number. On 2012/03/29 19:19:07, Alexei Svitkine wrote: > Add a test for this logic. I already added one, but it was missing one expectation to check we return the static value... I added it now... https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:284: *default_group_number = FieldTrial::kDefaultGroupNumber; On 2012/03/29 19:19:07, Alexei Svitkine wrote: > Nit: Move this to the top of the function. Then, in the "existing trial" block, > you can just do: > > if (default_group_number && > default_group_name == existing_trial->default_group_name()) { > *default_group_number = existing_trial->group(); > } Done. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:367: name_end + 1); On 2012/03/29 19:19:07, Alexei Svitkine wrote: > Nit: Align this with kPersistentStringSeparator, so that there's no diff with > the original. Done. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.cc:372: group_name_end - name_end - 1); On 2012/03/29 19:19:07, Alexei Svitkine wrote: > Nit: Align this with trials_string, so that there's no diff with the original. Done. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.h:143: std::string default_group_name() const { return default_group_name_; } On 2012/03/29 19:19:07, Alexei Svitkine wrote: > Can you also remove this/make this private for the same reason as making > kDefaultGroupNumber private? Good idea... Done! https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.h:291: // name). |name| and |default_group_name| may not be empty but On 2012/03/29 19:19:07, Alexei Svitkine wrote: > Nit: Either remove the |'s around |name| and |default_group_name| (And start > the sentence with "Arguments name and default_group_name .."), or put |'s around > the other uses of variable names in this comment, for consistency. Done. https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... base/metrics/field_trial.h:292: // default_group_number can be NULL if the value is not needed. On 2012/03/29 19:19:07, Alexei Svitkine wrote: > Please explain what |default_group_number| is for, rather than just mentioning > it can be NULL. Done.
On 2012/04/02 18:31:14, MAD wrote: > I think I addressed all comments... Please Take Another Look! > > Thanks! > > BYE > MAD > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > File base/metrics/field_trial.cc (right): > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.cc:98: if (group_name_ != default_group_name_) { > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > Hmm. If group_name_ == default_group_name_, then we don't know whether it's at > > the default value or not. :/ > > > > So this effectively makes Disable() not work for forced trials. I am okay with > > this as long as its clear to callers. Document this in the header. > > Filed a bug for this since it's already broken without this change (although > just for the group name). And added a TODO comment in the header file. > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.cc:148: // since we can't be forced and not finalized. > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > Add a DCHECK(!forced_); > > Done. > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.cc:270: // If the default group was forced, it won't > have kDefaultGroupNumber > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > I don't think this comment is clear. How about: > > > > "If the field trial has already been forced, check whether it was forced to > the > > default group. Return the chosen group number, in that case." > > Done. > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.cc:271: // as a group number, so we must return the > chosen group number. > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > Add a test for this logic. > > I already added one, but it was missing one expectation to check we return the > static value... I added it now... > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.cc:284: *default_group_number = > FieldTrial::kDefaultGroupNumber; > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > Nit: Move this to the top of the function. Then, in the "existing trial" > block, > > you can just do: > > > > if (default_group_number && > > default_group_name == existing_trial->default_group_name()) { > > *default_group_number = existing_trial->group(); > > } > > Done. > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.cc:367: name_end + 1); > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > Nit: Align this with kPersistentStringSeparator, so that there's no diff with > > the original. > > Done. > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.cc:372: group_name_end - name_end - 1); > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > Nit: Align this with trials_string, so that there's no diff with the original. > > Done. > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > File base/metrics/field_trial.h (right): > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.h:143: std::string default_group_name() const { return > default_group_name_; } > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > Can you also remove this/make this private for the same reason as making > > kDefaultGroupNumber private? > > Good idea... > Done! > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.h:291: // name). |name| and |default_group_name| may > not be empty but > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > Nit: Either remove the |'s around |name| and |default_group_name| (And start > > the sentence with "Arguments name and default_group_name .."), or put |'s > around > > the other uses of variable names in this comment, for consistency. > > Done. > > https://chromiumcodereview.appspot.com/9705074/diff/28002/base/metrics/field_... > base/metrics/field_trial.h:292: // default_group_number can be NULL if the value > is not needed. > On 2012/03/29 19:19:07, Alexei Svitkine wrote: > > Please explain what |default_group_number| is for, rather than just mentioning > > it can be NULL. > > Done. Oops... Wait... wrong version of the code got uploaded... I'll upload another patch ASAP...
OK, now this patch should be good... Sorry about that... BYE MAD
LGTM
Thanks Alexei... Jim, can you take another look from the OWNER's perspective? We had a lot of discussions about how to deal with the problem of not knowing the default group when a choice is forced by the FieldTrialList::CreateFieldTrial method (whether from the new command line or as it currently happen in a child/render process). My initial suggestion was to pass along the default group name as part of the state string, but that was making the command line a bit cumbersome and fragile (too easy to inverse chosen group and default group). We thought we could not specify the default at construction time and use AppendGroup for the default too so that we could return the default group number instead of using the static member, but then the FieldTrial objects would be in an invalid state (if they were not created by FieldTrialList::CreateFieldTrial) until AppendGroup was called at least once and we thought this wasn't a good idea. Then we also thought about the idea of adding a FieldTrial::GetDefaultGroupNumber() method, but then it wouldn't be able to return the right thing when FieldTrialList::CreateFieldTrial() is used to create a new FieldTrial, because there are no default group being set in this case. So we decided it would be simpler and safer to add an int* to the arguments of the factory method so that the default group number can be returned when needed. This doesn't resolve (and make it a bit worst) the existing problem (for which I filed http://crbug.com/121446) that calling FieldTrial::Disable on a trial that was created by FieldTrialList::CreateFieldTrial won't really reset to the default group since we don't know which group is the default one. We used to at least use the proper group (the static const member) but we didn't set the proper name (which used to be available from FieldTrial::default_group_name() which I have now made private). But we felt this wasn't a blocker for this change, since this was already broken for the default group name. Let us know what you think of all this... Thanks! BYE MAD
https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/24001/base/metrics/field_... base/metrics/field_trial.h:303: const std::string& name, FieldTrial::Probability total_probability, nit: Here too, one arg per line. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.cc:101: group_name_hash_ = HashName(group_name_); I'm starting to see this parallel initialization many times in this file (re: both group_name_ and group_name_hash_). I suspect you should have a accessor for group_name_ that also sets the hash, so that you don't mess up and fail to set it hear and there. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.cc:114: } else { nit: else not needed since we returned. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.cc:263: const std::string& name, FieldTrial::Probability total_probability, nit: one parameter per line. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.cc:271: DCHECK(existing_trial->forced_); This is tempting to make a CHECK. If a user tries to force a value too late... they'll never know that their parameter did nothing :-/. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.h:289: // it has to be because it was forced by CreateFieldTrial. nit: I had to read the sentence a few times, and stumbled over "...it has to be because it was..." IMO, the subtle distinction between "finding an existing instance" and "creates" should not be surfaced here, as that is an internal element of the implementation. For instance, a list of "forced" group-results could be stored separately, and then the construction (which might take place now!) could have adjusted to their planned/forced state. IMO, this CL just changes from using a constructor, to using a Factory, and the factory has more liberties about construction and reuse... but that is hidden inside the implementation. I'd suggest you use that metaphor, and you can leave your implementation hidden more cleanly. I'd suggest this function should probably be called something more like: FactoryGetFieldTrial() IF you don't like that picture, and want to stay with the current commentary (exposing the hidden fact that the instance may already be created) I'd suggest changing your wording to something like: Generally, this function should only be called once by a caller. If the field trial exists prior to this call, then it was probably created by a call to CreateFieldTrial, which forced a specific group result. Such pre-existence should be transparent to the caller. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.h:309: const std::string& name, FieldTrial::Probability total_probability, nit: I think it is more readable with one parameter per line once you had to wrap, and that might follow from the style rule: "All parameters should be aligned if possible" https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.h:310: const std::string& default_group_name, int* default_group_number, Since the return group number is a "return argument," it should be placed at the end of the arg list, after all the "input" arguments. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... File base/metrics/field_trial_unittest.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial_unittest.cc:518: int chosen_group = forced_trial->AppendGroup("Force", 100); For tests, you should call with the ordering and semantics that you expect this to be used with. As a result, I think you should: CreateFieldTrial() to get a result. Then do the standard version that should GetFieldTrialInstance(), and that should includ specifying probability divisor, default name, etc. etc. You are welcome to EXPECT that you got the same instance. Then do the append group, being careful not NOT exceed the divisor. https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:596: // Also stop the metrics service so that we don't pollute UMA. I'm not sure on the ordering, but disabling UMA is probably better done around line 1095. You might need to separate the setting of field trials, from the control of UMA as well. https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1095: // non-official builds. This is probably a better place to control (disable) UMA, since that is what this code is all about (so the flow is guaranteed to work as expected... I think). https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... File chrome/browser/prerender/prerender_field_trial.cc (left): https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:79: const int kControl1Group = This is the correct style. https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... File chrome/browser/prerender/prerender_field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:82: const int control_1_group = constant names should be a k followed by CamelCase. https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:250: group == omnibox_trial_default_group_number; Global variable should be g_omnibox_trial_default_group_number I was not completely clear on the initialization order, and would like to see: CHECK_NE(g_omnibox_trial_default_group_number, -1); to assure that we ran the field trial code before we got here. https://chromiumcodereview.appspot.com/9705074/diff/38003/content/public/comm... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/content/public/comm... content/public/common/content_switches.cc:372: // Some field tests may be randomized in the browser, and the randomly selected nit: "field tests" --> "field trials" https://chromiumcodereview.appspot.com/9705074/diff/38003/content/public/comm... content/public/common/content_switches.cc:380: const char kForceFieldTestNameAndValue[] = "force-fieldtest"; Is there a reason why you didn't call this something more like: force-fieldtrial Perchance I am the one that wrote this code :-(. In any case, it should probably be fieldtrial unless there is a compelling reason not to be such.
Thanks for the comments... Please take another look... BYE MAD https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... File base/metrics/field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.cc:101: group_name_hash_ = HashName(group_name_); On 2012/04/06 01:41:29, jar wrote: > I'm starting to see this parallel initialization many times in this file (re: > both group_name_ and group_name_hash_). I suspect you should have a accessor > for group_name_ that also sets the hash, so that you don't mess up and fail to > set it hear and there. Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.cc:114: } else { On 2012/04/06 01:41:29, jar wrote: > nit: else not needed since we returned. Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.cc:263: const std::string& name, FieldTrial::Probability total_probability, On 2012/04/06 01:41:29, jar wrote: > nit: one parameter per line. Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.cc:271: DCHECK(existing_trial->forced_); On 2012/04/06 01:41:29, jar wrote: > This is tempting to make a CHECK. > If a user tries to force a value too late... they'll never know that their > parameter did nothing :-/. Done. Though it doesn't change much currently since we only force trials from the command line in a DEBUG build, but it will protect us in the future... But this wouldn't catch a forcing done too late, this would happen in CreateFieldTrial below... Not here... And in that case we return NULL (or false if called indirectly from CreateTrialsFromString), so it shouldn't go unnoticed. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... File base/metrics/field_trial.h (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.h:289: // it has to be because it was forced by CreateFieldTrial. On 2012/04/06 01:41:29, jar wrote: > nit: I had to read the sentence a few times, and stumbled over "...it has to be > because it was..." > > IMO, the subtle distinction between "finding an existing instance" and "creates" > should not be surfaced here, as that is an internal element of the > implementation. For instance, a list of "forced" group-results could be stored > separately, and then the construction (which might take place now!) could have > adjusted to their planned/forced state. > > IMO, this CL just changes from using a constructor, to using a Factory, and the > factory has more liberties about construction and reuse... but that is hidden > inside the implementation. > > I'd suggest you use that metaphor, and you can leave your implementation hidden > more cleanly. I'd suggest this function should probably be called something > more like: > FactoryGetFieldTrial() > > IF you don't like that picture, and want to stay with the current commentary > (exposing the hidden fact that the instance may already be created) I'd suggest > changing your wording to something like: > Generally, this function should only be called once by a caller. If the field > trial exists prior to this call, then it was probably created by a call to > CreateFieldTrial, which forced a specific group result. Such pre-existence > should be transparent to the caller. > OK, I agree, I like this picture... Thanks! Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.h:309: const std::string& name, FieldTrial::Probability total_probability, On 2012/04/06 01:41:29, jar wrote: > nit: I think it is more readable with one parameter per line once you had to > wrap, and that might follow from the style rule: "All parameters should be > aligned if possible" Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial.h:310: const std::string& default_group_name, int* default_group_number, On 2012/04/06 01:41:29, jar wrote: > Since the return group number is a "return argument," it should be placed at the > end of the arg list, after all the "input" arguments. Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... File base/metrics/field_trial_unittest.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial_unittest.cc:518: int chosen_group = forced_trial->AppendGroup("Force", 100); On 2012/04/06 01:41:29, jar wrote: > For tests, you should call with the ordering and semantics that you expect this > to be used with. As a result, I think you should: > CreateFieldTrial() to get a result. > > Then do the standard version that should GetFieldTrialInstance(), and that > should includ specifying probability divisor, default name, etc. etc. > > You are welcome to EXPECT that you got the same instance. > > Then do the append group, being careful not NOT exceed the divisor. Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:596: // Also stop the metrics service so that we don't pollute UMA. On 2012/04/06 01:41:29, jar wrote: > I'm not sure on the ordering, but disabling UMA is probably better done around > line 1095. You might need to separate the setting of field trials, from the > control of UMA as well. Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1095: // non-official builds. On 2012/04/06 01:41:29, jar wrote: > This is probably a better place to control (disable) UMA, since that is what > this code is all about (so the flow is guaranteed to work as expected... I > think). Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... File chrome/browser/prerender/prerender_field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:82: const int control_1_group = On 2012/04/06 01:41:29, jar wrote: > constant names should be a k followed by CamelCase. Compile-time constant are to be named as you describe, but these are not compile time constants, they are dependent on the return value of AppendGroup. https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:250: group == omnibox_trial_default_group_number; On 2012/04/06 01:41:29, jar wrote: > Global variable should be g_omnibox_trial_default_group_number > > I was not completely clear on the initialization order, and would like to see: > CHECK_NE(g_omnibox_trial_default_group_number, -1); > to assure that we ran the field trial code before we got here. Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/content/public/comm... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/content/public/comm... content/public/common/content_switches.cc:372: // Some field tests may be randomized in the browser, and the randomly selected On 2012/04/06 01:41:29, jar wrote: > nit: "field tests" --> "field trials" Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/content/public/comm... content/public/common/content_switches.cc:380: const char kForceFieldTestNameAndValue[] = "force-fieldtest"; On 2012/04/06 01:41:29, jar wrote: > Is there a reason why you didn't call this something more like: > force-fieldtrial > > Perchance I am the one that wrote this code :-(. > > In any case, it should probably be fieldtrial unless there is a compelling > reason not to be such. > Done.
https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... File base/metrics/field_trial_unittest.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/base/metrics/field_... base/metrics/field_trial_unittest.cc:518: int chosen_group = forced_trial->AppendGroup("Force", 100); I didn't see the change I was expecting. Specifically, I'd expect a call at line 16 like: wanna_be_trial = FactoryGetFieldTrial("Use the", divisor, year, month, day, &default_group_number); and then perhaps: EXPECT_EQ(wanna_be_trial, forced_trial); On 2012/04/11 02:38:08, MAD wrote: > On 2012/04/06 01:41:29, jar wrote: > > For tests, you should call with the ordering and semantics that you expect > this > > to be used with. As a result, I think you should: > > CreateFieldTrial() to get a result. > > > > Then do the standard version that should GetFieldTrialInstance(), and that > > should includ specifying probability divisor, default name, etc. etc. > > > > You are welcome to EXPECT that you got the same instance. > > > > Then do the append group, being careful not NOT exceed the divisor. > > Done. https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... File chrome/browser/prerender/prerender_field_trial.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/38003/chrome/browser/prer... chrome/browser/prerender/prerender_field_trial.cc:82: const int control_1_group = OK... I can live with that. Thanks for the correction! On 2012/04/11 02:38:08, MAD wrote: > On 2012/04/06 01:41:29, jar wrote: > > constant names should be a k followed by CamelCase. > > Compile-time constant are to be named as you describe, but these are not compile > time constants, they are dependent on the return value of AppendGroup. https://chromiumcodereview.appspot.com/9705074/diff/48021/chrome/browser/net/... File chrome/browser/net/http_pipelining_compatibility_client.cc (right): https://chromiumcodereview.appspot.com/9705074/diff/48021/chrome/browser/net/... chrome/browser/net/http_pipelining_compatibility_client.cc:470: // After April 14, 2012, the trial will disable itself. Someone updated the end date to the 14th.... but you lost that in the merge. Line 472 should say 14 rather than 1.
Oupsss.... Sorry about that... Both fixed now, thanks for catching... BYE MAD
lgtm
On 2012/04/11 18:35:01, jar wrote: > lgtm Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/9705074/51025
Presubmit check for 9705074-51025 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/ui/webui/ntp
content/renderer
chrome/browser/component_updater
content/public
content/browser
Presubmit checks took 2.3s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/9705074/51025
chrome/browser/ui/webui/ntp LGTM (stamp for any presubmit issues you may have)
Try job failure for 9705074-51025 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/9705074/51025
These errors don't seem related to the changes... Hitting the CQ once more... :-/ On Wed, Apr 11, 2012 at 6:17 PM, <commit-bot@chromium.org> wrote: > Try job failure for 9705074-51025 (retry) on win_rel for step > "browser_tests". > It's a second try, previously, step "browser_tests" failed. > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=win_rel&**number=19950<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=19950> > > > http://codereview.chromium.**org/9705074/<http://codereview.chromium.org/9705... >
Try job failure for 9705074-51025 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/9705074/51025
Change committed as 131948 |
