Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10381021:
Add filtering logic for client-side variations. (Closed)
|
Created:
8 years, 7 months ago by Alexei Svitkine (slow) Modified:
8 years, 7 months ago CC:
chromium-reviews, MAD, Ilya Sherman, jar (doing other things), finch-team_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd filtering logic for client-side variations.
BUG=126512
TEST=variation_service_unittest.cc
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135757
Patch Set 1 : - #
Total comments: 18
Patch Set 2 : rebased #Patch Set 3 : address comments #
Total comments: 2
Patch Set 4 : address final nits #
Total comments: 4
Messages
Total messages: 14 (0 generated)
Here are my comments... Good stuff! BYE MAD... http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:40: } DCHECK on default please And didn't you get: warning C4715: 'ConvertStudyChannelToVersionChannel' : not all control paths return a value ??? http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:129: if (!CheckStudyDate(study, base::GetBuildTime())) Please add a comment explaining why we use build time, e.g., to do as the FieldTrials do, and avoid relying on system time that users might change. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:132: return true; I wonder if we should add DVLOG(1)s when we filter out an experiment. What do you think? http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:151: DCHECK(false); We try not to DCHECK AND handle the error usually... Unless we have a very good reason to. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service_unittest.cc (right): http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:38: // All channels up to, but not including |channels[i]| have been added. I would also do it backwards, so that we check that having later channels yet not ours also work... http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:44: study.add_channel(study_channels[i]); Is it me or you won't be checking the last channel? http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:96: // Check intersection semantics. Excellent! :-) http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:102: if (!min_test_cases[i].expected_result) { Why not use the same scheme as below for the dates? http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:119: // The current client logic does not version numbers containing wildcards. [...]does not "handle" version[...]?
Thanks for the review! I'll likely address the code changes Monday morning, but here are some initial replies. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:40: } On 2012/05/05 13:51:17, MAD wrote: > DCHECK on default please > > And didn't you get: > warning C4715: 'ConvertStudyChannelToVersionChannel' : not all control paths > return a value > ??? It's fine with clang on the Mac. I believe clang knows that all possible |study_channel| enum values are handled so: a) the function will always reach a return and b) the default case is not necessarily. I think there also exists a warning where you don't handle some enum values in a switch that doesn't have a default case. I'm not sure if we build with that warning enabled, though. But you're right that this may not compile on Windows or possibly even with gcc - I haven't tested since this patch depends on Jesse's patch and I'm not sure there's a way to run try bots on this before Jesse's patch has landed. Is there? Regardless, it's probably a good a idea to add DCHECK and return below, for portability with compilers that aren't as smart as clang. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:151: DCHECK(false); On 2012/05/05 13:51:17, MAD wrote: > We try not to DCHECK AND handle the error usually... Unless we have a very good > reason to. I copied this from elsewhere. I think the reasoning is that the version string passed in the parameter (which comes from Chrome's version and not from the server) should always be valid. I think that's a reasonable thing to DCHECK() for. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service_unittest.cc (right): http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:102: if (!min_test_cases[i].expected_result) { On 2012/05/05 13:51:17, MAD wrote: > Why not use the same scheme as below for the dates? That one only works if the input values are the same. Here, the test cases specify the input values to compare, so that it's not always the case that min_test_cases[i].version == max_test_cases[j].version for some i,j pair. Hence the test logic can not rely on the expected_results from both sets, since they correspond to different inputs. It can say, however, that "if that test is expected to fail with the given min version, then it should still fail if a max version was added to the mix" and vice versa, which is exactly what it's doing. I'll add some comments.
All comments addressed, please PTAL. Thanks! http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:40: } On 2012/05/05 15:14:37, Alexei Svitkine wrote: > On 2012/05/05 13:51:17, MAD wrote: > > DCHECK on default please > > > > And didn't you get: > > warning C4715: 'ConvertStudyChannelToVersionChannel' : not all control paths > > return a value > > ??? > Regardless, it's probably a good a idea to add DCHECK and return below, for > portability with compilers that aren't as smart as clang. Done. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:129: if (!CheckStudyDate(study, base::GetBuildTime())) On 2012/05/05 13:51:17, MAD wrote: > Please add a comment explaining why we use build time, e.g., to do as the > FieldTrials do, and avoid relying on system time that users might change. Done. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:132: return true; On 2012/05/05 13:51:17, MAD wrote: > I wonder if we should add DVLOG(1)s when we filter out an experiment. What do > you think? Done. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service_unittest.cc (right): http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:38: // All channels up to, but not including |channels[i]| have been added. On 2012/05/05 13:51:17, MAD wrote: > I would also do it backwards, so that we check that having later channels yet > not ours also work... Done. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:44: study.add_channel(study_channels[i]); On 2012/05/05 13:51:17, MAD wrote: > Is it me or you won't be checking the last channel? You're right. Fixed. http://codereview.chromium.org/10381021/diff/3001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:119: // The current client logic does not version numbers containing wildcards. On 2012/05/05 13:51:17, MAD wrote: > [...]does not "handle" version[...]? Done.
Hey, is the chrome_variations::Study stuff (study.pb.h) introduced for the first time here? If so, be prepared to do through the same static initializer funtimes as Jesse and I (http://code.google.com/p/chromium/issues/detail?id=105626).
On 2012/05/07 16:34:52, SteveT wrote: > Hey, is the chrome_variations::Study stuff (study.pb.h) introduced for the first > time here? If so, be prepared to do through the same static initializer funtimes > as Jesse and I (http://code.google.com/p/chromium/issues/detail?id=105626). I don't think it would be needed - since the existing code already includes trial_list.pb.h which pulls in study.pb.h. (And I assume that the ones included in variations_service_unittest.cc don't get counted since it's in a test binary.)
Most excellent. On Mon, May 7, 2012 at 12:44 PM, <asvitkine@chromium.org> wrote: > On 2012/05/07 16:34:52, SteveT wrote: > >> Hey, is the chrome_variations::Study stuff (study.pb.h) introduced for the >> > first > >> time here? If so, be prepared to do through the same static initializer >> > funtimes > >> as Jesse and I (http://code.google.com/p/**chromium/issues/detail?id=** >> 105626 <http://code.google.com/p/chromium/issues/detail?id=105626>). >> > > I don't think it would be needed - since the existing code already includes > trial_list.pb.h which pulls in study.pb.h. (And I assume that the ones > included > in variations_service_unittest.cc don't get counted since it's in a test > binary.) > > http://codereview.chromium.**org/10381021/<http://codereview.chromium.org/103... >
LGTM with two small nits... Thanks for the fixes! BYE MAD... http://codereview.chromium.org/10381021/diff/9001/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service_unittest.cc (right): http://codereview.chromium.org/10381021/diff/9001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:38: // Check in the forwarded order. |num_cases| is 1 greater than the number of |num_cases|? http://codereview.chromium.org/10381021/diff/9001/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:48: if (i != arraysize(study_channels)) I would prefer < instead of !=, in case we eventually evolve to go beyond size + 1...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10381021/3004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10381021/3004
Couple of drive-by comments. I'm hoping these won't somehow confuse the CQ and cancel the commit... I'm perfectly happy for these to be addressed, if need be, in a follow-up CL rather than this one. http://codereview.chromium.org/10381021/diff/3004/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10381021/diff/3004/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:136: if (!CheckStudyDate(study, base::GetBuildTime())) { Hmm, this seems redundant with the version check. Am I missing something, or is this intentionally meant to be a convenient way to filter based on future versions according to their release date? http://codereview.chromium.org/10381021/diff/3004/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:161: DCHECK(false); nit: NOTREACHED()
Change committed as 135757
http://codereview.chromium.org/10381021/diff/3004/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10381021/diff/3004/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:136: if (!CheckStudyDate(study, base::GetBuildTime())) { On 2012/05/07 22:49:04, Ilya Sherman wrote: > Hmm, this seems redundant with the version check. Am I missing something, or is > this intentionally meant to be a convenient way to filter based on future > versions according to their release date? I agree, it's a bit strange. I think originally, the intention was to be able to limit the trial to e.g. a period of a specific week, for example. But in the end, we decided to use build time (at least for this original CL), so that it's consistent with the field trials code expiry param. I agree that this makes the original intent dubious since it's based on build time and not system time. We could discuss it in our weekly.
http://codereview.chromium.org/10381021/diff/3004/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10381021/diff/3004/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:136: if (!CheckStudyDate(study, base::GetBuildTime())) { On 2012/05/08 14:43:25, Alexei Svitkine wrote: > On 2012/05/07 22:49:04, Ilya Sherman wrote: > > Hmm, this seems redundant with the version check. Am I missing something, or > is > > this intentionally meant to be a convenient way to filter based on future > > versions according to their release date? > > I agree, it's a bit strange. I think originally, the intention was to be able to > limit the trial to e.g. a period of a specific week, for example. > > But in the end, we decided to use build time (at least for this original CL), so > that it's consistent with the field trials code expiry param. I agree that this > makes the original intent dubious since it's based on build time and not system > time. > > We could discuss it in our weekly. That sounds good. I think it would also be helpful to document the current decision in the corresponding protocol buffer comments, so that it's less likely anyone adding a server-side configuration would be surprised by this... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
