Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
645303003:
Parametrize Suggest polling strategy and delay. Also add a parameter (Closed)
DescriptionParametrize Suggest polling strategy and delay. Also add a parameter
to disable results caching.
TESTS=OmniboxFieldTrialTest + manual tests.
BUG=420903
Committed: https://crrev.com/1c07e721b7a3dcc01b34728c5a9eea80d907a432
Cr-Commit-Position: refs/heads/master@{#301413}
Patch Set 1 #
Total comments: 32
Patch Set 2 : Addressed Mark's & Harry's comments. #
Total comments: 4
Patch Set 3 : Added unit tests to OmniboxFieldTrialTest. #
Total comments: 6
Patch Set 4 : Added more tests. #
Total comments: 2
Patch Set 5 : Fixed two comments. #
Messages
Total messages: 23 (5 generated)
bartn@chromium.org changed reviewers: + hfung@chromium.org, mpearson@chromium.org
Mark & Harry, can you please take a look? I'm planning to add unit tests after your initial feedback.
https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... File components/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:349: kSuggestPollingDelaySinceTheLastRequestRule) == "true"; I'm not sure if you want to have a flag to take in when to reset the counter, such as "keystroke" and "request" instead of using true/false. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:385: const char OmniboxFieldTrial::kSuggestPollingDelayMsRule[] = This is a param, not a rule?
https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... File components/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:356: *polling_delay_ms <= 0) { nit: parans around binary operator <= https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:357: // Status Quo (100ms since the last suggest request). This comment is helpful for reviewers. For the code, I'd either suggest your remove it or phrase it differently such as // Use a default of 100ms since the last suggest request. (once you see that phrasing, you'll see what I mean when you say it's not necessary in the code) https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:384: "SuggestPollingDelaySinceTheLastRequest"; If you change the name of the rule, please revise the string here too. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:385: const char OmniboxFieldTrial::kSuggestPollingDelayMsRule[] = On 2014/10/20 21:59:42, H Fung wrote: > This is a param, not a rule? It's a "rule". In this file, rule correspond to the key part of the key-value field that make up the params structure. The value part of the param variables. I know it's weird nomenclature. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.h:284: // For Search Provider related experiments. nit: SearchProvider https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.h:311: // Search Provider experiments. nit: comment unnecessary (we omit all similar comments in this list of rules) https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.h:313: static const char kSuggestPollingDelaySinceTheLastRequestRule[]; nit: I'd prefer this rule name had a verb such as kMeasureSuggestPollingDelayFromLastRequestRule https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:120: int SearchProvider::kMinimumTimeBetweenSuggestQueriesMs = 100; Shall we delete this? It's unused now. https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:583: bool since_the_last_request = true; I suggest you don't initialize these two variables here. Leave it up to GetSuggestPollingStrategy to set their (default) values correctly. https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:594: return std::max(base::TimeDelta(), delay - time_since_last_suggest_request); You shouldn't need this max. TimeTicks is supposed to be monotonically increasing. https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:605: ClearAllResults(); For your experiment, do you also want to clear cached answers? https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.h:205: base::TimeDelta GetSuggestQueryDelay(); Can this function be const?
Thanks Mark and Harry. See my replies (one question about answers, as I'm not sure if I know how it currently works). Will send another update with some unit tests if there are no more comments to the core logic. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... File components/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:349: kSuggestPollingDelaySinceTheLastRequestRule) == "true"; On 2014/10/20 21:59:42, H Fung wrote: > I'm not sure if you want to have a flag to take in when to reset the counter, > such as "keystroke" and "request" instead of using true/false. I think I'd prefer to stay with the current rule, since we are not planning to add any new type soon. If so, we can change it easily. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:356: *polling_delay_ms <= 0) { On 2014/10/21 21:28:22, Mark P wrote: > nit: parans around binary operator <= Done. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:357: // Status Quo (100ms since the last suggest request). On 2014/10/21 21:28:22, Mark P wrote: > This comment is helpful for reviewers. For the code, I'd either suggest your > remove it or phrase it differently such as > // Use a default of 100ms since the last suggest request. > > (once you see that phrasing, you'll see what I mean when you say it's not > necessary in the code) Done. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:384: "SuggestPollingDelaySinceTheLastRequest"; On 2014/10/21 21:28:22, Mark P wrote: > If you change the name of the rule, please revise the string here too. Done. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:385: const char OmniboxFieldTrial::kSuggestPollingDelayMsRule[] = On 2014/10/21 21:28:22, Mark P wrote: > On 2014/10/20 21:59:42, H Fung wrote: > > This is a param, not a rule? > > It's a "rule". In this file, rule correspond to the key part of the key-value > field that make up the params structure. The value part of the param variables. > I know it's weird nomenclature. Acknowledged. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.cc:385: const char OmniboxFieldTrial::kSuggestPollingDelayMsRule[] = On 2014/10/20 21:59:42, H Fung wrote: > This is a param, not a rule? Acknowledged. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.h:284: // For Search Provider related experiments. On 2014/10/21 21:28:22, Mark P wrote: > nit: SearchProvider Done. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.h:311: // Search Provider experiments. On 2014/10/21 21:28:22, Mark P wrote: > nit: comment unnecessary > (we omit all similar comments in this list of rules) Done. https://codereview.chromium.org/645303003/diff/1/components/omnibox/omnibox_f... components/omnibox/omnibox_field_trial.h:313: static const char kSuggestPollingDelaySinceTheLastRequestRule[]; On 2014/10/21 21:28:22, Mark P wrote: > nit: I'd prefer this rule name had a verb such as > kMeasureSuggestPollingDelayFromLastRequestRule Done. https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:120: int SearchProvider::kMinimumTimeBetweenSuggestQueriesMs = 100; On 2014/10/21 21:28:22, Mark P wrote: > Shall we delete this? It's unused now. Moved to OFT. https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:583: bool since_the_last_request = true; On 2014/10/21 21:28:22, Mark P wrote: > I suggest you don't initialize these two variables here. Leave it up to > GetSuggestPollingStrategy to set their (default) values correctly. Done. https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:594: return std::max(base::TimeDelta(), delay - time_since_last_suggest_request); On 2014/10/21 21:28:22, Mark P wrote: > You shouldn't need this max. TimeTicks is supposed to be monotonically > increasing. What if time_since_last_suggest_request is more than delay? Wouldn't it return a negative delta? We don't want to wait in this case (thus 0). https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:605: ClearAllResults(); On 2014/10/21 21:28:22, Mark P wrote: > For your experiment, do you also want to clear cached answers? I think so. Do you know why it is not cleared already in ClearAllResults? https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... File components/omnibox/search_provider.h (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.h:205: base::TimeDelta GetSuggestQueryDelay(); On 2014/10/21 21:28:22, Mark P wrote: > Can this function be const? Oh yes :) not sure why I forgot that.
No complaints about the core logic; feel free to write unit tests to the extent you feel is useful. --mark https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:594: return std::max(base::TimeDelta(), delay - time_since_last_suggest_request); On 2014/10/22 01:19:08, Bart N. wrote: > On 2014/10/21 21:28:22, Mark P wrote: > > You shouldn't need this max. TimeTicks is supposed to be monotonically > > increasing. > What if time_since_last_suggest_request is more than delay? Wouldn't it return a > negative delta? We don't want to wait in this case (thus 0). Well, negative delays shouldn't wait in any case. But I see your point that it makes more sense to return 0 for no delay, not a negative delay (whatever that means). https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:605: ClearAllResults(); On 2014/10/22 01:19:08, Bart N. wrote: > On 2014/10/21 21:28:22, Mark P wrote: > > For your experiment, do you also want to clear cached answers? > I think so. Do you know why it is not cleared already in ClearAllResults? Well, my understanding of how answers work is that the answers are stored and annotated with the query they're attached to. They're only displayed if we would normally display a query suggestion that matches something in the answers cache. I think this means it's okay to not clear cached answers in this experiment -- it's not like a suggestion/answer would display at a time incommensurate with what would've been displayed had no answers been there. But I asked because I wanted to reason this out with you. You will get cached answers with queries that previously had answers. That seems reasonable. https://codereview.chromium.org/645303003/diff/20001/components/omnibox/omnib... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/645303003/diff/20001/components/omnibox/omnib... components/omnibox/omnibox_field_trial.h:325: // previous one. Non-const because some unittests modify this value. one -> one unless overridden by a field trial parameter https://codereview.chromium.org/645303003/diff/20001/components/omnibox/omnib... components/omnibox/omnibox_field_trial.h:326: static int kMinimumTimeBetweenSuggestQueriesMs; add Default in this name somewhere
I've added some unit tests and for the remaining logic I'm planning to test by pointing my chrome to a test Finch server. I decided not to implement unit tests for SP, because of complexity and also the original implementation of polling had no coverage. I think the logic is fairly simple and it's sufficient if I test it locally before submitting this CL. PTAL https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... File components/omnibox/search_provider.cc (right): https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:594: return std::max(base::TimeDelta(), delay - time_since_last_suggest_request); On 2014/10/22 20:33:06, Mark P wrote: > On 2014/10/22 01:19:08, Bart N. wrote: > > On 2014/10/21 21:28:22, Mark P wrote: > > > You shouldn't need this max. TimeTicks is supposed to be monotonically > > > increasing. > > What if time_since_last_suggest_request is more than delay? Wouldn't it return > a > > negative delta? We don't want to wait in this case (thus 0). > > Well, negative delays shouldn't wait in any case. But I see your point that it > makes more sense to return 0 for no delay, not a negative delay (whatever that > means). Acknowledged. https://codereview.chromium.org/645303003/diff/1/components/omnibox/search_pr... components/omnibox/search_provider.cc:605: ClearAllResults(); On 2014/10/22 20:33:06, Mark P wrote: > On 2014/10/22 01:19:08, Bart N. wrote: > > On 2014/10/21 21:28:22, Mark P wrote: > > > For your experiment, do you also want to clear cached answers? > > I think so. Do you know why it is not cleared already in ClearAllResults? > > Well, my understanding of how answers work is that the answers are stored and > annotated with the query they're attached to. They're only displayed if we > would normally display a query suggestion that matches something in the answers > cache. I think this means it's okay to not clear cached answers in this > experiment -- it's not like a suggestion/answer would display at a time > incommensurate with what would've been displayed had no answers been there. But > I asked because I wanted to reason this out with you. You will get cached > answers with queries that previously had answers. That seems reasonable. > Acknowledged. https://codereview.chromium.org/645303003/diff/20001/components/omnibox/omnib... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/645303003/diff/20001/components/omnibox/omnib... components/omnibox/omnibox_field_trial.h:325: // previous one. Non-const because some unittests modify this value. On 2014/10/22 20:33:07, Mark P wrote: > one > -> > one unless overridden by a field trial parameter Done. https://codereview.chromium.org/645303003/diff/20001/components/omnibox/omnib... components/omnibox/omnibox_field_trial.h:326: static int kMinimumTimeBetweenSuggestQueriesMs; On 2014/10/22 20:33:07, Mark P wrote: > add Default in this name somewhere Done.
https://codereview.chromium.org/645303003/diff/40001/components/omnibox/omnib... File components/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/645303003/diff/40001/components/omnibox/omnib... components/omnibox/omnibox_field_trial_unittest.cc:107: bool expected_from_last_keystroke, this and below might as well be const https://codereview.chromium.org/645303003/diff/40001/components/omnibox/omnib... components/omnibox/omnibox_field_trial_unittest.cc:431: TEST_F(OmniboxFieldTrialTest, GetSuggestPollingStrategy) { The function this test uses VerifySuggestPollingStrategy() always sets both parameters. Can you revise this framework to allow testing that things work right if only one of the polling strategy parameter fields is set? https://codereview.chromium.org/645303003/diff/40001/components/omnibox/omnib... components/omnibox/omnibox_field_trial_unittest.cc:445: } Please add a test for the default values of this polling strategy.
Thanks for the quick review. https://codereview.chromium.org/645303003/diff/40001/components/omnibox/omnib... File components/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/645303003/diff/40001/components/omnibox/omnib... components/omnibox/omnibox_field_trial_unittest.cc:107: bool expected_from_last_keystroke, On 2014/10/22 23:03:15, Mark P wrote: > this and below might as well be const We don't typically put const for params passed by value. See examples above and elsewhere. If you really insist, I can do it, but please provide a good reason of doing so. https://codereview.chromium.org/645303003/diff/40001/components/omnibox/omnib... components/omnibox/omnibox_field_trial_unittest.cc:431: TEST_F(OmniboxFieldTrialTest, GetSuggestPollingStrategy) { On 2014/10/22 23:03:15, Mark P wrote: > The function this test uses VerifySuggestPollingStrategy() always sets both > parameters. Can you revise this framework to allow testing that things work > right if only one of the polling strategy parameter fields is set? > Done. https://codereview.chromium.org/645303003/diff/40001/components/omnibox/omnib... components/omnibox/omnibox_field_trial_unittest.cc:445: } On 2014/10/22 23:03:15, Mark P wrote: > Please add a test for the default values of this polling strategy. Done.
lgtm https://codereview.chromium.org/645303003/diff/60001/components/omnibox/omnib... File components/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/645303003/diff/60001/components/omnibox/omnib... components/omnibox/omnibox_field_trial_unittest.cc:68: // EXPECTS that OmniboxFieldTrial::GetSuggestPollingStrategy returns EXPECT()s instead of EXPECTS similar to above?
lgtm; note harry's minor comment
Thanks. I'm doing more sanity tests and will submit sometime later this week. https://codereview.chromium.org/645303003/diff/60001/components/omnibox/omnib... File components/omnibox/omnibox_field_trial_unittest.cc (right): https://codereview.chromium.org/645303003/diff/60001/components/omnibox/omnib... components/omnibox/omnibox_field_trial_unittest.cc:68: // EXPECTS that OmniboxFieldTrial::GetSuggestPollingStrategy returns On 2014/10/23 17:58:56, H Fung wrote: > EXPECT()s instead of EXPECTS similar to above? It was a copy from VerifyDemotion above, which I also fixed.
The CQ bit was checked by bartn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645303003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
bartn@chromium.org changed reviewers: + jered@chromium.org, kmadhusu@chromium.org, samarth@chromium.org
Adding Instant folks to LGTM the tiny change in chrome/browser/ui/search/.
lgtm for the test change
The CQ bit was checked by bartn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/645303003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1c07e721b7a3dcc01b34728c5a9eea80d907a432 Cr-Commit-Position: refs/heads/master@{#301413} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
