Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
905023003:
Adding knobs on HQP provider. (Closed)
|
Created:
5 years, 10 months ago by Ashok vardhan Modified:
5 years, 10 months ago CC:
chromium-reviews, browser-components-watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding knobs on HQP provider. Having an option of experimenting with topicality score and changing final relevance score ranges from omnibox bundled params.
1. Option to suppress the URLs with topicality score lower than the given threshold. This way we can control the URLs with high popularity and low topicality scoring.
2. Changing the final-relevance score buckets to linearly interpolate with in-certain ranges. This will help to control the score ranges.
With above options one can set the bundled params. Ex:
HQPExperimentalScoringEnabled="true"
HQPExperimentalScoringTopicalityThreshold="0.8"
HQPExperimentalScoringBuckets="0.0:400,1.5:600,12.0:1300,20.0:1399"
More details can be found in the bug: crbug/306198
BUG=306198
TEST=ScoredHistoryMatchTest.GetFinalRelevancyScore
Committed: https://crrev.com/0b06844f4d6e6a9a3b42438493c52d248397011d
Cr-Commit-Position: refs/heads/master@{#317369}
Patch Set 1 : Initial Change to control HQP scoring. #
Total comments: 14
Patch Set 2 : Addressing bart comments. PTAL. #
Total comments: 4
Patch Set 3 : Minor fix. #
Total comments: 20
Patch Set 4 : Addressing mark comments. #
Total comments: 36
Patch Set 5 : Addressing more comments. #Patch Set 6 : Addressing mark comments. #
Total comments: 18
Patch Set 7 : Syncing with new refactoring #Patch Set 8 : Minor fix in Unit test #Patch Set 9 : Addressing mark comments. #
Total comments: 12
Patch Set 10 : Addressing comments. #Patch Set 11 : Minor fix #Patch Set 12 : Float error fix #
Messages
Total messages: 46 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
ashokvardhan@chromium.org changed reviewers: + bartn@chromium.org, mpearson@chromium.org
On 2015/02/09 23:20:05, ashokvardhan wrote: > mailto:ashokvardhan@chromium.org changed reviewers: > + mailto:bartn@chromium.org, mailto:mpearson@chromium.org Its ready for review. Please take a look. Thanks.
A few high level comments. I haven't looked deeply though (leaving up to Mark). Thanks for putting it together so quick! https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:450: if ((hqp_experimental_scoring_enabled_) && Not sure but I don't think we need () around single expressions? https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:452: return 0.0; Wrong indent. https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:598: if (base::SplitStringIntoKeyValuePairs(hqp_relevance_buckets, I don't think you want to parse it each time... I'd parse it once and keep it as a const&. See how I implemented HUP scoring params. https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... File chrome/browser/history/scored_history_match.h (right): https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:90: FRIEND_TEST_ALL_PREFIXES(ScoredHistoryMatchTest, GetFinalRelevancyScore); Sort? https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:144: // 3. topicality_threshold_,is used to control the topicality scoring. extra comma? https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... File chrome/browser/history/scored_history_match_unittest.cc (right): https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match_unittest.cc:370: EXPECT_EQ(600, // 500 + (((900 - 500)/(8 -4)) * 1) = 600. Extra space after , (at least 2 spaces).
Seems like reasonable general knobs. --mark https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:598: if (base::SplitStringIntoKeyValuePairs(hqp_relevance_buckets, On 2015/02/10 01:22:29, Bart N. wrote: > I don't think you want to parse it each time... Indeed, this function likely gets called thousands of time per keystroke. > I'd parse it once and keep it as > a const&. See how I implemented HUP scoring params.
Addressed comments. PTAL. https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:450: if ((hqp_experimental_scoring_enabled_) && On 2015/02/10 01:22:29, Bart N. wrote: > Not sure but I don't think we need () around single expressions? Done. https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:452: return 0.0; On 2015/02/10 01:22:29, Bart N. wrote: > Wrong indent. Done. https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:598: if (base::SplitStringIntoKeyValuePairs(hqp_relevance_buckets, On 2015/02/10 18:33:40, Mark P wrote: > On 2015/02/10 01:22:29, Bart N. wrote: > > I don't think you want to parse it each time... > Indeed, this function likely gets called thousands of time per keystroke. > > > I'd parse it once and keep it as > > a const&. See how I implemented HUP scoring params. Completely agreed and didn't realise. Changed the code. Thanks for pointing. https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:598: if (base::SplitStringIntoKeyValuePairs(hqp_relevance_buckets, On 2015/02/10 01:22:29, Bart N. wrote: > I don't think you want to parse it each time... I'd parse it once and keep it as > a const&. See how I implemented HUP scoring params. Done. https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... File chrome/browser/history/scored_history_match.h (right): https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:90: FRIEND_TEST_ALL_PREFIXES(ScoredHistoryMatchTest, GetFinalRelevancyScore); On 2015/02/10 01:22:29, Bart N. wrote: > Sort? Acknowledged. https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:144: // 3. topicality_threshold_,is used to control the topicality scoring. On 2015/02/10 01:22:29, Bart N. wrote: > extra comma? Done. https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... File chrome/browser/history/scored_history_match_unittest.cc (right): https://codereview.chromium.org/905023003/diff/160001/chrome/browser/history/... chrome/browser/history/scored_history_match_unittest.cc:370: EXPECT_EQ(600, // 500 + (((900 - 500)/(8 -4)) * 1) = 600. On 2015/02/10 01:22:30, Bart N. wrote: > Extra space after , (at least 2 spaces). Done.
lgtm https://codereview.chromium.org/905023003/diff/180001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/180001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:638: topicality_threshold_ = hqp_experimental_topicality_threhold; Extra space after =. https://codereview.chromium.org/905023003/diff/180001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:643: if (!hqp_experimental_scoring_buckets.empty()) { You should be consistent with how you use {} around single line "if" statements (check the line above).
New patchsets have been uploaded after l-g-t-m from bartn@chromium.org
Thanks for review bart. https://codereview.chromium.org/905023003/diff/180001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/180001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:638: topicality_threshold_ = hqp_experimental_topicality_threhold; On 2015/02/11 01:30:15, Bart N. wrote: > Extra space after =. Done. https://codereview.chromium.org/905023003/diff/180001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:643: if (!hqp_experimental_scoring_buckets.empty()) { On 2015/02/11 01:30:15, Bart N. wrote: > You should be consistent with how you use {} around single line "if" statements > (check the line above). Done.
This is still an inefficient implementation. See comments below. I haven't finished reviewing it, but this should be enough for you to make revisions. --mark https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:67: InitializeHQPExperimentalParams(); This new location for initializing the parameters does not help. This class gets constructed each time we want to score a URL; you're still calling the initialization functions thousands of times on each keystroke. It would make more sense to make your variables static and initialize them once by calling your init function within the main Init(). https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... File chrome/browser/history/scored_history_match.h (right): https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:131: // an appropriate value to use as a relevancy score. This comment doesn't make sense attached to this typedef. Please comment the typedef and also move this comment to GetFinalRelevancyScore() and revise it appropriately. https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:137: std::vector<ScoreMaxRelevance>& hqp_relevance_buckets); const https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... File components/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.cc:320: base::StringToDouble(topicality_threhold_str, &topicality_threshold); You might as well use the return value https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:273: // For HQP related experiments. please make this comment clearer about what this section is. (There are other "HQP related" experiments in this file.) https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:274: // nit: want a blank line without a // here, like all other sections https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:275: // Returns true if the HQP experimenal scoring is enabled. spelling mistake, plus omit "the" https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:279: // incase not initialized. 1. "in case" is two words. 2. in case what is not "initialized"? The scoring buckets? The parameter that defines the scoring buckets? ... Did you mean "defined"? 3. Any function should describe its return type. How are "scoring buckets" stored in a string form? You can point people to a description elsewhere if you want. 4. Also, I have a minor preference for returning a plausible string as the return type if the parameter isn't defined; the empty string makes no sense. https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:283: // initialized return -1. again, I think you mean: it -> the parameter initialized -> defined https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:315: // Parameter names used by the HQP scoring experiments. insert "experimental", as you're going to do in the earlier comment
Addressed the comments. PTAL. https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:67: InitializeHQPExperimentalParams(); On 2015/02/11 21:57:29, Mark P wrote: > This new location for initializing the parameters does not help. This class > gets constructed each time we want to score a URL; you're still calling the > initialization functions thousands of times on each keystroke. > > It would make more sense to make your variables static and initialize them once > by calling your init function within the main Init(). I see. Got your point. Thought my parsing was the problem for each URL, but didn't notice this. Fixed it. https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... File chrome/browser/history/scored_history_match.h (right): https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:131: // an appropriate value to use as a relevancy score. On 2015/02/11 21:57:29, Mark P wrote: > This comment doesn't make sense attached to this typedef. Please comment the > typedef and also move this comment to GetFinalRelevancyScore() and revise it > appropriately. Done. https://codereview.chromium.org/905023003/diff/200001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:137: std::vector<ScoreMaxRelevance>& hqp_relevance_buckets); On 2015/02/11 21:57:30, Mark P wrote: > const Done. https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... File components/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.cc:320: base::StringToDouble(topicality_threhold_str, &topicality_threshold); On 2015/02/11 21:57:30, Mark P wrote: > You might as well use the return value Done. https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:273: // For HQP related experiments. On 2015/02/11 21:57:30, Mark P wrote: > please make this comment clearer about what this section is. (There are other > "HQP related" experiments in this file.) Done. https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:274: // On 2015/02/11 21:57:30, Mark P wrote: > nit: want a blank line without a // here, like all other sections Done. https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:275: // Returns true if the HQP experimenal scoring is enabled. On 2015/02/11 21:57:30, Mark P wrote: > spelling mistake, plus omit "the" Done. https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:279: // incase not initialized. On 2015/02/11 21:57:30, Mark P wrote: > 1. "in case" is two words. > 2. in case what is not "initialized"? The scoring buckets? The parameter that > defines the scoring buckets? ... Did you mean "defined"? > 3. Any function should describe its return type. How are "scoring buckets" > stored in a string form? You can point people to a description elsewhere if you > want. > 4. Also, I have a minor preference for returning a plausible string as the return type if the parameter isn't defined; the empty string makes no sense. I dont want to return the special string if the experimental scoring is not enabled. Hence i am returning empty string. Ideally this function is not called, if experimental scoring is disabled. Done. https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:283: // initialized return -1. On 2015/02/11 21:57:30, Mark P wrote: > again, I think you mean: > it -> the parameter > initialized -> defined Done. https://codereview.chromium.org/905023003/diff/200001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:315: // Parameter names used by the HQP scoring experiments. On 2015/02/11 21:57:30, Mark P wrote: > insert "experimental", as you're going to do in the earlier comment Done.
Here's another batch of comments. I still haven't looked at the unittest. --mark https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:45: bool ScoredHistoryMatch::hqp_experimental_scoring_enabled_ = false; (1) These static initializers should be in the order of their declaration in the file. (Respond to this after you change the header.) (2) There's no reason to separate these variables from the others. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:583: // HQP default scoring buckets: "1.5:600,12.0:1300,20.0:1399" I think you'll want your format to require having a 0.0 point set. Don't hard-code it to 400 as you do below. This will allow you to try more types of experiments later. As you make this fix, you'll want to change this example string and the example strings and default value string below. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:590: // The score maxes out at 1400 (i.e., cannot beat a good inline result). 1400 -> 1399 also, good inline result -> good inlineable result from HistoryURL provider https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:592: // If experimental scoring is enabled, then the score buckets will be like: I don't know what line 592-593 are adding. Is it merely that you wanted to give another example of a scoring bucket string? I'm not sure that's necessary. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:596: double base_intermediate_score = 0.0; Please comment the variables in this block more or give them better names (or both!). I can't figure out what you're doing without reading the code in detail. P.S. I think having the 0 point specified in the config string will make the code easier; you probably need two fewer variables. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:603: ScoreMaxRelevance hqp_bucket = hqp_relevance_buckets[i]; const & https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:610: const int final_hqp_score = (base_hqp_score + does the whole right side of the equals fit on one line? If so, do that rather than spread it over three. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:610: const int final_hqp_score = (base_hqp_score + This function returns a float. Don't force it to an int unnecessarily. ditto below with other return value (though in that case it matters less) https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:613: return std::min(final_hqp_score, max_hqp_score); Why is this min necessary? https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:619: // when buckets are not specified. Return max_hqp_score. Buckets should never be not specified. Correct this comment. Perhaps even put a DCHECK(buckets.size > 0) at the beginning of the function, if you feel like it. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:637: if (hqp_experimental_topicality_threhold > 0) Why have a >0 test? If you're checking for unspecified, use the -1 that you say you return for unspecified parameters. That said, I'm not convinced you need to check the return value here. -1 is a perfectly fine default to set the threshold_ variable to. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:657: base::StringToDouble(it->first, &bucket.first); minor nit: consider DCHECKING the return value for these https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... File chrome/browser/history/scored_history_match.h (right): https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:32: // ScoreMaxRelevance maps from intermediate-score to the final-relevance from -> from an to the -> to the maximum https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:33: // score given to URL. This is used to store the score ranges of HQP relevance to URL -> to a URL for this intermediate score. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:137: // specified through |hqp_relevance_buckets|. Please see function see -> see the https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:144: // Initializes the HQP experimental params. This comment is a verb, implying it should go next to a function. Yet it's associated with a variable. Please rearrange/revise these comments so they're appropriate. Also note that the style guide says all functions should be declared in class files before all variables. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:145: // If the experimental scoring is enabled, it sets: By the way, this comment is wrong as written. In particular, you end up seeing some of these variables even if hqp_experimental_scoring is not enabled (and you rely on these settings in the code even when experimental scoring is not enabled). https://codereview.chromium.org/905023003/diff/220001/components/omnibox/omni... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/905023003/diff/220001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:277: // |kHQPExperimentalScoringEnabledParam| is not defined. This last sentence is wrong. This variable is a static member variable and is always defined/initialized. You mean something like "Returns false if the appropriate parameter is not specified in the field trial." analogous both places below (something like) Returns false if experimental scoring is not enabled or if the appropriate parameter is not specified in the field trial.
Thanks for comments. PTAL. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:45: bool ScoredHistoryMatch::hqp_experimental_scoring_enabled_ = false; On 2015/02/14 01:27:14, Mark P wrote: > (1) These static initializers should be in the order of their declaration in the > file. > (Respond to this after you change the header.) > (2) There's no reason to separate these variables from the others. Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:583: // HQP default scoring buckets: "1.5:600,12.0:1300,20.0:1399" On 2015/02/14 01:27:13, Mark P wrote: > I think you'll want your format to require having a 0.0 point set. Don't > hard-code it to 400 as you do below. This will allow you to try more types of > experiments later. > As you make this fix, you'll want to change this example string and the example > strings and default value string below. Make sense. Thought all the minimum scores for providers are not controlled from params. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:590: // The score maxes out at 1400 (i.e., cannot beat a good inline result). On 2015/02/14 01:27:14, Mark P wrote: > 1400 -> 1399 > also, good inline result -> good inlineable result from HistoryURL provider Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:592: // If experimental scoring is enabled, then the score buckets will be like: On 2015/02/14 01:27:14, Mark P wrote: > I don't know what line 592-593 are adding. Is it merely that you wanted to give > another example of a scoring bucket string? I'm not sure that's necessary. Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:596: double base_intermediate_score = 0.0; On 2015/02/14 01:27:14, Mark P wrote: > Please comment the variables in this block more or give them better names (or > both!). I can't figure out what you're doing without reading the code in > detail. > > P.S. I think having the 0 point specified in the config string will make the > code easier; you probably need two fewer variables. Acknowledged. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:603: ScoreMaxRelevance hqp_bucket = hqp_relevance_buckets[i]; On 2015/02/14 01:27:14, Mark P wrote: > const & Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:610: const int final_hqp_score = (base_hqp_score + On 2015/02/14 01:27:14, Mark P wrote: > This function returns a float. Don't force it to an int unnecessarily. > > ditto below with other return value > (though in that case it matters less) Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:610: const int final_hqp_score = (base_hqp_score + On 2015/02/14 01:27:13, Mark P wrote: > does the whole right side of the equals fit on one line? If so, do that rather > than spread it over three. Nope. And also i guess its easy to read if it is like this. Please let me know your thoughts. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:610: const int final_hqp_score = (base_hqp_score + On 2015/02/14 01:27:13, Mark P wrote: > does the whole right side of the equals fit on one line? If so, do that rather > than spread it over three. Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:613: return std::min(final_hqp_score, max_hqp_score); On 2015/02/14 01:27:13, Mark P wrote: > Why is this min necessary? Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:619: // when buckets are not specified. Return max_hqp_score. On 2015/02/14 01:27:14, Mark P wrote: > Buckets should never be not specified. Correct this comment. > > Perhaps even put a DCHECK(buckets.size > 0) at the beginning of the function, if > you feel like it. Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:637: if (hqp_experimental_topicality_threhold > 0) On 2015/02/14 01:27:14, Mark P wrote: > Why have a >0 test? If you're checking for unspecified, use the -1 that you say > you return for unspecified parameters. > > That said, I'm not convinced you need to check the return value here. -1 is a > perfectly fine default to set the threshold_ variable to. Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:657: base::StringToDouble(it->first, &bucket.first); On 2015/02/14 01:27:14, Mark P wrote: > minor nit: consider DCHECKING the return value for these DCHECK seems to be not working here. std::string x = "1"; int a = 0; int b = 0; base::StringToInt(x, &a); DCHECK(base::StringToInt(x, &b)); LOG(INFO) << "a is : " << a << std::endl; LOG(INFO) << "b is : " << b << std::endl; Logs: a is : 1 b is : 0 Am i missing anything here ? I used CHECK in our code-base, but this is strange. I can use if condition checkings but not sure why this is not working. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... File chrome/browser/history/scored_history_match.h (right): https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:32: // ScoreMaxRelevance maps from intermediate-score to the final-relevance On 2015/02/14 01:27:14, Mark P wrote: > from -> from an > to the -> to the maximum Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:33: // score given to URL. This is used to store the score ranges of HQP relevance On 2015/02/14 01:27:14, Mark P wrote: > to URL -> to a URL for this intermediate score. Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:137: // specified through |hqp_relevance_buckets|. Please see function On 2015/02/14 01:27:14, Mark P wrote: > see -> see the Done. https://codereview.chromium.org/905023003/diff/220001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:144: // Initializes the HQP experimental params. On 2015/02/14 01:27:14, Mark P wrote: > This comment is a verb, implying it should go next to a function. Yet it's > associated with a variable. Please rearrange/revise these comments so they're > appropriate. > > Also note that the style guide says all functions should be declared in class > files before all variables. Done. https://codereview.chromium.org/905023003/diff/220001/components/omnibox/omni... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/905023003/diff/220001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:277: // |kHQPExperimentalScoringEnabledParam| is not defined. On 2015/02/14 01:27:14, Mark P wrote: > This last sentence is wrong. This variable is a static member variable and is > always defined/initialized. You mean something like "Returns false if the > appropriate parameter is not specified in the field trial." > > analogous both places below (something like) > Returns false if experimental scoring is not enabled or if the appropriate > parameter is not specified in the field trial. Acknowledged.
The big ScoredHistoryMatch refactoring was just submitted. https://codereview.chromium.org/896983003 You'll have a lot of work to do as you rebase. The key highlight for you is that the class has been split into two, to ScoredHistoryMatch and ScoredHistoryMatchBuilderImpl. The former has the scoring code. The latter has what you might think of as the static constructors and initialization code. Neither class lives in the current file location that you were modifying; that file has been deleted. Please feel free to ask me if you have trouble figuring out how to rearrange your code after syncing. You should haven't to rewrite anything I think; it should just be moving lines around. --mark On Mon, Feb 16, 2015 at 5:23 PM, <ashokvardhan@chromium.org> wrote: > Thanks for comments. PTAL. > > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc > File chrome/browser/history/scored_history_match.cc (right): > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode45 > chrome/browser/history/scored_history_match.cc:45: bool > ScoredHistoryMatch::hqp_experimental_scoring_enabled_ = false; > On 2015/02/14 01:27:14, Mark P wrote: > >> (1) These static initializers should be in the order of their >> > declaration in the > >> file. >> (Respond to this after you change the header.) >> (2) There's no reason to separate these variables from the others. >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode583 > chrome/browser/history/scored_history_match.cc:583: // HQP default > scoring buckets: "1.5:600,12.0:1300,20.0:1399" > On 2015/02/14 01:27:13, Mark P wrote: > >> I think you'll want your format to require having a 0.0 point set. >> > Don't > >> hard-code it to 400 as you do below. This will allow you to try more >> > types of > >> experiments later. >> As you make this fix, you'll want to change this example string and >> > the example > >> strings and default value string below. >> > > Make sense. Thought all the minimum scores for providers are not > controlled from params. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode590 > chrome/browser/history/scored_history_match.cc:590: // The score maxes > out at 1400 (i.e., cannot beat a good inline result). > On 2015/02/14 01:27:14, Mark P wrote: > >> 1400 -> 1399 >> also, good inline result -> good inlineable result from HistoryURL >> > provider > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode592 > chrome/browser/history/scored_history_match.cc:592: // If experimental > scoring is enabled, then the score buckets will be like: > On 2015/02/14 01:27:14, Mark P wrote: > >> I don't know what line 592-593 are adding. Is it merely that you >> > wanted to give > >> another example of a scoring bucket string? I'm not sure that's >> > necessary. > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode596 > chrome/browser/history/scored_history_match.cc:596: double > base_intermediate_score = 0.0; > On 2015/02/14 01:27:14, Mark P wrote: > >> Please comment the variables in this block more or give them better >> > names (or > >> both!). I can't figure out what you're doing without reading the code >> > in > >> detail. >> > > P.S. I think having the 0 point specified in the config string will >> > make the > >> code easier; you probably need two fewer variables. >> > > Acknowledged. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode603 > chrome/browser/history/scored_history_match.cc:603: ScoreMaxRelevance > hqp_bucket = hqp_relevance_buckets[i]; > On 2015/02/14 01:27:14, Mark P wrote: > >> const & >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode610 > chrome/browser/history/scored_history_match.cc:610: const int > final_hqp_score = (base_hqp_score + > On 2015/02/14 01:27:14, Mark P wrote: > >> This function returns a float. Don't force it to an int >> > unnecessarily. > > ditto below with other return value >> (though in that case it matters less) >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode610 > chrome/browser/history/scored_history_match.cc:610: const int > final_hqp_score = (base_hqp_score + > On 2015/02/14 01:27:13, Mark P wrote: > >> does the whole right side of the equals fit on one line? If so, do >> > that rather > >> than spread it over three. >> > > Nope. And also i guess its easy to read if it is like this. Please let > me know your thoughts. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode610 > chrome/browser/history/scored_history_match.cc:610: const int > final_hqp_score = (base_hqp_score + > On 2015/02/14 01:27:13, Mark P wrote: > >> does the whole right side of the equals fit on one line? If so, do >> > that rather > >> than spread it over three. >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode613 > chrome/browser/history/scored_history_match.cc:613: return > std::min(final_hqp_score, max_hqp_score); > On 2015/02/14 01:27:13, Mark P wrote: > >> Why is this min necessary? >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode619 > chrome/browser/history/scored_history_match.cc:619: // when buckets are > not specified. Return max_hqp_score. > On 2015/02/14 01:27:14, Mark P wrote: > >> Buckets should never be not specified. Correct this comment. >> > > Perhaps even put a DCHECK(buckets.size > 0) at the beginning of the >> > function, if > >> you feel like it. >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode637 > chrome/browser/history/scored_history_match.cc:637: if > (hqp_experimental_topicality_threhold > 0) > On 2015/02/14 01:27:14, Mark P wrote: > >> Why have a >0 test? If you're checking for unspecified, use the -1 >> > that you say > >> you return for unspecified parameters. >> > > That said, I'm not convinced you need to check the return value here. >> > -1 is a > >> perfectly fine default to set the threshold_ variable to. >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.cc#newcode657 > chrome/browser/history/scored_history_match.cc:657: > base::StringToDouble(it->first, &bucket.first); > On 2015/02/14 01:27:14, Mark P wrote: > >> minor nit: consider DCHECKING the return value for these >> > > DCHECK seems to be not working here. > > std::string x = "1"; > int a = 0; > int b = 0; > base::StringToInt(x, &a); > DCHECK(base::StringToInt(x, &b)); > > LOG(INFO) << "a is : " << a << std::endl; > LOG(INFO) << "b is : " << b << std::endl; > > Logs: > a is : 1 > b is : 0 > > Am i missing anything here ? I used CHECK in our code-base, but this is > strange. I can use if condition checkings but not sure why this is not > working. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.h > File chrome/browser/history/scored_history_match.h (right): > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.h#newcode32 > chrome/browser/history/scored_history_match.h:32: // ScoreMaxRelevance > maps from intermediate-score to the final-relevance > On 2015/02/14 01:27:14, Mark P wrote: > >> from -> from an >> to the -> to the maximum >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.h#newcode33 > chrome/browser/history/scored_history_match.h:33: // score given to URL. > This is used to store the score ranges of HQP relevance > On 2015/02/14 01:27:14, Mark P wrote: > >> to URL -> to a URL for this intermediate score. >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.h#newcode137 > chrome/browser/history/scored_history_match.h:137: // specified through > |hqp_relevance_buckets|. Please see function > On 2015/02/14 01:27:14, Mark P wrote: > >> see -> see the >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > chrome/browser/history/scored_history_match.h#newcode144 > chrome/browser/history/scored_history_match.h:144: // Initializes the > HQP experimental params. > On 2015/02/14 01:27:14, Mark P wrote: > >> This comment is a verb, implying it should go next to a function. Yet >> > it's > >> associated with a variable. Please rearrange/revise these comments so >> > they're > >> appropriate. >> > > Also note that the style guide says all functions should be declared >> > in class > >> files before all variables. >> > > Done. > > https://codereview.chromium.org/905023003/diff/220001/ > components/omnibox/omnibox_field_trial.h > File components/omnibox/omnibox_field_trial.h (right): > > https://codereview.chromium.org/905023003/diff/220001/ > components/omnibox/omnibox_field_trial.h#newcode277 > components/omnibox/omnibox_field_trial.h:277: // > |kHQPExperimentalScoringEnabledParam| is not defined. > On 2015/02/14 01:27:14, Mark P wrote: > >> This last sentence is wrong. This variable is a static member >> > variable and is > >> always defined/initialized. You mean something like "Returns false if >> > the > >> appropriate parameter is not specified in the field trial." >> > > analogous both places below (something like) >> Returns false if experimental scoring is not enabled or if the >> > appropriate > >> parameter is not specified in the field trial. >> > > Acknowledged. > > https://codereview.chromium.org/905023003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for informing. I made changes accordingly and moved the code to the ScoredHistoryMatchBuilderImpl. PTAL.
More comments below. Apologies these comments are on an earlier (pre-refactoring) patchset. --mark https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:592: // from HistoryURL provider) nit: period. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:595: double min_intermediate_score; If intermediate_score == 0, you end up using this value which you never explicitly initialize. Ditto with the variable below. Is that intended? Should these be explicitly initialized, or should that ">" in line 600 be a >=? Also, perhaps we should DCHECK that hqp_relevance_buckets[0].first is 0.0. Finally, I still think these two variables need a comment. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:609: return final_hqp_score; nit: simply return final_hqp_score; no need to use a temporary variable here. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:649: base::StringToDouble(it->first, &bucket.first); On 2015/02/17 01:23:52, ashokvardhan wrote: > On 2015/02/14 01:27:14, Mark P wrote: > > minor nit: consider DCHECKING the return value for these > > DCHECK seems to be not working here. > > std::string x = "1"; > int a = 0; > int b = 0; > base::StringToInt(x, &a); > DCHECK(base::StringToInt(x, &b)); > LOG(INFO) << "a is : " << a << std::endl; > LOG(INFO) << "b is : " << b << std::endl; > > Logs: > a is : 1 > b is : 0 > > Am i missing anything here ? I used CHECK in our code-base, but this is strange. > I can use if condition checkings but not sure why this is not working. DCHECKS are not executed in non-debug builds, meaning base::StringToInt(x, &b) is not executed. That's why b remains 0. I guess perhaps you can save the return type of the function, then DCHECK(that_value). https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... File chrome/browser/history/scored_history_match.h (right): https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:145: // Initializes the HQP experimental params, nit: should this be a ":" or "("/")", not a ","? What experimental params are not included in the below list? https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:147: // |hqp_relevance_buckets_| specified through omnibox field trials. You missed this earlier comment: By the way, this comment is wrong as written. In particular, you end up seeing some of these variables even if hqp_experimental_scoring is not enabled (and you rely on these settings in the code even when experimental scoring is not enabled). https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:219: // True, if we enable hqp experimental scoring. nit: True, if we enable hqp experimental scoring. -> True if hqp experimental scoring is enabled. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:224: // are given score 0. By default it is initalized to -1. "score of 0" or "topicality score of 0"? I think you meant the former. Yet, it appears that topicality score of 0 doesn't translate into a final score of 0. (With current parameters as you point out, it gets assigned a score of 400.) https://codereview.chromium.org/905023003/diff/260001/components/omnibox/omni... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/905023003/diff/260001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:281: // in case |kHQPExperimentalScoringBucketsParam| is specified in the field This sentence is wrong in two ways: (1) you meant "is not specified" (2) you missed my earlier correction that you return the empty string even if it is specified by the enabled param is not specified. This second comment also applies below.
Addressed the comments.PTAL. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... File chrome/browser/history/scored_history_match.cc (right): https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:592: // from HistoryURL provider) On 2015/02/18 00:03:32, Mark P wrote: > nit: period. Done. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:595: double min_intermediate_score; On 2015/02/18 00:03:32, Mark P wrote: > If intermediate_score == 0, you end up using this value which you never > explicitly initialize. Ditto with the variable below. Is that intended? > Should these be explicitly initialized, or should that ">" in line 600 be a >=? > > Also, perhaps we should DCHECK that hqp_relevance_buckets[0].first is 0.0. > > Finally, I still think these two variables need a comment. Yes, i have added ">=" after refactoring. You can see in the immediate-patch-set. Added comment and check. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:609: return final_hqp_score; On 2015/02/18 00:03:32, Mark P wrote: > nit: simply return final_hqp_score; no need to use a temporary variable here. Done. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.cc:649: base::StringToDouble(it->first, &bucket.first); On 2015/02/18 00:03:32, Mark P wrote: > On 2015/02/17 01:23:52, ashokvardhan wrote: > > On 2015/02/14 01:27:14, Mark P wrote: > > > minor nit: consider DCHECKING the return value for these > > > > DCHECK seems to be not working here. > > > > std::string x = "1"; > > int a = 0; > > int b = 0; > > base::StringToInt(x, &a); > > DCHECK(base::StringToInt(x, &b)); > > > LOG(INFO) << "a is : " << a << std::endl; > > LOG(INFO) << "b is : " << b << std::endl; > > > > Logs: > > a is : 1 > > b is : 0 > > > > Am i missing anything here ? I used CHECK in our code-base, but this is > strange. > > I can use if condition checkings but not sure why this is not working. > > DCHECKS are not executed in non-debug builds, meaning > base::StringToInt(x, &b) > is not executed. That's why b remains 0. I see, thanks.! > > I guess perhaps you can save the return type of the function, then > DCHECK(that_value). Done. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... File chrome/browser/history/scored_history_match.h (right): https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:145: // Initializes the HQP experimental params, On 2015/02/18 00:03:32, Mark P wrote: > nit: should this be a ":" or "("/")", not a ","? What experimental params are > not included in the below list? Done. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:147: // |hqp_relevance_buckets_| specified through omnibox field trials. On 2015/02/18 00:03:32, Mark P wrote: > You missed this earlier comment: > By the way, this comment is wrong as written. In particular, you end up seeing > some of these variables even if hqp_experimental_scoring is not enabled (and you > rely on these settings in the code even when experimental scoring is not > enabled). Done. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:219: // True, if we enable hqp experimental scoring. On 2015/02/18 00:03:32, Mark P wrote: > nit: > True, if we enable hqp experimental scoring. > -> > True if hqp experimental scoring is enabled. Done. https://codereview.chromium.org/905023003/diff/260001/chrome/browser/history/... chrome/browser/history/scored_history_match.h:224: // are given score 0. By default it is initalized to -1. On 2015/02/18 00:03:32, Mark P wrote: > "score of 0" or "topicality score of 0"? I think you meant the former. Yet, it > appears that topicality score of 0 doesn't translate into a final score of 0. > (With current parameters as you point out, it gets assigned a score of 400.) Done. https://codereview.chromium.org/905023003/diff/260001/components/omnibox/omni... File components/omnibox/omnibox_field_trial.h (right): https://codereview.chromium.org/905023003/diff/260001/components/omnibox/omni... components/omnibox/omnibox_field_trial.h:281: // in case |kHQPExperimentalScoringBucketsParam| is specified in the field On 2015/02/18 00:03:32, Mark P wrote: > This sentence is wrong in two ways: > (1) you meant "is not specified" > (2) you missed my earlier correction that you return the empty string even if it > is specified by the enabled param is not specified. > > This second comment also applies below. Done.
Almost ready. --mark https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... File chrome/browser/autocomplete/scored_history_match_builder_impl.cc (right): https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.cc:533: float final_topicality_score = topicality_score / num_terms; nit: const https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.cc:535: // Demote all the URLs if the topicality score is less than threshold. all the URLS -> the URL https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.cc:620: // score fall into. Looking at the code below, these variables are always set to the values of the "previous bucket". In that case, why don't you simply eliminate these two variables and simply use hqp_relevance_buckets[i-1] where they occur below? https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.cc:624: for (size_t i = 0; i < hqp_relevance_buckets.size(); ++i) { It ought to be safe/correct to start this loop at 1. https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... File chrome/browser/autocomplete/scored_history_match_builder_impl.h (right): https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.h:121: static void InitializeHQPExperimentalParams(); nit: all the other initialization functions in this and the .cc file are Init, not Initialize. https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... File chrome/browser/autocomplete/scored_history_match_builder_impl_unittest.cc (right): https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl_unittest.cc:475: // hqp_relevance_buckets = "0.0:100,1.0:200,4.0:500,8.0:900,10.0:1000"; optional (though encouraged) nit: test the code that converts between the scoring buckets string and the vector. (If you've tested it thoroughly by hand, it might be okay to skip it.)
Please take a look. https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... File chrome/browser/autocomplete/scored_history_match_builder_impl.cc (right): https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.cc:533: float final_topicality_score = topicality_score / num_terms; On 2015/02/19 20:47:00, Mark P wrote: > nit: const Done. https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.cc:535: // Demote all the URLs if the topicality score is less than threshold. On 2015/02/19 20:47:00, Mark P wrote: > all the URLS > -> > the URL Done. https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.cc:620: // score fall into. On 2015/02/19 20:47:00, Mark P wrote: > Looking at the code below, these variables are always set to the values of the > "previous bucket". In that case, why don't you simply eliminate these two > variables and simply use hqp_relevance_buckets[i-1] where they occur below? I prefer using min_* variables as they are more readable and i-1 notations. But i am fine both the notations. https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.cc:624: for (size_t i = 0; i < hqp_relevance_buckets.size(); ++i) { On 2015/02/19 20:47:00, Mark P wrote: > It ought to be safe/correct to start this loop at 1. Done. https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... File chrome/browser/autocomplete/scored_history_match_builder_impl.h (right): https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl.h:121: static void InitializeHQPExperimentalParams(); On 2015/02/19 20:47:00, Mark P wrote: > nit: all the other initialization functions in this and the .cc file are Init, > not Initialize. Done. https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... File chrome/browser/autocomplete/scored_history_match_builder_impl_unittest.cc (right): https://codereview.chromium.org/905023003/diff/320001/chrome/browser/autocomp... chrome/browser/autocomplete/scored_history_match_builder_impl_unittest.cc:475: // hqp_relevance_buckets = "0.0:100,1.0:200,4.0:500,8.0:900,10.0:1000"; On 2015/02/19 20:47:00, Mark P wrote: > optional (though encouraged) nit: test the code that converts between the > scoring buckets string and the vector. > > (If you've tested it thoroughly by hand, it might be okay to skip it.) Done.
lgtm
The CQ bit was checked by ashokvardhan@chromium.org
The CQ bit was unchecked by ashokvardhan@chromium.org
The CQ bit was checked by ashokvardhan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/905023003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ashokvardhan@chromium.org
The CQ bit was unchecked by ashokvardhan@chromium.org
The CQ bit was checked by ashokvardhan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/905023003/360001
New patchsets have been uploaded after l-g-t-m from mpearson@chromium.org
The CQ bit was checked by ashokvardhan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/905023003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ashokvardhan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/905023003/380001
Message was sent while issue was closed.
Committed patchset #12 (id:380001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/0b06844f4d6e6a9a3b42438493c52d248397011d Cr-Commit-Position: refs/heads/master@{#317369} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
