Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12220004:
Introduce dynamic ablation of AutocompleteProviders using AC dynamic trials. (Closed)
|
Created:
7 years, 10 months ago by Bart N. Modified:
7 years, 10 months ago CC:
chromium-reviews, James Su Visibility:
Public. |
DescriptionIntroduce dynamic ablation of AutocompleteProviders using AC dynamic trials.
The idea is simple. Use Autocomplete dynamic field trial group names to indicate which providers should be disabled. This information, due to limitations in field trial design, is passed in the group name in a string form of "DisabledProviders_<bitmask>".
For instance, in order to run a SHORTCUTS ablation, one need to create a field trial with a group name that contains "DisabledProviders_512" (1<<9).
BUG=174694
TBR=sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182018
Patch Set 1 #Patch Set 2 : Cleanup + unit tests. #Patch Set 3 : #
Total comments: 14
Patch Set 4 : #
Total comments: 5
Patch Set 5 : #Patch Set 6 : Rebase. #Patch Set 7 : Remove unused variable. #
Messages
Total messages: 30 (0 generated)
Mark, as discussed offline, this is ready for review.
Is there a bug for this? The use of numbers to encode providers seems unfortunate. As of yet the fact that we use an enum (and thus that providers are effectively numbered) has stayed an implementation detail. I worry about things like making it harder to add/remove providers or reorder the list without worrying about field trial effects. Would it be possible to instead support some one-string-per-disabled-provider syntax? Then we could refer to providers by name, which would be more readable and also more flexible. If we can't use separate strings, perhaps delimiter characters (an underscore?) between each provider name?
On 2013/02/06 02:16:37, Peter Kasting wrote: > Is there a bug for this? No external bug. > > The use of numbers to encode providers seems unfortunate. As of yet the fact > that we use an enum (and thus that providers are effectively numbered) has > stayed an implementation detail. I worry about things like making it harder to > add/remove providers or reorder the list without worrying about field trial > effects. We don't plan to have this code forever; in fact, at most for 1-2 releases. This is also intended to be used internally (dynamic field trials). > > Would it be possible to instead support some one-string-per-disabled-provider > syntax? Then we could refer to providers by name, which would be more readable > and also more flexible. If we can't use separate strings, perhaps delimiter > characters (an underscore?) between each provider name? Honestly, this is not needed. This code will go away as soon as we have results we need. I'd be more willing to follow your approach if there was a way to map a string onto ID (e.g. if the enum was defined in a proto), but apparently this is not the case.
OK, if this is temporary, I guess I don't care as much.
On 2013/02/06 02:39:43, Bart N. wrote: > On 2013/02/06 02:16:37, Peter Kasting wrote: > > Is there a bug for this? > No external bug. Any reason why not? We try hard to enforce that all substantive changes have an associated bug, which hopefully gives more background on what we're trying to achieve.
On 2013/02/06 18:54:41, Peter Kasting wrote: > On 2013/02/06 02:39:43, Bart N. wrote: > > On 2013/02/06 02:16:37, Peter Kasting wrote: > > > Is there a bug for this? > > No external bug. > > Any reason why not? We try hard to enforce that all substantive changes have an > associated bug, which hopefully gives more background on what we're trying to > achieve. Done. I wish it was enforced consistently for each CL.
Friendly ping? I'm not sure who is going to review this, Mark or Peter?
On 2013/02/11 17:53:10, Bart N. wrote: > Friendly ping? I'm not sure who is going to review this, Mark or Peter? I don't know; you listed two reviewers and didn't say what you wanted each to look at. You called out Mark in your first message so I vaguely assumed he was reviewing.
On 2013/02/11 20:03:00, Peter Kasting wrote: > On 2013/02/11 17:53:10, Bart N. wrote: > > Friendly ping? I'm not sure who is going to review this, Mark or Peter? > > I don't know; you listed two reviewers and didn't say what you wanted each to > look at. You called out Mark in your first message so I vaguely assumed he was > reviewing. Mark before leaving OOO mentioned you would be on it, but apparently it wasn't the case. How about Mark finishing this review, since he pre-approved this idea before I even sent out for official review, and you Peter just do optional pass if you see something worth commenting on? Peter, my understanding is that you have no further objections regarding the bitmap mask part, as discussed above, right?
Yeah, you're good to go.
Generally fine. I'm worried about the unit_test corrupting the global field trial state and messing up other unit tests. I don't think it's acceptable as it is. If (for timing reasons) you want to check in this changelist without it and add it later, that's fine with me; I'll approve that. --mark https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:131: std::string DynamicFieldTrialName(int id) { please comment about expected use. https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:268: size_t pos = group_name.find(kDisabledProviders); const https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:271: int types = 0; do you expect pos to ever not be 0? perhaps dcheck this https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:272: base::StringToInt(base::StringPiece( consider dchecking the return type of this https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.h (right): https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.h:32: // all found bitmap masks OR-ed together. "all found". ick. please rephrase
https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:131: std::string DynamicFieldTrialName(int id) { On 2013/02/11 21:13:34, Mark P wrote: > please comment about expected use. Done. https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:268: size_t pos = group_name.find(kDisabledProviders); On 2013/02/11 21:13:34, Mark P wrote: > const Done. https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:271: int types = 0; On 2013/02/11 21:13:34, Mark P wrote: > do you expect pos to ever not be 0? > perhaps dcheck this I don't think it's a good idea to DCHECK against something that is not entirely in your control. In this case, if someone makes a mistake on the server side, we would start DCHECKing in Chrome. Instead, I've added a warning message. Is that OK with you? https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:272: base::StringToInt(base::StringPiece( On 2013/02/11 21:13:34, Mark P wrote: > consider dchecking the return type of this Not really; clarified in the comment. https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.h (right): https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.h:32: // all found bitmap masks OR-ed together. On 2013/02/11 21:13:34, Mark P wrote: > "all found". ick. > please rephrase Ooops. Fixed.
> I'm worried about the unit_test corrupting the global field trial state and > messing up other unit tests. I don't think it's acceptable as it is. If (for > timing reasons) you want to check in this changelist without it and add it > later, that's fine with me; I'll approve that. I think this is no longer the case. If we run into this again, we can refactor ACFT class.
On 2013/02/11 21:44:13, Bart N. wrote: > > I'm worried about the unit_test corrupting the global field trial state and > > messing up other unit tests. I don't think it's acceptable as it is. If (for > > timing reasons) you want to check in this changelist without it and add it > > later, that's fine with me; I'll approve that. > I think this is no longer the case. I don't understand. How is your call to CreateFieldTrial() not mucking with FieldTrialList::global_? > If we run into this again, we can refactor > ACFT class.
https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:271: int types = 0; On 2013/02/11 21:42:12, Bart N. wrote: > On 2013/02/11 21:13:34, Mark P wrote: > > do you expect pos to ever not be 0? > > perhaps dcheck this > I don't think it's a good idea to DCHECK against something that is not entirely > in your control. In this case, if someone makes a mistake on the server side, we > would start DCHECKing in Chrome. > > Instead, I've added a warning message. Is that OK with you? I don't see a warning message. Can you please add it? https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:272: base::StringToInt(base::StringPiece( On 2013/02/11 21:42:12, Bart N. wrote: > On 2013/02/11 21:13:34, Mark P wrote: > > consider dchecking the return type of this > Not really; clarified in the comment. You really do want perfect conversions. Don't try to convert DisabledProviders_12i ("i" typo instead of 8) to 12 and blindly continue. Please do some sort of error checking. (The one test you added doesn't catch this.)
lgtm Generally fine, pending resolution of comments on chat. Glad to hear that this fieldtriallist isn't messing up other tests' state. https://codereview.chromium.org/12220004/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc (right): https://codereview.chromium.org/12220004/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc:38: static base::FieldTrialList* field_trial_list_; Consider making this a non-pointer, non-static member as in https://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/metrics/vari... https://codereview.chromium.org/12220004/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc:50: // as it goes. If you want to disable already reported groups, you can use a strategy like this: https://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome_brow...
In case it's not obvious from my last message, feel free to submit after you fix the remaining minor points; don't wait for me to say anything (because I probably won't check my e-mail again today). --mark On 2013/02/11 22:39:46, Mark P wrote: > lgtm > > > Generally fine, pending resolution of comments on chat. Glad to hear that this > fieldtriallist isn't messing up other tests' state. > > https://codereview.chromium.org/12220004/diff/10002/chrome/browser/autocomple... > File chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc (right): > > https://codereview.chromium.org/12220004/diff/10002/chrome/browser/autocomple... > chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc:38: static > base::FieldTrialList* field_trial_list_; > Consider making this a non-pointer, non-static member as in > https://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/metrics/vari... > > https://codereview.chromium.org/12220004/diff/10002/chrome/browser/autocomple... > chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc:50: // as it > goes. > If you want to disable already reported groups, you can use a strategy like > this: > https://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome_brow...
On 2013/02/12 00:17:22, Mark P wrote: > In case it's not obvious from my last message, feel free to submit after you fix > the remaining minor points; don't wait for me to say anything (because I > probably won't check my e-mail again today). Thanks, Mark. I was in a meeting and haven't had a chance to reply yet.
+sky for chrome/chrome_tests_unit.gypi (meanwhile I'm addressing last set of comments).
Mark, thanks for your feedback. As we discussed offline, I addressed the outstanding issues. https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial.cc (right): https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:271: int types = 0; On 2013/02/11 22:04:18, Mark P wrote: > On 2013/02/11 21:42:12, Bart N. wrote: > > On 2013/02/11 21:13:34, Mark P wrote: > > > do you expect pos to ever not be 0? > > > perhaps dcheck this > > I don't think it's a good idea to DCHECK against something that is not > entirely > > in your control. In this case, if someone makes a mistake on the server side, > we > > would start DCHECKing in Chrome. > > > > Instead, I've added a warning message. Is that OK with you? > > I don't see a warning message. Can you please add it? As discussed offline, I made now a stronger requirement and group name must match "DisabledProviders_XXX". https://codereview.chromium.org/12220004/diff/12001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial.cc:272: base::StringToInt(base::StringPiece( On 2013/02/11 22:04:18, Mark P wrote: > On 2013/02/11 21:42:12, Bart N. wrote: > > On 2013/02/11 21:13:34, Mark P wrote: > > > consider dchecking the return type of this > > Not really; clarified in the comment. > > You really do want perfect conversions. Don't try to convert > DisabledProviders_12i ("i" typo instead of 8) to 12 and blindly continue. As discussed offline, it was intended. However, I made it strict now. > > Please do some sort of error checking. (The one test you added doesn't catch > this.) > https://codereview.chromium.org/12220004/diff/10002/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc (right): https://codereview.chromium.org/12220004/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc:38: static base::FieldTrialList* field_trial_list_; On 2013/02/11 22:39:46, Mark P wrote: > Consider making this a non-pointer, non-static member as in > https://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/metrics/vari... As discussed offline, I wanted to avoid initialization in each test method. I know we currently have only 1, but if we add more coverage, we would have to do it each time. https://codereview.chromium.org/12220004/diff/10002/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc:50: // as it goes. On 2013/02/11 22:39:46, Mark P wrote: > If you want to disable already reported groups, you can use a strategy like > this: > https://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome_brow... I actually found a better way. I've added ResetFieldTrialList method, so I can clean the state between various checks.
-sky If you added me for the .gypi, you can TBR it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartn@chromium.org/12220004/21006
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://chromiumcodereview.appspot.com/12220004/diff/10002/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc (right): https://chromiumcodereview.appspot.com/12220004/diff/10002/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_field_trial_unittest.cc:50: // as it goes. On 2013/02/12 01:18:19, Bart N. wrote: > On 2013/02/11 22:39:46, Mark P wrote: > > If you want to disable already reported groups, you can use a strategy like > > this: > > > https://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome_brow... > I actually found a better way. I've added ResetFieldTrialList method, so I can > clean the state between various checks. Nice.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartn@chromium.org/12220004/21006
On 2013/02/12 17:55:18, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/bartn%40chromium.org/12220004/21006 Mark - I've unchecked the commit - there is one issue I have to fix (I forgot to remove unused variable "trial" which fails on android-dbg).
On 2013/02/12 18:01:20, Bart N. wrote: > On 2013/02/12 17:55:18, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/bartn%2540chromium.org/12220004/21006 > > Mark - I've unchecked the commit - there is one issue I have to fix (I forgot to > remove unused variable "trial" which fails on android-dbg). Done. Now it should commit properly. Peter/Mark - do you know why unused variable warning is not an error on linux/mac builds?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartn@chromium.org/12220004/7
Message was sent while issue was closed.
Change committed as 182018 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
