Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
700953002:
Send all field trials from the browser. (Closed)
|
Created:
6 years, 1 month ago by Georges Khalil Modified:
5 years, 11 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, erikwright+watch_chromium.org, Ilya Sherman Base URL:
https://chromium.googlesource.com/chromium/src.git@finch4 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSend all field trials from the browser.
- Switching behavior of command line to send all field trials from the browser to the renderers.
- Added notification from renderer to browser whenever a trial is activated.
BUG=430924
Committed: https://crrev.com/a0a12d5a031878cee3e8532bc5817ad84a55cd87
Cr-Commit-Position: refs/heads/master@{#311554}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Responded to comments. #
Total comments: 14
Patch Set 3 : Rebased to trunk (ignore this patch). #Patch Set 4 : Responded to comments. #
Total comments: 6
Patch Set 5 : Responded to comments and fixed some minor issues. #
Total comments: 8
Patch Set 6 : Responded to comments and added a test. #
Total comments: 6
Patch Set 7 : Responded to comments. #Patch Set 8 : Added test StatesStringFormat. #
Total comments: 4
Patch Set 9 : Merging to ToT. #Patch Set 10 : Responded to comments. #Messages
Total messages: 49 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
georgesak@chromium.org changed reviewers: + asvitkine@chromium.org
PTAL. Note: This does not change the behavior for trials created after the renderer. Those are still only sent when activated (through IPC).
https://codereview.chromium.org/700953002/diff/60001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/700953002/diff/60001/base/metrics/field_trial... base/metrics/field_trial.cc:403: it != all_groups.end(); ++it) { Nit: We now support C++11, so this can be: for (const FieldTrial::AllGroup& group : all_groups) { Same for your loop in GetAllFieldTrialGroups(). https://codereview.chromium.org/700953002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/700953002/diff/60001/base/metrics/field_trial... base/metrics/field_trial.h:111: struct AllGroup { AllGroup is a weird name. How about FieldTrialState? https://codereview.chromium.org/700953002/diff/60001/base/metrics/field_trial... base/metrics/field_trial.h:443: static void GetAllFieldTrialGroups( Does this need to be public? https://codereview.chromium.org/700953002/diff/60001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/700953002/diff/60001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_render_message_filter.cc:369: const std::string& trial_name, const std::string& group_name) { Nit: If first param is on a new line, each param should be on a separate line.
Patchset #2 (id:80001) has been deleted
Patchset #3 (id:120001) has been deleted
PTAL. https://codereview.chromium.org/700953002/diff/60001/base/metrics/field_trial.cc File base/metrics/field_trial.cc (right): https://codereview.chromium.org/700953002/diff/60001/base/metrics/field_trial... base/metrics/field_trial.cc:403: it != all_groups.end(); ++it) { On 2014/11/05 16:17:13, Alexei Svitkine wrote: > Nit: We now support C++11, so this can be: > > for (const FieldTrial::AllGroup& group : all_groups) { > > Same for your loop in GetAllFieldTrialGroups(). Done. https://codereview.chromium.org/700953002/diff/60001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/700953002/diff/60001/base/metrics/field_trial... base/metrics/field_trial.h:443: static void GetAllFieldTrialGroups( On 2014/11/05 16:17:14, Alexei Svitkine wrote: > Does this need to be public? No, moved it to private.
Generally, looks good. Various nits, though. Please create a crbug for this and associate it. https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_tria... base/metrics/field_trial.cc:433: void FieldTrialList::GetAllFieldTrialGroups( Hmm, actually if this method is private, I suggest not to have it at all and just inline this into the AllStatesToString() implementation. i.e. that function can iterate directly over global_->registered_. https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_tria... base/metrics/field_trial.cc:440: for (const auto& it : global_->registered_) { Nit: I prefer to explicitly name the type for clarity and name it something other than it, e.g.: for (const FieldTrial& trial : global_->registered_) { https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_tria... base/metrics/field_trial.h:248: bool GetFieldTrialState(FieldTrialState* field_trial_state) const; Nit: Actually, just name this GetState(). https://codereview.chromium.org/700953002/diff/100001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/700953002/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_message_filter.cc:371: base::FieldTrialList::FindFullName(trial_name); Add a comment to explain this, please. https://codereview.chromium.org/700953002/diff/100001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/700953002/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_message_filter.h:121: void OnFieldTrialActivated(const std::string& trial_name, Nit: Maybe add a comment. https://codereview.chromium.org/700953002/diff/100001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.cc (right): https://codereview.chromium.org/700953002/diff/100001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.cc:302: // Report any field trials activations to the browser. Nit: I would make the comment say "// Listen for field trial activations to report them to the browser." https://codereview.chromium.org/700953002/diff/100001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.h (right): https://codereview.chromium.org/700953002/diff/100001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.h:57: const std::string& group_name) override; Nit: Leave an empty line before the start of fields.
Patchset #3 (id:140001) has been deleted
PTAL. https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_tria... base/metrics/field_trial.cc:433: void FieldTrialList::GetAllFieldTrialGroups( On 2014/11/05 22:50:53, Alexei Svitkine wrote: > Hmm, actually if this method is private, I suggest not to have it at all and > just inline this into the AllStatesToString() implementation. i.e. that function > can iterate directly over global_->registered_. Done. https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_tria... base/metrics/field_trial.cc:440: for (const auto& it : global_->registered_) { On 2014/11/05 22:50:53, Alexei Svitkine wrote: > Nit: I prefer to explicitly name the type for clarity and name it something > other than it, e.g.: > > for (const FieldTrial& trial : global_->registered_) { Type is std::pair<string, base::FieldTrial *> and caused indentation issue, hence the use of auto. https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/700953002/diff/100001/base/metrics/field_tria... base/metrics/field_trial.h:248: bool GetFieldTrialState(FieldTrialState* field_trial_state) const; On 2014/11/05 22:50:53, Alexei Svitkine wrote: > Nit: Actually, just name this GetState(). Done. https://codereview.chromium.org/700953002/diff/100001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/700953002/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_message_filter.cc:371: base::FieldTrialList::FindFullName(trial_name); On 2014/11/05 22:50:53, Alexei Svitkine wrote: > Add a comment to explain this, please. Done. https://codereview.chromium.org/700953002/diff/100001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/700953002/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_message_filter.h:121: void OnFieldTrialActivated(const std::string& trial_name, On 2014/11/05 22:50:53, Alexei Svitkine wrote: > Nit: Maybe add a comment. Done. https://codereview.chromium.org/700953002/diff/100001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.cc (right): https://codereview.chromium.org/700953002/diff/100001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.cc:302: // Report any field trials activations to the browser. On 2014/11/05 22:50:53, Alexei Svitkine wrote: > Nit: I would make the comment say "// Listen for field trial activations to > report them to the browser." Done. https://codereview.chromium.org/700953002/diff/100001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.h (right): https://codereview.chromium.org/700953002/diff/100001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.h:57: const std::string& group_name) override; On 2014/11/05 22:50:53, Alexei Svitkine wrote: > Nit: Leave an empty line before the start of fields. Done.
Patchset #3 (id:160001) has been deleted
The latest diff looks strange - looks like it's against the last patchset instead of trunk. Could you check what's wrong?
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
Patchset #3 (id:220001) has been deleted
Patchset #3 (id:240001) has been deleted
Patchset #3 (id:260001) has been deleted
On 2014/11/06 22:25:03, Alexei Svitkine wrote: > The latest diff looks strange - looks like it's against the last patchset > instead of trunk. Could you check what's wrong? Sorry about that, should be fixed now.
https://codereview.chromium.org/700953002/diff/300001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/700953002/diff/300001/base/metrics/field_tria... base/metrics/field_trial.cc:412: for (const FieldTrial::FieldTrialState& trial : all_groups) { Nit: My suggestion is actually to combine the two loops and remove the AllGroups typedef. https://codereview.chromium.org/700953002/diff/300001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/700953002/diff/300001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_message_filter.cc:371: // Activate trial by calling FinFullName. Nit: You have a typo in the comment. However, a better comment would mention the reason we're doing this. e.g. // Active the trial in the browser process to match its state in the renderer. https://codereview.chromium.org/700953002/diff/300001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.h (right): https://codereview.chromium.org/700953002/diff/300001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.h:56: void OnFieldTrialGroupFinalized(const std::string& trial_name, This is implementing base::FieldTrialList::Observer, so actually please put this in a separate section with the following comment above it: // base::FieldTrialList::Observer: I suggest put the section above the OnSetIsIncognitoProcess and separating it by newlines between above and below it.
PTAL. https://codereview.chromium.org/700953002/diff/300001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/700953002/diff/300001/base/metrics/field_tria... base/metrics/field_trial.cc:412: for (const FieldTrial::FieldTrialState& trial : all_groups) { On 2014/11/07 15:51:02, Alexei Svitkine wrote: > Nit: My suggestion is actually to combine the two loops and remove the AllGroups > typedef. Done. https://codereview.chromium.org/700953002/diff/300001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/700953002/diff/300001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_message_filter.cc:371: // Activate trial by calling FinFullName. On 2014/11/07 15:51:02, Alexei Svitkine wrote: > Nit: You have a typo in the comment. However, a better comment would mention the > reason we're doing this. > > e.g. > > // Active the trial in the browser process to match its state in the renderer. Done. https://codereview.chromium.org/700953002/diff/300001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.h (right): https://codereview.chromium.org/700953002/diff/300001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.h:56: void OnFieldTrialGroupFinalized(const std::string& trial_name, On 2014/11/07 15:51:02, Alexei Svitkine wrote: > This is implementing base::FieldTrialList::Observer, so actually please put this > in a separate section with the following comment above it: > > // base::FieldTrialList::Observer: > > I suggest put the section above the OnSetIsIncognitoProcess and separating it by > newlines between above and below it. Done.
Sorry, forgot to send these Friday. https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_tria... base/metrics/field_trial.cc:406: if (registered.second->GetState(&trial)) { Nit: Early return/continue is preferred. https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_tria... base/metrics/field_trial.h:118: typedef std::vector<FieldTrialState> AllGroups; This can be removed right? https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_tria... base/metrics/field_trial.h:483: FieldTrial::AllGroups* all_groups); This can be removed, right? https://codereview.chromium.org/700953002/diff/320001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.h (right): https://codereview.chromium.org/700953002/diff/320001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.h:48: // base::FieldTrialList::Observer implementation. Nit: New preferred wording for this comment is just: // base::FieldTrialList::Observer: Update the one above at the same time.
Patchset #6 (id:340001) has been deleted
PTAL. https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_tria... base/metrics/field_trial.cc:406: if (registered.second->GetState(&trial)) { On 2014/11/10 18:30:43, Alexei Svitkine wrote: > Nit: Early return/continue is preferred. Done. https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_tria... base/metrics/field_trial.h:118: typedef std::vector<FieldTrialState> AllGroups; On 2014/11/10 18:30:43, Alexei Svitkine wrote: > This can be removed right? Done. https://codereview.chromium.org/700953002/diff/320001/base/metrics/field_tria... base/metrics/field_trial.h:483: FieldTrial::AllGroups* all_groups); On 2014/11/10 18:30:43, Alexei Svitkine wrote: > This can be removed, right? Done. https://codereview.chromium.org/700953002/diff/320001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.h (right): https://codereview.chromium.org/700953002/diff/320001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.h:48: // base::FieldTrialList::Observer implementation. On 2014/11/10 18:30:43, Alexei Svitkine wrote: > Nit: New preferred wording for this comment is just: > > // base::FieldTrialList::Observer: > > Update the one above at the same time. Done.
LGTM % request for another test and one more comment https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... base/metrics/field_trial.h:109: // A triplet representing a Field Trial, its selected group and whether it's Nit: "FieldTrial" (one word) to refer to the name of the class, or "field trial" (not capitalized) to refer to the concept. https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... base/metrics/field_trial_unittest.cc:456: EXPECT_EQ("*Some name/Winner/*xxx/yyyy/zzz/default/", save_string); Could you also add a test that de-serializing the string results in the expected trials being enabled/disabled? For that, you can re-create a new FieldTrialList.
georgesak@chromium.org changed reviewers: + jhawkins@chromium.org, nasko@chromium.org
nasko@chromium.org: Please review changes in - chrome/common/render_messages.h - content/renderer/renderer_main.cc jhawkins@chromium.org: Please review changes in - chrome/browser/renderer_host/chrome_render_message_filter.* - chrome/renderer/chrome_render_process_observer.* https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... base/metrics/field_trial.h:109: // A triplet representing a Field Trial, its selected group and whether it's On 2014/11/10 19:28:47, Alexei Svitkine wrote: > Nit: "FieldTrial" (one word) to refer to the name of the class, or "field trial" > (not capitalized) to refer to the concept. Done. https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... base/metrics/field_trial_unittest.cc:456: EXPECT_EQ("*Some name/Winner/*xxx/yyyy/zzz/default/", save_string); On 2014/11/10 19:28:47, Alexei Svitkine wrote: > Could you also add a test that de-serializing the string results in the expected > trials being enabled/disabled? For that, you can re-create a new FieldTrialList. Already exists from a previous CL that landed, test is called CreateTrialsFromStringForceActivation.
https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... base/metrics/field_trial_unittest.cc:456: EXPECT_EQ("*Some name/Winner/*xxx/yyyy/zzz/default/", save_string); On 2014/11/10 20:50:34, Georges Khalil wrote: > On 2014/11/10 19:28:47, Alexei Svitkine wrote: > > Could you also add a test that de-serializing the string results in the > expected > > trials being enabled/disabled? For that, you can re-create a new > FieldTrialList. > > Already exists from a previous CL that landed, test is called > CreateTrialsFromStringForceActivation. That doesn't test that the two formats match though. If you change the format of AllStatesToString() output and update this test, then the CreateTrialsFromStringForceActivation test won't break even though that function now won't be able to parse the output of AllStatesToString().
chrome/common/render_messages.h and content/renderer/renderer_main.cc LGTM
Patchset #8 (id:400001) has been deleted
https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... File base/metrics/field_trial_unittest.cc (right): https://codereview.chromium.org/700953002/diff/360001/base/metrics/field_tria... base/metrics/field_trial_unittest.cc:456: EXPECT_EQ("*Some name/Winner/*xxx/yyyy/zzz/default/", save_string); On 2014/11/10 20:52:35, Alexei Svitkine wrote: > On 2014/11/10 20:50:34, Georges Khalil wrote: > > On 2014/11/10 19:28:47, Alexei Svitkine wrote: > > > Could you also add a test that de-serializing the string results in the > > expected > > > trials being enabled/disabled? For that, you can re-create a new > > FieldTrialList. > > > > Already exists from a previous CL that landed, test is called > > CreateTrialsFromStringForceActivation. > > That doesn't test that the two formats match though. If you change the format of > AllStatesToString() output and update this test, then the > CreateTrialsFromStringForceActivation test won't break even though that function > now won't be able to parse the output of AllStatesToString(). New test added (StatesStringFormat).
lgtm
On 2014/11/10 22:37:21, Alexei Svitkine wrote: > lgtm @jhawkings: friendly ping.
https://codereview.chromium.org/700953002/diff/420001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/700953002/diff/420001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_message_filter.cc:371: base::FieldTrialList::FindFullName(trial_name); Is this method just poorly named? It's not clear (without your comment) that there are side effects, let alone something so heavy as activating a trial. https://codereview.chromium.org/700953002/diff/420001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.h (left): https://codereview.chromium.org/700953002/diff/420001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.h:41: // RenderProcessObserver implementation. Please don't change the existing style; added code should match existing style.
Patchset #9 (id:440001) has been deleted
@jhawkings: PTAnL. https://codereview.chromium.org/700953002/diff/420001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/700953002/diff/420001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_message_filter.cc:371: base::FieldTrialList::FindFullName(trial_name); On 2015/01/05 21:05:05, James Hawkins wrote: > Is this method just poorly named? It's not clear (without your comment) that > there are side effects, let alone something so heavy as activating a trial. I modified the comment to make the intent clearer. That's how the field trial system works, querying a trial's state will activate it (it's documented in the header file for the API). https://codereview.chromium.org/700953002/diff/420001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_process_observer.h (left): https://codereview.chromium.org/700953002/diff/420001/chrome/renderer/chrome_... chrome/renderer/chrome_render_process_observer.h:41: // RenderProcessObserver implementation. On 2015/01/05 21:05:05, James Hawkins wrote: > Please don't change the existing style; added code should match existing style. Done.
On 2015/01/05 21:28:58, Georges Khalil wrote: > @jhawkings: PTAnL. > > https://codereview.chromium.org/700953002/diff/420001/chrome/browser/renderer... > File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): > > https://codereview.chromium.org/700953002/diff/420001/chrome/browser/renderer... > chrome/browser/renderer_host/chrome_render_message_filter.cc:371: > base::FieldTrialList::FindFullName(trial_name); > On 2015/01/05 21:05:05, James Hawkins wrote: > > Is this method just poorly named? It's not clear (without your comment) that > > there are side effects, let alone something so heavy as activating a trial. > > I modified the comment to make the intent clearer. > > That's how the field trial system works, querying a trial's state will activate > it (it's documented in the header file for the API). OK, then let's get that changed (I'm not sure who owns that system, but anyone can change it). > > https://codereview.chromium.org/700953002/diff/420001/chrome/renderer/chrome_... > File chrome/renderer/chrome_render_process_observer.h (left): > > https://codereview.chromium.org/700953002/diff/420001/chrome/renderer/chrome_... > chrome/renderer/chrome_render_process_observer.h:41: // RenderProcessObserver > implementation. > On 2015/01/05 21:05:05, James Hawkins wrote: > > Please don't change the existing style; added code should match existing > style. > > Done.
On 2015/01/05 23:29:38, James Hawkins wrote: > OK, then let's get that changed (I'm not sure who owns that system, but anyone > can change it). I own it. I'm not sure what you're suggesting to change. It's by design that the first time a field trial's state is queried (i.e. it's used) marks it as active. This way, only active trials are reported to things like UMA and crash reporting. A trial that's never used would not be reported. This is how it's supposed to work and shouldn't be changed. I suppose we can add a method called ActivateTrial(trial_name) or something similar which would just be an alias for FindFullName(), if you really prefer. Is that what you're suggesting?
On 2015/01/06 14:53:12, Alexei Svitkine wrote: > On 2015/01/05 23:29:38, James Hawkins wrote: > > OK, then let's get that changed (I'm not sure who owns that system, but anyone > > can change it). > > I own it. I'm not sure what you're suggesting to change. > > It's by design that the first time a field trial's state is queried (i.e. it's > used) marks it as active. This way, only active trials are reported to things > like UMA and crash reporting. A trial that's never used would not be reported. > This is how it's supposed to work and shouldn't be changed. > > I suppose we can add a method called ActivateTrial(trial_name) or something > similar which would just be an alias for FindFullName(), if you really prefer. > Is that what you're suggesting? @jhawkings: friendly ping, what do you think?
On 2015/01/12 19:56:05, Georges Khalil wrote: > On 2015/01/06 14:53:12, Alexei Svitkine wrote: > > On 2015/01/05 23:29:38, James Hawkins wrote: > > > OK, then let's get that changed (I'm not sure who owns that system, but > anyone > > > can change it). > > > > I own it. I'm not sure what you're suggesting to change. > > > > It's by design that the first time a field trial's state is queried (i.e. it's > > used) marks it as active. This way, only active trials are reported to things > > like UMA and crash reporting. A trial that's never used would not be reported. > > This is how it's supposed to work and shouldn't be changed. > > > > I suppose we can add a method called ActivateTrial(trial_name) or something > > similar which would just be an alias for FindFullName(), if you really prefer. > > Is that what you're suggesting? > > @jhawkings: friendly ping, what do you think? Sorry for the delay. Renaming FindFullName to FindAndActivateFullName is an OK compromise, though I don't think I understand the API well enough to reason why querying and activating can't be two separate, explicit operations.
On 2015/01/14 19:31:42, James Hawkins wrote: > On 2015/01/12 19:56:05, Georges Khalil wrote: > > On 2015/01/06 14:53:12, Alexei Svitkine wrote: > > > On 2015/01/05 23:29:38, James Hawkins wrote: > > > > OK, then let's get that changed (I'm not sure who owns that system, but > > anyone > > > > can change it). > > > > > > I own it. I'm not sure what you're suggesting to change. > > > > > > It's by design that the first time a field trial's state is queried (i.e. > it's > > > used) marks it as active. This way, only active trials are reported to > things > > > like UMA and crash reporting. A trial that's never used would not be > reported. > > > This is how it's supposed to work and shouldn't be changed. > > > > > > I suppose we can add a method called ActivateTrial(trial_name) or something > > > similar which would just be an alias for FindFullName(), if you really > prefer. > > > Is that what you're suggesting? > > > > @jhawkings: friendly ping, what do you think? > > Sorry for the delay. > > Renaming FindFullName to FindAndActivateFullName is an OK compromise, though I > don't think I understand the API well enough to reason why querying and > activating can't be two separate, explicit operations. And if you're OK with the rename, I'm OK with it being done in a separate CL.
On 2015/01/14 19:32:27, James Hawkins wrote: > On 2015/01/14 19:31:42, James Hawkins wrote: > > On 2015/01/12 19:56:05, Georges Khalil wrote: > > > On 2015/01/06 14:53:12, Alexei Svitkine wrote: > > > > On 2015/01/05 23:29:38, James Hawkins wrote: > > > > > OK, then let's get that changed (I'm not sure who owns that system, but > > > anyone > > > > > can change it). > > > > > > > > I own it. I'm not sure what you're suggesting to change. > > > > > > > > It's by design that the first time a field trial's state is queried (i.e. > > it's > > > > used) marks it as active. This way, only active trials are reported to > > things > > > > like UMA and crash reporting. A trial that's never used would not be > > reported. > > > > This is how it's supposed to work and shouldn't be changed. > > > > > > > > I suppose we can add a method called ActivateTrial(trial_name) or > something > > > > similar which would just be an alias for FindFullName(), if you really > > prefer. > > > > Is that what you're suggesting? > > > > > > @jhawkings: friendly ping, what do you think? > > > > Sorry for the delay. > > > > Renaming FindFullName to FindAndActivateFullName is an OK compromise, though I > > don't think I understand the API well enough to reason why querying and > > activating can't be two separate, explicit operations. > > And if you're OK with the rename, I'm OK with it being done in a separate CL. Alright, I filed a bug for which we can do a separate CL. As for the rest, are you OK with the CL as-is?
(I suggested offline to Georges to file a bug and cc isherman@ so we can discuss the names.) Note: that this isn't the only API that activates field trials. For example, you can currently do FieldTrialList::Find(name)->group() which also activates it. If we're renaming things, we should do it in a way that makes things clear between the different APIs and so we should discuss on the bug. On Wed, Jan 14, 2015 at 2:55 PM, <georgesak@chromium.org> wrote: > On 2015/01/14 19:32:27, James Hawkins wrote: > >> On 2015/01/14 19:31:42, James Hawkins wrote: >> > On 2015/01/12 19:56:05, Georges Khalil wrote: >> > > On 2015/01/06 14:53:12, Alexei Svitkine wrote: >> > > > On 2015/01/05 23:29:38, James Hawkins wrote: >> > > > > OK, then let's get that changed (I'm not sure who owns that >> system, >> > but > >> > > anyone >> > > > > can change it). >> > > > >> > > > I own it. I'm not sure what you're suggesting to change. >> > > > >> > > > It's by design that the first time a field trial's state is queried >> > (i.e. > >> > it's >> > > > used) marks it as active. This way, only active trials are reported >> to >> > things >> > > > like UMA and crash reporting. A trial that's never used would not be >> > reported. >> > > > This is how it's supposed to work and shouldn't be changed. >> > > > >> > > > I suppose we can add a method called ActivateTrial(trial_name) or >> something >> > > > similar which would just be an alias for FindFullName(), if you >> really >> > prefer. >> > > > Is that what you're suggesting? >> > > >> > > @jhawkings: friendly ping, what do you think? >> > >> > Sorry for the delay. >> > >> > Renaming FindFullName to FindAndActivateFullName is an OK compromise, >> though >> > I > >> > don't think I understand the API well enough to reason why querying and >> > activating can't be two separate, explicit operations. >> > > And if you're OK with the rename, I'm OK with it being done in a separate >> CL. >> > > Alright, I filed a bug for which we can do a separate CL. > > As for the rest, are you OK with the CL as-is? > > https://codereview.chromium.org/700953002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by georgesak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/700953002/480001
Message was sent while issue was closed.
Committed patchset #10 (id:480001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a0a12d5a031878cee3e8532bc5817ad84a55cd87 Cr-Commit-Position: refs/heads/master@{#311554} |
