Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
55413002:
Don't demote top match for certain match types. (Closed)
|
Created:
7 years, 1 month ago by H Fung Modified:
7 years, 1 month ago CC:
chromium-reviews, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDon't demote top match for certain match types.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232979
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Add tests, fix bugs. #
Total comments: 4
Patch Set 3 : Mark's comments #
Total comments: 6
Patch Set 4 : Simplify code, modify comments. #
Total comments: 13
Patch Set 5 : Fix bug depending on sorting. #Patch Set 6 : Peter's comments #
Messages
Total messages: 17 (0 generated)
This is what I have so far to not demote the top suggestion as discussed earlier this week. I'm sending this out now to see if there are objections to the approach. I'll add tests tomorrow.
https://codereview.chromium.org/55413002/diff/20001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/55413002/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_result.cc:164: matches_.begin()->demoteable = false; Is this basically only used by code called from the block below? Because if so, rather than add a new member to AutocompleteMatch, perhaps we could simply pull off the front match here, run the sort/demote on the rest, and then put the front match back.
I've added tests now. PTAL. https://codereview.chromium.org/55413002/diff/20001/chrome/browser/autocomple... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/55413002/diff/20001/chrome/browser/autocomple... chrome/browser/autocomplete/autocomplete_result.cc:164: matches_.begin()->demoteable = false; On 2013/10/31 23:27:59, Peter Kasting wrote: > Is this basically only used by code called from the block below? > > Because if so, rather than add a new member to AutocompleteMatch, perhaps we > could simply pull off the front match here, run the sort/demote on the rest, and > then put the front match back. Yes, thanks. I've made the change.
https://codereview.chromium.org/55413002/diff/110001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/55413002/diff/110001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:156: size_t max_num_matches = std::min(kMaxMatches, matches_.size()); Can you put this "don't demote top match under certain conditions" logic in the ComparingObject somehow? The reason for my request is that ComparingObject is used in two places. You've corrected to make one not do the demotion. I'm not sure if you noticed the other place, a place you didn't change. See line 334. I'd prefer the logic in ComparingObject but if you can't manage to do that you can put logic as necessary around line 334. (Logic may or may not actually be necessary; I haven't thought about it. Regardless, something needs to go there, if only a comment explaining why nothing is needed there.) https://codereview.chromium.org/55413002/diff/110001/chrome/browser/omnibox/o... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/55413002/diff/110001/chrome/browser/omnibox/o... chrome/browser/omnibox/omnibox_field_trial.cc:276: std::vector<std::string> types; Please explicitly comment on the format for the value of the rule and what the value means, a la lines 247-251.
https://codereview.chromium.org/55413002/diff/110001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/55413002/diff/110001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:156: size_t max_num_matches = std::min(kMaxMatches, matches_.size()); On 2013/11/04 18:47:56, Mark P wrote: > Can you put this "don't demote top match under certain conditions" logic in the > ComparingObject somehow? I think that would require CompareWithDemoteByType to know whether AutocompleteMatch was the first match. I originally added an extra variable in AutocompleteMatch but Peter suggested removing it. (I think both solutions are kind of messy) > The reason for my request is that ComparingObject is used in two places. You've > corrected to make one not do the demotion. I'm not sure if you noticed the > other place, a place you didn't change. See line 334. > > I'd prefer the logic in ComparingObject but if you can't manage to do that you > can put logic as necessary around line 334. (Logic may or may not actually be > necessary; I haven't thought about it. Regardless, something needs to go there, > if only a comment explaining why nothing is needed there.) Comment added. I only found one non-test where AddMatch is called (CopyOldMatches calls MergeMatchesByProvider calls AddMatch, and CopyOldMatches calls SortAndCull afterwards) https://codereview.chromium.org/55413002/diff/110001/chrome/browser/omnibox/o... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/55413002/diff/110001/chrome/browser/omnibox/o... chrome/browser/omnibox/omnibox_field_trial.cc:276: std::vector<std::string> types; On 2013/11/04 18:47:56, Mark P wrote: > Please explicitly comment on the format for the value of the rule and what the > value means, a la lines 247-251. Done.
once you fix comments below (no need to wait for me to look at those fixes), lgtm, but please wait for Peter's okay too. --mark https://codereview.chromium.org/55413002/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/55413002/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:164: undemotable_top_types.end(); nit: parens around != please https://codereview.chromium.org/55413002/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:337: // this function is called. This comment made me realize the comment in autocomplet_result.h by the declaration of AddMatch() is wrong: // Adds a single match. The match is inserted at the appropriate position // based on relevancy and display order. This is ONLY for use after // SortAndCull() has been invoked, and preserves default_match_. Can you fix it and replace the last sentence of that comment with this sentence: // It preserves default_match_. (after all, it's never used after SortAndCull, only before) As for the comment here, how about replacing your comment with this? (or combining your comment with this suggestion:) // GetUndemotableTopTypes() is not used here because we're only worried about preserving the top result when calculating the default match, which happens in SortAndCull(). Because AddMatch() does not change the default match, there is no need think about undemotable types here. https://codereview.chromium.org/55413002/diff/300001/chrome/browser/omnibox/o... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/55413002/diff/300001/chrome/browser/omnibox/o... chrome/browser/omnibox/omnibox_field_trial.cc:277: // AutocompleteMatchType::Type enums represented as an integer. Perhaps add something about demotion scores (refer to GetDemotionsByType) don't get applied to a top match if the top match is of a type on this list.
Thanks Mark for the quick comments. I'm not sure your suggested changes to comments are all correct, though. I also simplified the code that preserves the top match. PTAL. https://codereview.chromium.org/55413002/diff/300001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/55413002/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:164: undemotable_top_types.end(); On 2013/11/04 20:30:12, Mark P wrote: > nit: parens around != please Done. https://codereview.chromium.org/55413002/diff/300001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:337: // this function is called. On 2013/11/04 20:30:12, Mark P wrote: > This comment made me realize the comment in autocomplet_result.h by the > declaration of AddMatch() is wrong: > // Adds a single match. The match is inserted at the appropriate position > // based on relevancy and display order. This is ONLY for use after > // SortAndCull() has been invoked, and preserves default_match_. > Can you fix it and replace the last sentence of that comment with this sentence: > // It preserves default_match_. > (after all, it's never used after SortAndCull, only before) Actually, SortAndCull() is called both before and after AddMatch(). AutocompleteController::UpdateResult() calls SortAndCull() before CopyOldMatches() (which calls MergeMatchesByProvider() which calls AddMatch). The only other caller to CopyOldMatches() is a test. > As for the comment here, how about replacing your comment with this? > (or combining your comment with this suggestion:) > // GetUndemotableTopTypes() is not used here because we're only worried about > preserving the top result when calculating the default match, which happens in > SortAndCull(). Because AddMatch() does not change the default match, there is > no need think about undemotable types here. I'm not sure that's true. What |default_match_| points to is not changed here (even if the match is added to the top position), but |default_match_| can get reset in SortAndCull(). https://codereview.chromium.org/55413002/diff/300001/chrome/browser/omnibox/o... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/55413002/diff/300001/chrome/browser/omnibox/o... chrome/browser/omnibox/omnibox_field_trial.cc:277: // AutocompleteMatchType::Type enums represented as an integer. On 2013/11/04 20:30:12, Mark P wrote: > Perhaps add something about demotion scores (refer to GetDemotionsByType) don't > get applied to a top match if the top match is of a type on this list. Done.
lgtm, but again please wait for Peter. (I'll leave it up to him to debate the best explanation for those comments.) BTW, your code simplification in SortAndCull() is very nice. --mark
After some more testing, I realized there was a bug and uploaded a fix and modified a test. I was relying on the matches to be sorted before hand (it was, but it was using destination URL). On 2013/11/04 21:50:42, Mark P wrote: > lgtm, but again please wait for Peter. (I'll leave it up to him to debate the > best explanation for those comments.) > > BTW, your code simplification in SortAndCull() is very nice. Thanks. (It's partially because the original implementation wasn't very good. :) > > --mark
LGTM https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:162: (undemotable_top_types.find(matches_.begin()->type) != Nit: Instead of using find(), use count() and check for the result being nonzero. https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:168: std::sort(preserve_top_match ? matches_.begin() + 1 : matches_.begin(), Nit: Slightly briefer: matches.begin() + (preserve_top_match ? 1 : 0) https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:383: { Nit: Rather than these sub-blocks, you could simply create |matches| as a 3-element vector, then modify matches[0], matches[1], and matches[2]. This would be both briefer and slightly more efficient (not that that matters...) https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:412: "1:50"; // 3 == HOME_PAGE Nit: Since the '3' is not on the same physical line as the comment, this might actually be better as a single comment above these two statements. Or just don't comment them -- if someone doesn't know what the "3" is, the ":" and ":*" and "1:50" and such aren't going to be clear either. https://codereview.chromium.org/55413002/diff/420001/chrome/browser/omnibox/o... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/55413002/diff/420001/chrome/browser/omnibox/o... chrome/browser/omnibox/omnibox_field_trial.cc:268: void OmniboxFieldTrial::GetUndemotableTopTypes( This should return by value rather than outparam. I assume you're trying to use outparams for efficiency reasons, but in this case the RVO should eliminate any performance difference, and this function reads better and would have simpler callsites with return-by-value. https://codereview.chromium.org/55413002/diff/420001/chrome/browser/omnibox/o... chrome/browser/omnibox/omnibox_field_trial.cc:285: // downloaded from the server to be perfect. There's no need for handle Nit: for -> to (and fix above code also)
Thanks for the reviews! https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:162: (undemotable_top_types.find(matches_.begin()->type) != On 2013/11/05 04:09:55, Peter Kasting wrote: > Nit: Instead of using find(), use count() and check for the result being > nonzero. Done. Actually, should ContainsKey() be used instead? I won't do it now since I don't see it used too often and might commit this change soon. https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:168: std::sort(preserve_top_match ? matches_.begin() + 1 : matches_.begin(), On 2013/11/05 04:09:55, Peter Kasting wrote: > Nit: Slightly briefer: matches.begin() + (preserve_top_match ? 1 : 0) Done. https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right): https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:383: { On 2013/11/05 04:09:55, Peter Kasting wrote: > Nit: Rather than these sub-blocks, you could simply create |matches| as a > 3-element vector, then modify matches[0], matches[1], and matches[2]. This > would be both briefer and slightly more efficient (not that that matters...) Done. https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result_unittest.cc:412: "1:50"; // 3 == HOME_PAGE On 2013/11/05 04:09:55, Peter Kasting wrote: > Nit: Since the '3' is not on the same physical line as the comment, this might > actually be better as a single comment above these two statements. Or just > don't comment them -- if someone doesn't know what the "3" is, the ":" and ":*" > and "1:50" and such aren't going to be clear either. Done. I kept the comment since HOME_PAGE is used below. https://codereview.chromium.org/55413002/diff/420001/chrome/browser/omnibox/o... File chrome/browser/omnibox/omnibox_field_trial.cc (right): https://codereview.chromium.org/55413002/diff/420001/chrome/browser/omnibox/o... chrome/browser/omnibox/omnibox_field_trial.cc:268: void OmniboxFieldTrial::GetUndemotableTopTypes( On 2013/11/05 04:09:55, Peter Kasting wrote: > This should return by value rather than outparam. I assume you're trying to use > outparams for efficiency reasons, but in this case the RVO should eliminate any > performance difference, and this function reads better and would have simpler > callsites with return-by-value. Done. https://codereview.chromium.org/55413002/diff/420001/chrome/browser/omnibox/o... chrome/browser/omnibox/omnibox_field_trial.cc:285: // downloaded from the server to be perfect. There's no need for handle On 2013/11/05 04:09:55, Peter Kasting wrote: > Nit: for -> to (and fix above code also) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hfung@chromium.org/55413002/700002
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hfung@chromium.org/55413002/700002
Message was sent while issue was closed.
Change committed as 232979
Message was sent while issue was closed.
https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_result.cc (right): https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_result.cc:162: (undemotable_top_types.find(matches_.begin()->type) != On 2013/11/05 06:11:27, H Fung wrote: > On 2013/11/05 04:09:55, Peter Kasting wrote: > > Nit: Instead of using find(), use count() and check for the result being > > nonzero. > > Done. Actually, should ContainsKey() be used instead? I won't do it now since > I don't see it used too often and might commit this change soon. From stl_util.h? I don't really know why we have that -- it's not particularly shorter, less complicated, or more efficient than using count(). We should probably just remove that. The only advantage I could see would be if this supports more containers than expose count(), and thus makes code a bit more "type-agnostic" in case the container type is changed later.
Message was sent while issue was closed.
On 2013/11/05 20:00:33, Peter Kasting wrote: > https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... > File chrome/browser/autocomplete/autocomplete_result.cc (right): > > https://codereview.chromium.org/55413002/diff/420001/chrome/browser/autocompl... > chrome/browser/autocomplete/autocomplete_result.cc:162: > (undemotable_top_types.find(matches_.begin()->type) != > On 2013/11/05 06:11:27, H Fung wrote: > > On 2013/11/05 04:09:55, Peter Kasting wrote: > > > Nit: Instead of using find(), use count() and check for the result being > > > nonzero. > > > > Done. Actually, should ContainsKey() be used instead? I won't do it now > since > > I don't see it used too often and might commit this change soon. > > From stl_util.h? I don't really know why we have that -- it's not particularly > shorter, less complicated, or more efficient than using count(). We should > probably just remove that. > > The only advantage I could see would be if this supports more containers than > expose count(), and thus makes code a bit more "type-agnostic" in case the > container type is changed later. Using count() here is a perfect example of bad coding. I'll save my time of explaining why, but we can chat offline :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
