Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541143002:
Omnibox - Boost Frequency Scores Based on Number of Matching Pages (Closed)
|
Created:
4 years ago by Mark P Modified:
4 years ago Reviewers:
Peter Kasting CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox - Boost Frequency Scores Based on Number of Matching Pages
This changelist adds a knob, controlled by an experiment, that
enables boosting the frequency scores in HistoryQuick provider
based on the number of documents matching the user's input.
The intuition is that if a user's input matches a single or
tiny number of documents from the user's history, then that input
is probably seeking those pages and they should be scored more
aggressively. Boosting their frequency score (in effect the
document quality / popularity score) makes more sense to me
than boosting the topicality score or altering the final function
that translates the frequency and topicality score into a final
score.
This should be the last changelist I need. Next up is
to use the knobs to find reasonable parameters and run some
experiments.
BUG=369989
Committed: https://crrev.com/33a75da3fddb8b626a76e0ac586452f659193e67
Cr-Commit-Position: refs/heads/master@{#438290}
Patch Set 1 #Patch Set 2 : remove setup/teardown test case code #
Total comments: 29
Patch Set 3 : dramatically redone. compiles. rewrote tests. all pass. #Patch Set 4 : improved comments and formatting #
Total comments: 10
Patch Set 5 : pkasting comments; still didn't handle leaky variable #Patch Set 6 : fix leak #
Total comments: 8
Patch Set 7 : peter's comments #Patch Set 8 : rebase; didn't even compile yet #Patch Set 9 : fix rebase errors #
Messages
Total messages: 24 (7 generated)
Description was changed from ========== Omnibox - Boost Frequency Scores Based on Number of Matching Pages This changelist adds a knob, controlled by an experiment, that enables boosting the frequency scores in HistoryQuick provider based on the number of documents matching the user's input. The intuition is that if a user's input matches a single or tiny number of documents from the user's history, then that input is probably seeking those pages and they should be scored more aggressively. Boosting their frequency score (in effect the document quality / popularity score) makes more sense to me than boosting the topicality score or altering the final function that translates the frequency and topicality score into a final score. BUG=369989 ========== to ========== Omnibox - Boost Frequency Scores Based on Number of Matching Pages This changelist adds a knob, controlled by an experiment, that enables boosting the frequency scores in HistoryQuick provider based on the number of documents matching the user's input. The intuition is that if a user's input matches a single or tiny number of documents from the user's history, then that input is probably seeking those pages and they should be scored more aggressively. Boosting their frequency score (in effect the document quality / popularity score) makes more sense to me than boosting the topicality score or altering the final function that translates the frequency and topicality score into a final score. This should be the last changelist I need. Next up is to use the knobs to find reasonable parameters and run some experiments. BUG=369989 ==========
mpearson@chromium.org changed reviewers: + pkasting@chromium.org
Peter, Please take a look at your convenience. No hurry. thanks, mark
https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:451: // e.g., "1:3,2:2.5,3:2,4:1.5" Super nitpicky nit: "pairs, e.g. "..."." (comma rather than period after "pairs", no period after "e.g.", period at end of sentence) https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:453: // downloaded from the server to be perfect. There's no need for handle Nit: for -> to https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:459: it != kv_pairs.end(); ++it) { Nit: Range-based for? https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:465: num_matches_multipliers->end()); Nit: DCHECK(!base::ContainsKey(num_matches_multipliers, num)) ? https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:110: typedef base::hash_map<size_t, double> NumMatchesMultipliers; I don't know that I love the data structure choice here. Leaving aside whether a map vs. a hash_map makes sense, why not a simple vector of pair<size_t, double> in sorted order? If you expect the size to be a couple of elements lookups will probably be slightly faster than either map type; if it will be larger, you could use a binary search to match the algorithmic efficiency of a map but with better cache behavior; and most importantly, the nature of the data structure implies that it's tracking ranges rather than exact values, so if you just send down "1:2,3:1.5", it implies that 2 should be mapped to 2.0, which makes more sense than mapping it to 1.0 as the hash_map suggests. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:321: NumMatchesMultipliers* num_matches_multipliers); Nit: You could probably return the multipliers by value rather than by outpointer, since this will be done once. (I tend to find such code easier to read.) https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:420: new OmniboxFieldTrial::NumMatchesMultipliers(); This leaks. If you want a leaky object of class type, prefer something like CR_DEFINE_STATIC_LOCAL. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:635: return frequency; I don't love putting this here. Part of this is complexity. The function is already complicated enough and the code that was at the end was easier to read before adding the temp to store the intermediate result in. From a readability perspective, it seems like wrapping this in a helper that does the adjustment, or simply doing it on the caller side, might be nice. Part of this is testability. Long-term, if we want to do this all the time, it seems like it might be nice to accurately test "find the real frequency" separately from the boosting here. And part is readability. I find it confusing that you implemented this by boosting frequency early instead of making an adjustment when calculating the final score. Conceptually, why not claim the final algorithm is "frequency + recency + relevance" (I don't mean "add" by "+", I just mean those are the relevant inputs) and have this be the way we compute the "relevance" factor? In terms of concrete implementation, you could do it however you want to, I am just interested in where the computation happens. And even if we do this by just tweaking the frequency number early, a function called "GetFrequency()" and a separate one like "GetRelevanceAdjustedFrequency()" or some other similar name that makes it clear what it does and what it returns seems like it would be less confusing to understand. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:59: size_t num_matching_pages, Nit: There are a lot of args here and the unittests pass "1" a lot. Do default args, or alternate constructors that take fewer args, or just a helper in the unittest to construct these, make sense? https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:182: // Omitted values are assumed to have multipliers of 1.0. Nit: I would omit this last sentence and possibly refer the user to the documentation on the type itself (if need be) https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match_unittest.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match_unittest.cc:510: // With different parameters, however, it can change. In this case, the Nit: parameters -> match multiplier parameters? (Or something; "parameters" seems vague.) https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match_unittest.cc:518: } You test this three times. Helper function? Do the further tests actually test what you want them to (i.e. that the two behavior fixes don't interact with this modifier list) given that you use vague tests like EXPECT_GT instead of checking exact values? Maybe they're not really valuable and should just go away? https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:724: &now](const HistoryID history_id) { Nit: Bleh, I'm inclined to replace this huge list of captures with just [&].
Inspired by your suggestions, I rewrote this whole patch in a way that's more understandable and more testable. Please take a look at your leisure. There is one open issue (leaky static member variable) on my TODO, but I'm sending this now because I won't be able to do anything on this until the latter half of next week. Rather than sit on it til then, I'll throw it in your court. There are enough changes that it might be worthwhile to re-review from scratch. The intermediate diffs will be large. cheers, mark https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:451: // e.g., "1:3,2:2.5,3:2,4:1.5" On 2016/12/01 07:07:53, Peter Kasting wrote: > Super nitpicky nit: > > "pairs, e.g. "..."." > > (comma rather than period after "pairs", no period after "e.g.", period at end > of sentence) Done. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:453: // downloaded from the server to be perfect. There's no need for handle On 2016/12/01 07:07:53, Peter Kasting wrote: > Nit: for -> to Done. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:459: it != kv_pairs.end(); ++it) { On 2016/12/01 07:07:53, Peter Kasting wrote: > Nit: Range-based for? Now moot. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:465: num_matches_multipliers->end()); On 2016/12/01 07:07:53, Peter Kasting wrote: > Nit: DCHECK(!base::ContainsKey(num_matches_multipliers, num)) ? Did something similar. (Between this duplicates-the-immediately-previous-item check and the is_sorted check, I think everything is covered.) https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:110: typedef base::hash_map<size_t, double> NumMatchesMultipliers; On 2016/12/01 07:07:53, Peter Kasting wrote: > I don't know that I love the data structure choice here. Leaving aside whether > a map vs. a hash_map makes sense, why not a simple vector of pair<size_t, > double> in sorted order? If you expect the size to be a couple of elements > lookups will probably be slightly faster than either map type; if it will be > larger, you could use a binary search to match the algorithmic efficiency of a > map but with better cache behavior; and most importantly, the nature of the data > structure implies that it's tracking ranges rather than exact values, so if you > just send down "1:2,3:1.5", it implies that 2 should be mapped to 2.0, which > makes more sense than mapping it to 1.0 as the hash_map suggests. Sure. I debated data structures with myself. Happy to change to the other. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:321: NumMatchesMultipliers* num_matches_multipliers); On 2016/12/01 07:07:53, Peter Kasting wrote: > Nit: You could probably return the multipliers by value rather than by > outpointer, since this will be done once. (I tend to find such code easier to > read.) Okay. I guess it's small enough to copy off the stack. (I assume that's what gets done.) I agree with you on readability. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:420: new OmniboxFieldTrial::NumMatchesMultipliers(); On 2016/12/01 07:07:53, Peter Kasting wrote: > This leaks. If you want a leaky object of class type, prefer something like > CR_DEFINE_STATIC_LOCAL. Still TODO. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:635: return frequency; On 2016/12/01 07:07:53, Peter Kasting wrote: > I don't love putting this here. I agree with your reasoning below. After some thought, I realized it shouldn't be part of the frequency score at all. I called this new factor a "document specificity score" and added it as a third factor (in addition to frequency and topicality) to go into the final relevancy calculation. This makes a lot more conceptual sense, makes the code cleaner and more readable and more testable. Win all around. > Part of this is complexity. The function is already complicated enough and the > code that was at the end was easier to read before adding the temp to store the > intermediate result in. From a readability perspective, it seems like wrapping > this in a helper that does the adjustment, or simply doing it on the caller > side, might be nice. > > Part of this is testability. Long-term, if we want to do this all the time, it > seems like it might be nice to accurately test "find the real frequency" > separately from the boosting here. > > And part is readability. I find it confusing that you implemented this by > boosting frequency early instead of making an adjustment when calculating the > final score. Conceptually, why not claim the final algorithm is "frequency + > recency + relevance" (I don't mean "add" by "+", I just mean those are the > relevant inputs) and have this be the way we compute the "relevance" factor? In > terms of concrete implementation, you could do it however you want to, I am just > interested in where the computation happens. > > And even if we do this by just tweaking the frequency number early, a function > called "GetFrequency()" and a separate one like > "GetRelevanceAdjustedFrequency()" or some other similar name that makes it clear > what it does and what it returns seems like it would be less confusing to > understand. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:59: size_t num_matching_pages, On 2016/12/01 07:07:53, Peter Kasting wrote: > Nit: There are a lot of args here and the unittests pass "1" a lot. Do default > args, or alternate constructors that take fewer args, or just a helper in the > unittest to construct these, make sense? Almost all of these args are requires and need reasonable values consistent with other values. The only exceptions are |is_url_bookmarked| and |num_matching_pages|. Aside from the unittest, we only construct these classes in one place. I don't think having an alternate constructor or default args will buy us anything. I can imagine doing something in the unit tests, though I appreciate unittests being easily readable and clear about how parameters are set. In other words, I'd like to use the full constructor, not a helper function, in the unit tests, especially because it only lets us get rid of two args. The constructors will remain long; the extra level of indirection isn't worth it in my opinion. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:182: // Omitted values are assumed to have multipliers of 1.0. On 2016/12/01 07:07:53, Peter Kasting wrote: > Nit: I would omit this last sentence and possibly refer the user to the > documentation on the type itself (if need be) Did both. Now with the restructuring of the data type, I think the reference "need be". https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match_unittest.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match_unittest.cc:510: // With different parameters, however, it can change. In this case, the On 2016/12/01 07:07:53, Peter Kasting wrote: > Nit: parameters -> match multiplier parameters? (Or something; "parameters" > seems vague.) Now moot. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match_unittest.cc:518: } On 2016/12/01 07:07:53, Peter Kasting wrote: > You test this three times. Helper function? > > Do the further tests actually test what you want them to (i.e. that the two > behavior fixes don't interact with this modifier list) given that you use vague > tests like EXPECT_GT instead of checking exact values? Maybe they're not really > valuable and should just go away? Now moot. https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:724: &now](const HistoryID history_id) { On 2016/12/01 07:07:53, Peter Kasting wrote: > Nit: Bleh, I'm inclined to replace this huge list of captures with just [&]. Done.
The only real issue I see is the leaking object. Beyond that, and the below stuff, LGTM https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:463: (num_matches_scores[i].first != num_matches_scores[i - 1].first)); Wouldn't using ">" here obviate the need for the is_sorted() call below? Or, instead, you could eliminate this DCHECK entirely and replace that with: DCHECK(std::is_sorted(num_matches_scores.begin(), num_matches_scores.end(), [](a, b) { return a.first < b.first; })); Note that here a and b will need type declarations of your pair type. It might be easier to use a type alias in the header for this. Doing this would also allow switching the loop to use range-based for. https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:631: // The floating point value below doesn't matter. I'm not sure that's true. Since upper_bound() returns the first element greater than the given value, if the server sends a score of 1.0 or less for a particular number of pages, this will fail to match. A more accurate comment would probably be "The floating point value below must be less than the lowest score the server would send down" or similar, and a safer value might be -1. https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:638: return it->second; Nit: Could use ?: https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:138: float GetDocumentSpecificityScore(const size_t num_matching_pages) const; I'd avoid declaring the parameter here const.
https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:420: new OmniboxFieldTrial::NumMatchesMultipliers(); On 2016/12/01 07:07:53, Peter Kasting wrote: > This leaks. If you want a leaky object of class type, prefer something like > CR_DEFINE_STATIC_LOCAL. I don't understand how this macro should be used in this context. That macro talks about static local variables, not static class member variables. From some code searching, I couldn't easily find examples of use in the class context. Can you more clearly explain what you want me to do? (Is that the right macro you wanted to point me to?)
https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:420: new OmniboxFieldTrial::NumMatchesMultipliers(); On 2016/12/06 21:02:47, Mark P wrote: > On 2016/12/01 07:07:53, Peter Kasting wrote: > > This leaks. If you want a leaky object of class type, prefer something like > > CR_DEFINE_STATIC_LOCAL. > > I don't understand how this macro should be used in this context. That macro > talks about static local variables, not static class member variables. From > some code searching, I couldn't easily find examples of use in the class > context. Can you more clearly explain what you want me to do? (Is that the > right macro you wanted to point me to?) There are two ways to do this I can think of: (1) Use CR_DEFINE_STATIC_LOCAL in the method that returns the correct document specificity score. Provide a (static or not) class member pointer that's normally set to the address of this static. Tests can temporarily override this pointer to point at a different vector. The method then just unconditionally dereferences the pointer. (2) Use a base::LazyInstance in the method or as a static class member. The rest of (1) still applies. This will be slightly trickier since you need a separate static local bool "initialized" or similar to guard the code that sets up the vector contents the first time it's accessed. I'd do (1).
This is ready to go except for waiting of resolution of https://codereview.chromium.org/2548363010/ and the corresponding revision of this changelist to resolve the leaky variable. --mark https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/20001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:420: new OmniboxFieldTrial::NumMatchesMultipliers(); On 2016/12/06 21:41:27, Peter Kasting wrote: > On 2016/12/06 21:02:47, Mark P wrote: > > On 2016/12/01 07:07:53, Peter Kasting wrote: > > > This leaks. If you want a leaky object of class type, prefer something like > > > CR_DEFINE_STATIC_LOCAL. > > > > I don't understand how this macro should be used in this context. That macro > > talks about static local variables, not static class member variables. From > > some code searching, I couldn't easily find examples of use in the class > > context. Can you more clearly explain what you want me to do? (Is that the > > right macro you wanted to point me to?) > > There are two ways to do this I can think of: > > (1) Use CR_DEFINE_STATIC_LOCAL in the method that returns the correct document > specificity score. Provide a (static or not) class member pointer that's > normally set to the address of this static. Tests can temporarily override this > pointer to point at a different vector. The method then just unconditionally > dereferences the pointer. > > (2) Use a base::LazyInstance in the method or as a static class member. The > rest of (1) still applies. This will be slightly trickier since you need a > separate static local bool "initialized" or similar to guard the code that sets > up the vector contents the first time it's accessed. > > I'd do (1). Thanks for the explanation. I'll do something like (1). It'll first appear in another changelist (where I have to make a similar change). Once that changelist is acceptable, I'll make the analogous change here. https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:463: (num_matches_scores[i].first != num_matches_scores[i - 1].first)); On 2016/12/06 05:19:30, Peter Kasting wrote: > Wouldn't using ">" here obviate the need for the is_sorted() call below? > > Or, instead, you could eliminate this DCHECK entirely and replace that with: > > DCHECK(std::is_sorted(num_matches_scores.begin(), num_matches_scores.end(), > [](a, b) { return a.first < b.first; })); > > Note that here a and b will need type declarations of your pair type. It might > be easier to use a type alias in the header for this. > > Doing this would also allow switching the loop to use range-based for. Of these two suggestions, I prefer the first one. Did the > and removed the is_sorted() call. https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:631: // The floating point value below doesn't matter. On 2016/12/06 05:19:30, Peter Kasting wrote: > I'm not sure that's true. > > Since upper_bound() returns the first element greater than the given value, if > the server sends a score of 1.0 or less for a particular number of pages, this > will fail to match. > > A more accurate comment would probably be "The floating point value below must > be less than the lowest score the server would send down" or similar, and a > safer value might be -1. Good point. I never imagined a field trial specifying a score of less than 1, but having an appropriate comment and defensive code makes sense. Who knows what I'll want in the future? :-) Done. https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:638: return it->second; On 2016/12/06 05:19:30, Peter Kasting wrote: > Nit: Could use ?: Done (after reversing the order, as that read better to me). https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.h:138: float GetDocumentSpecificityScore(const size_t num_matching_pages) const; On 2016/12/06 05:19:30, Peter Kasting wrote: > I'd avoid declaring the parameter here const. Done.
https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:638: return it->second; On 2016/12/08 00:21:31, Mark P wrote: > On 2016/12/06 05:19:30, Peter Kasting wrote: > > Nit: Could use ?: > > Done (after reversing the order, as that read better to me). My concern with the order reversal is that it sort of violates "don't have 'else' with a negated condition, or the 'else' reads like a double-negative". Except here the "else" is the ':'. So I'd put it back the other way, but up to you.
https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/60001/components/omnibox/brow... components/omnibox/browser/scored_history_match.cc:638: return it->second; On 2016/12/08 00:51:40, Peter Kasting wrote: > On 2016/12/08 00:21:31, Mark P wrote: > > On 2016/12/06 05:19:30, Peter Kasting wrote: > > > Nit: Could use ?: > > > > Done (after reversing the order, as that read better to me). > > My concern with the order reversal is that it sort of violates "don't have > 'else' with a negated condition, or the 'else' reads like a double-negative". > Except here the "else" is the ':'. > > So I'd put it back the other way, but up to you. I understand the point, but I find != end to read to me like a positive (exists), and when I read == end, I have to mentally reverse it (!exists).
Fixed the leaky static issue, which I believe was the final thing remaining on this changelist. Please take a look. thanks, mark
LGTM https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... components/omnibox/browser/scored_history_match.cc:639: : &default_num_matches_to_document_specificity_score; I feel like there might be a way to shorten all the names here and not have the same text repeat over and over... |matches_to_specificity|? I dunno. https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... components/omnibox/browser/scored_history_match.h:214: // Used for testing. If this point is not null, it overrides the static Nit: pointer https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... File components/omnibox/browser/scored_history_match_unittest.cc (right): https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... components/omnibox/browser/scored_history_match_unittest.cc:599: OmniboxFieldTrial::NumMatchesScores num_matches_to_document_specificity_score; Nit: Might want a blank line here since the lack of one implies this variable is only used for this block. https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... components/omnibox/browser/scored_history_match_unittest.cc:602: base::AutoReset<OmniboxFieldTrial::NumMatchesScores*> tmp( Nit: If you're going to declare the override outside these blocks, might as well hoist the AutoReset object there too. Then you only need it once and the blocks are maximally readable.
https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... File components/omnibox/browser/scored_history_match.cc (right): https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... components/omnibox/browser/scored_history_match.cc:639: : &default_num_matches_to_document_specificity_score; On 2016/12/10 02:22:26, Peter Kasting wrote: > I feel like there might be a way to shorten all the names here and not have the > same text repeat over and over... |matches_to_specificity|? I dunno. I was wondering if there was a good way to shorten the name that we'd both be comfortable with. It wasn't obvious. Thank you for coming up with one. Done. https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... File components/omnibox/browser/scored_history_match.h (right): https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... components/omnibox/browser/scored_history_match.h:214: // Used for testing. If this point is not null, it overrides the static On 2016/12/10 02:22:26, Peter Kasting wrote: > Nit: pointer Done. https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... File components/omnibox/browser/scored_history_match_unittest.cc (right): https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... components/omnibox/browser/scored_history_match_unittest.cc:599: OmniboxFieldTrial::NumMatchesScores num_matches_to_document_specificity_score; On 2016/12/10 02:22:26, Peter Kasting wrote: > Nit: Might want a blank line here since the lack of one implies this variable is > only used for this block. Done. https://codereview.chromium.org/2541143002/diff/100001/components/omnibox/bro... components/omnibox/browser/scored_history_match_unittest.cc:602: base::AutoReset<OmniboxFieldTrial::NumMatchesScores*> tmp( On 2016/12/10 02:22:26, Peter Kasting wrote: > Nit: If you're going to declare the override outside these blocks, might as well > hoist the AutoReset object there too. Then you only need it once and the blocks > are maximally readable. Done. (I forgot that I could modify the replacement variable multiple times and have everything work.)
rebased on top of the recently-submitted changelist. Feel free to take a look if you want. (I don't think one is necessary and will submit this if I don't hear from you.) --mark
DIDN'T READ LOL (Go for it)
The CQ bit was checked by mpearson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2541143002/#ps160001 (title: "fix rebase errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481657954612160,
"parent_rev": "86e7cfb92d57193f8d0b0682573da966acc4f849", "commit_rev":
"b81d77098bd420bf7e23ca1ce18f0b23dcdb5aa6"}
Message was sent while issue was closed.
Description was changed from ========== Omnibox - Boost Frequency Scores Based on Number of Matching Pages This changelist adds a knob, controlled by an experiment, that enables boosting the frequency scores in HistoryQuick provider based on the number of documents matching the user's input. The intuition is that if a user's input matches a single or tiny number of documents from the user's history, then that input is probably seeking those pages and they should be scored more aggressively. Boosting their frequency score (in effect the document quality / popularity score) makes more sense to me than boosting the topicality score or altering the final function that translates the frequency and topicality score into a final score. This should be the last changelist I need. Next up is to use the knobs to find reasonable parameters and run some experiments. BUG=369989 ========== to ========== Omnibox - Boost Frequency Scores Based on Number of Matching Pages This changelist adds a knob, controlled by an experiment, that enables boosting the frequency scores in HistoryQuick provider based on the number of documents matching the user's input. The intuition is that if a user's input matches a single or tiny number of documents from the user's history, then that input is probably seeking those pages and they should be scored more aggressively. Boosting their frequency score (in effect the document quality / popularity score) makes more sense to me than boosting the topicality score or altering the final function that translates the frequency and topicality score into a final score. This should be the last changelist I need. Next up is to use the knobs to find reasonable parameters and run some experiments. BUG=369989 Review-Url: https://codereview.chromium.org/2541143002 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Omnibox - Boost Frequency Scores Based on Number of Matching Pages This changelist adds a knob, controlled by an experiment, that enables boosting the frequency scores in HistoryQuick provider based on the number of documents matching the user's input. The intuition is that if a user's input matches a single or tiny number of documents from the user's history, then that input is probably seeking those pages and they should be scored more aggressively. Boosting their frequency score (in effect the document quality / popularity score) makes more sense to me than boosting the topicality score or altering the final function that translates the frequency and topicality score into a final score. This should be the last changelist I need. Next up is to use the knobs to find reasonable parameters and run some experiments. BUG=369989 Review-Url: https://codereview.chromium.org/2541143002 ========== to ========== Omnibox - Boost Frequency Scores Based on Number of Matching Pages This changelist adds a knob, controlled by an experiment, that enables boosting the frequency scores in HistoryQuick provider based on the number of documents matching the user's input. The intuition is that if a user's input matches a single or tiny number of documents from the user's history, then that input is probably seeking those pages and they should be scored more aggressively. Boosting their frequency score (in effect the document quality / popularity score) makes more sense to me than boosting the topicality score or altering the final function that translates the frequency and topicality score into a final score. This should be the last changelist I need. Next up is to use the knobs to find reasonable parameters and run some experiments. BUG=369989 Committed: https://crrev.com/33a75da3fddb8b626a76e0ac586452f659193e67 Cr-Commit-Position: refs/heads/master@{#438290} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/33a75da3fddb8b626a76e0ac586452f659193e67 Cr-Commit-Position: refs/heads/master@{#438290} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
