Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2583763003:
Factor out AutocompleteMatch creation from BookmarkProvider (Closed)
|
Created:
4 years ago by mattreynolds Modified:
3 years, 11 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFactor out AutocompleteMatch creation from BookmarkProvider
Move the logic for converting from TitledUrlMatch items to
AutocompleteMatch items out of BookmarkProvider into places where it can
be shared with other AutocompleteProviders.
BUG=630769
Patch Set 1 #
Total comments: 4
Patch Set 2 : unit tests #Patch Set 3 : add unit test #
Total comments: 49
Patch Set 4 : document TitledUrlIndex constructor #Patch Set 5 : changes for mpearson@ #
Total comments: 4
Patch Set 6 : fix nits #Patch Set 7 : move CorrectTitleAndMatchPositions to components/omnibox/browser #Patch Set 8 : re-add unittest #
Total comments: 5
Patch Set 9 : TEST_F -> TEST #
Total comments: 6
Patch Set 10 : copyright years, bookmarks namespace #Messages
Total messages: 34 (6 generated)
mattreynolds@chromium.org changed reviewers: + mpearson@chromium.org, sky@chromium.org
sky@chromium.org: Please review changes in components/bookmarks mpearson@chromium.org: Please review changes in components/omnibox
How about test coverage? https://codereview.chromium.org/2583763003/diff/1/components/bookmarks/browse... File components/bookmarks/browser/titled_url_match.cc (right): https://codereview.chromium.org/2583763003/diff/1/components/bookmarks/browse... components/bookmarks/browser/titled_url_match.cc:57: if (leading_whitespace_chars == 0) { no {} https://codereview.chromium.org/2583763003/diff/1/components/bookmarks/browse... components/bookmarks/browser/titled_url_match.cc:62: (*it) = MatchPosition(it->first - leading_whitespace_chars, optional: do an in place update, e.g. it->first -= leading_whitespace_chars.
Added unit tests for autocomplete_provider_utils and titled_url_match https://codereview.chromium.org/2583763003/diff/1/components/bookmarks/browse... File components/bookmarks/browser/titled_url_match.cc (right): https://codereview.chromium.org/2583763003/diff/1/components/bookmarks/browse... components/bookmarks/browser/titled_url_match.cc:57: if (leading_whitespace_chars == 0) { On 2016/12/16 19:32:54, sky wrote: > no {} Done. https://codereview.chromium.org/2583763003/diff/1/components/bookmarks/browse... components/bookmarks/browser/titled_url_match.cc:62: (*it) = MatchPosition(it->first - leading_whitespace_chars, On 2016/12/16 19:32:54, sky wrote: > optional: do an in place update, e.g. it->first -= leading_whitespace_chars. Done.
Apologies for the delay; I've been overwhelmed with metrics things this week. I will get to this review this week. Let me know there is an pressing time constraint. thanks, mark
On 2016/12/21 00:10:37, Mark P wrote: > Apologies for the delay; I've been overwhelmed with metrics things this week. I > will get to this review this week. Let me know there is an pressing time > constraint. > > thanks, > mark Thanks Mark, we're trying to get this in for M57 feature freeze which is the first week of January. I'll be out next week but back on the 3rd.
preliminary comment after looking at the .h files --mark https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_utils.h (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. This file seems awkwardly named for what's it doing. Also, functions without a namespace I think are discouraged. I'm not sure if I have a good suggestion for how to change this. I will mention that sometimes helper functions go as static member in AutocompleteProvider. We've also introduced intermediate subclasses, such as HistoryProvider (children HistoryURL and HistoryQuick) and BaseSearchProvider (children SearchProvider and ZeroSuggest). That I suppose is a possibility. Anyway, please give this a bit of thought about how it can be organized / structured better.
I reviewed the rest of the changelist and have an assortment or trivial comments. The only big comment I have is the one I previously published. Happy holidays, mark https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... File components/bookmarks/browser/titled_url_match.h (right): https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... components/bookmarks/browser/titled_url_match.h:45: base::string16* title, MatchPositions* title_match_positions); nit: This parameter spacing looks odd to me. Did you run this changelist through git cl format? https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... File components/bookmarks/browser/titled_url_match_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... components/bookmarks/browser/titled_url_match_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Thanks for writing tests. https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... components/bookmarks/browser/titled_url_match_unittest.cc:11: namespace bookmarks { I don't usually see unit tests within a namespace. If there's a reason for this, please comment. https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... components/bookmarks/browser/titled_url_match_unittest.cc:30: {1, 3}, {4, 5}, {10, 15}, {base::string16::npos, base::string16::npos} I'm a little surprised to see npos in this list of match positions. I didn't think that was a valid match position. https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... components/bookmarks/browser/titled_url_match_unittest.cc:57: TEST_F(TitledUrlMatchTest, ReplaceOffsetsRemovesItemsWithNposOffsets) { Can you please include one match in here that remains? https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_utils.cc (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:20: ACMatchClassifications ClassificationsFromMatchPositions( I assume you didn't change this code in any meaningful way. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:46: } // namespace nit: blank line above 46 https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:55: int relevance) { I assume you didn't change this implementation in any meaningful way. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:59: AutocompleteMatch match(provider, relevance, false, type); Please either keep the comment about non-deleteable matches here (or, better yet, revise it appropriately), or make this function take a deletable parameter and use that. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:103: match.fill_into_edit.substr(inline_autocomplete_offset); nit: spacing / git cl format https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_utils.h (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/12/22 05:24:31, Mark P (away thru Jan 2 2017) wrote: > This file seems awkwardly named for what's it doing. > Also, functions without a namespace I think are discouraged. > > I'm not sure if I have a good suggestion for how to change this. I will mention > that sometimes helper functions go as static member in AutocompleteProvider. > We've also introduced intermediate subclasses, such as HistoryProvider (children > HistoryURL and HistoryQuick) and BaseSearchProvider (children SearchProvider > and ZeroSuggest). That I suppose is a possibility. > > Anyway, please give this a bit of thought about how it can be organized / > structured better. Perhaps titled_url_match_utils, if nothing else seems better to you. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:9: #include "components/omnibox/browser/autocomplete_match_type.h" Include something for string16? https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:17: // the page title and type |type|. |input| is used to compute the match's Please add a sentence about the variable |relevance| here (after the current first sentence). https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:18: // inline_autocompletion. |fixed_up_input_text| is used in that way as well; By the way, I'm okay with the current comment that does not mention provider and scheme_classifier. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:20: AutocompleteMatch TitledUrlMatchToAutocompleteMatch( Given this (now-revised) description, I think match, type, and relevance should be listed first in the parameter list. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_utils_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Thank you again for writing good, comprehensive tests. You have a fan in me! :-) https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:52: return lhs.offset == rhs.offset && lhs.style == rhs.style; nit: parens around binary operators (==) https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:126: base::string16 match_url_string(base::ASCIIToUTF16("https://www.gmail.com")); nit: add trailing slash to make this a proper URL. ditto above (Sorry, I reviewed this test case first.) https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:144: metrics::OmniboxEventProto::INVALID_SPEC, Please use a valid page classifier like NTP. ditto above https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:145: false, false, false, true, false, classifier); Please make the third false to true to better align with typical calls. ditto above https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:147: AutocompleteMatch autocomplete_match = Maybe say something here about using input_text as your fixed up input? Or just declared a fixed up input variable and set it to input_text? ditto above https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:174: EXPECT_EQ(match_url_string, autocomplete_match.fill_into_edit); Please reverse the order of lines 173 and 174; allowed_to_be_default_match naturally goes with inline_autocompletion. ditto above https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/bookmark_provider.cc (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/bookmark_provider.cc:84: for (auto bookmark_match : matches) { Can this auto be const and/or &? https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/bookmark_provider.h (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/bookmark_provider.h:57: int CalculateBookmarkMatchRelevance(const bookmarks::TitledUrlMatch& match); Can this function be const?
https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... File components/bookmarks/browser/titled_url_match.h (right): https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... components/bookmarks/browser/titled_url_match.h:45: base::string16* title, MatchPositions* title_match_positions); On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > nit: This parameter spacing looks odd to me. Did you run this changelist > through git cl format? Done. https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... File components/bookmarks/browser/titled_url_match_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... components/bookmarks/browser/titled_url_match_unittest.cc:11: namespace bookmarks { On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > I don't usually see unit tests within a namespace. If there's a reason for > this, please comment. I was copying bookmark_utils_unittest.cc which used the namespace to shorten type names for BookmarkNode and other types. But, the same can be accomplished with "using". https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... components/bookmarks/browser/titled_url_match_unittest.cc:30: {1, 3}, {4, 5}, {10, 15}, {base::string16::npos, base::string16::npos} On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > I'm a little surprised to see npos in this list of match positions. I didn't > think that was a valid match position. It's valid in a MatchPosition but not as an input to OffsetsFromMatchPositions, so I'll remove it. When readjusting the offsets it's possible to get an npos in some cases (I can't find the comment now but IIRC it had to do with trailing slashes). https://codereview.chromium.org/2583763003/diff/40001/components/bookmarks/br... components/bookmarks/browser/titled_url_match_unittest.cc:57: TEST_F(TitledUrlMatchTest, ReplaceOffsetsRemovesItemsWithNposOffsets) { On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > Can you please include one match in here that remains? Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_utils.cc (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:20: ACMatchClassifications ClassificationsFromMatchPositions( On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > I assume you didn't change this code in any meaningful way. I renamed ClassificationsFromMatch -> ClassificationsFromMatchPositions (to match the actual type name) Also switched from using query_parser::Snippet::MatchPositions::const_iterator to bookmarks::TitledUrlMatch::MatchPositions::const_iterator as these are typedefs for the same type. Otherwise unchanged. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:46: } // namespace On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > nit: blank line above 46 Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:55: int relevance) { On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > I assume you didn't change this implementation in any meaningful way. Renamed from TitledUrlMatchToACMatch -> TitledUrlMatchToAutocompleteMatch to be more explicit about the different match types we're converting between The original had logic for computing the bookmark match relevance, which was factored out into BookmarkProvider::CalculateBookmarkMatchRelevance and a relevance param was added. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:59: AutocompleteMatch match(provider, relevance, false, type); On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > Please either keep the comment about non-deleteable matches here (or, better > yet, revise it appropriately), or make this function take a deletable parameter > and use that. Revised the comment. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:103: match.fill_into_edit.substr(inline_autocomplete_offset); On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > nit: spacing / git cl format Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_utils.h (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. I didn't want to add components/bookmarks dependencies in autocomplete_provider and didn't want autocomplete_provider dependencies in titled_url_match. A subclass seems like overkill for a single static function. I'll rename to titled_url_match_utils. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:9: #include "components/omnibox/browser/autocomplete_match_type.h" On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > Include something for string16? Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:17: // the page title and type |type|. |input| is used to compute the match's On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > Please add a sentence about the variable |relevance| here (after the current > first sentence). Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:18: // inline_autocompletion. |fixed_up_input_text| is used in that way as well; On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > By the way, I'm okay with the current comment that does not mention provider and > scheme_classifier. Acknowledged. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.h:20: AutocompleteMatch TitledUrlMatchToAutocompleteMatch( On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > Given this (now-revised) description, I think match, type, and relevance should > be listed first in the parameter list. Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_utils_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:52: return lhs.offset == rhs.offset && lhs.style == rhs.style; On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > nit: parens around binary operators (==) Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:126: base::string16 match_url_string(base::ASCIIToUTF16("https://www.gmail.com")); On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > nit: add trailing slash to make this a proper URL. > ditto above > > (Sorry, I reviewed this test case first.) Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:144: metrics::OmniboxEventProto::INVALID_SPEC, On 2016/12/23 22:38:50, Mark P (away thru Jan 2 2017) wrote: > Please use a valid page classifier like NTP. > ditto above Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:145: false, false, false, true, false, classifier); On 2016/12/23 22:38:50, Mark P (away thru Jan 2 2017) wrote: > Please make the third false to true to better align with typical calls. > ditto above Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:147: AutocompleteMatch autocomplete_match = On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > Maybe say something here about using input_text as your fixed up input? Or just > declared a fixed up input variable and set it to input_text? > ditto above Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils_unittest.cc:174: EXPECT_EQ(match_url_string, autocomplete_match.fill_into_edit); On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > Please reverse the order of lines 173 and 174; allowed_to_be_default_match > naturally goes with inline_autocompletion. > ditto above Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/bookmark_provider.cc (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/bookmark_provider.cc:84: for (auto bookmark_match : matches) { On 2016/12/23 22:38:50, Mark P (away thru Jan 2 2017) wrote: > Can this auto be const and/or &? Done. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/bookmark_provider.h (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/bookmark_provider.h:57: int CalculateBookmarkMatchRelevance(const bookmarks::TitledUrlMatch& match); On 2016/12/23 22:38:50, Mark P (away thru Jan 2 2017) wrote: > Can this function be const? Done.
lgtm modulo tiny comments I only reviewed the changes you made in response to my previous comments. Please tell me if you made any non-trivial changes elsewhere in this patchset. And otherwise, in any case, thanks for working with me through this refactoring. cheers, mark https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... File components/omnibox/browser/autocomplete_provider_utils.cc (right): https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:20: ACMatchClassifications ClassificationsFromMatchPositions( On 2017/01/04 00:58:02, mattreynolds wrote: > On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > > I assume you didn't change this code in any meaningful way. > > I renamed ClassificationsFromMatch -> ClassificationsFromMatchPositions (to > match the actual type name) > Also switched from using query_parser::Snippet::MatchPositions::const_iterator > to bookmarks::TitledUrlMatch::MatchPositions::const_iterator as these are > typedefs for the same type. > > Otherwise unchanged. Acknowledged. https://codereview.chromium.org/2583763003/diff/40001/components/omnibox/brow... components/omnibox/browser/autocomplete_provider_utils.cc:55: int relevance) { On 2017/01/04 00:58:02, mattreynolds wrote: > On 2016/12/23 22:38:49, Mark P (away thru Jan 2 2017) wrote: > > I assume you didn't change this implementation in any meaningful way. > > Renamed from TitledUrlMatchToACMatch -> TitledUrlMatchToAutocompleteMatch to be > more explicit about the different match types we're converting between > The original had logic for computing the bookmark match relevance, which was > factored out into BookmarkProvider::CalculateBookmarkMatchRelevance and a > relevance param was added. Acknowledged. https://codereview.chromium.org/2583763003/diff/80001/components/omnibox/brow... File components/omnibox/browser/titled_url_match_utils.cc (right): https://codereview.chromium.org/2583763003/diff/80001/components/omnibox/brow... components/omnibox/browser/titled_url_match_utils.cc:14: namespace bookmarks { nit: prefer putting this at line 49 or so, i.e., no need to nest namespaces I think in this case. https://codereview.chromium.org/2583763003/diff/80001/components/omnibox/brow... File components/omnibox/browser/titled_url_match_utils_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/80001/components/omnibox/brow... components/omnibox/browser/titled_url_match_utils_unittest.cc:88: base::string16 fixed_up_input(input_text); nit: const ditto below
Thanks Mark!
https://codereview.chromium.org/2583763003/diff/80001/components/omnibox/brow... File components/omnibox/browser/titled_url_match_utils.cc (right): https://codereview.chromium.org/2583763003/diff/80001/components/omnibox/brow... components/omnibox/browser/titled_url_match_utils.cc:14: namespace bookmarks { On 2017/01/05 20:56:02, Mark P wrote: > nit: prefer putting this at line 49 or so, i.e., no need to nest namespaces I > think in this case. Done. https://codereview.chromium.org/2583763003/diff/80001/components/omnibox/brow... File components/omnibox/browser/titled_url_match_utils_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/80001/components/omnibox/brow... components/omnibox/browser/titled_url_match_utils_unittest.cc:88: base::string16 fixed_up_input(input_text); On 2017/01/05 20:56:02, Mark P wrote: > nit: const > ditto below Done.
Are you doing this because you need to use CorrectTitleAndMatchPositions from places other than components/omnibox/browser ?
On 2017/01/05 23:13:49, sky wrote: > Are you doing this because you need to use CorrectTitleAndMatchPositions from > places other than components/omnibox/browser ? No, the reason for this refactor is so we can use TitledUrlMatchToAutocompleteMatch in PhysicalWebProvider, which is also in components/omnibox/browser. See ConstructQuerySuggestMatches in https://codereview.chromium.org/2591053002/patch/100001/110006
Couldn't you keep the function in components/omnibox/browser then? Why move to components/bookmarks when it isn't used there? On Thu, Jan 5, 2017 at 3:21 PM, <mattreynolds@chromium.org> wrote: > On 2017/01/05 23:13:49, sky wrote: >> Are you doing this because you need to use CorrectTitleAndMatchPositions >> from >> places other than components/omnibox/browser ? > > No, the reason for this refactor is so we can use > TitledUrlMatchToAutocompleteMatch in PhysicalWebProvider, which is also in > components/omnibox/browser. > > See ConstructQuerySuggestMatches in > https://codereview.chromium.org/2591053002/patch/100001/110006 > > https://codereview.chromium.org/2583763003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/01/05 23:37:59, sky wrote: > Couldn't you keep the function in components/omnibox/browser then? Why > move to components/bookmarks when it isn't used there? SGTM, I moved it to titled_url_match_utls.cc.
Shouldn't the unit test move too?
On 2017/01/06 00:09:28, mattreynolds wrote: > On 2017/01/05 23:37:59, sky wrote: > > Couldn't you keep the function in components/omnibox/browser then? Why > > move to components/bookmarks when it isn't used there? > > SGTM, I moved it to titled_url_match_utls.cc. Please keep the unit test you wrote (by moving it to titled_url_match_utls_unittest.cc). --mark
On 2017/01/06 00:19:17, Mark P wrote: > On 2017/01/06 00:09:28, mattreynolds wrote: > > On 2017/01/05 23:37:59, sky wrote: > > > Couldn't you keep the function in components/omnibox/browser then? Why > > > move to components/bookmarks when it isn't used there? > > > > SGTM, I moved it to titled_url_match_utls.cc. > > Please keep the unit test you wrote (by moving it to > titled_url_match_utls_unittest.cc). Actually nevermind, putting that tiny function in an anonymous namespace is fine. No need to expose it just for testing. This change still lgtm. --mark
On 2017/01/06 00:20:10, Mark P wrote: > On 2017/01/06 00:19:17, Mark P wrote: > > On 2017/01/06 00:09:28, mattreynolds wrote: > > > On 2017/01/05 23:37:59, sky wrote: > > > > Couldn't you keep the function in components/omnibox/browser then? Why > > > > move to components/bookmarks when it isn't used there? > > > > > > SGTM, I moved it to titled_url_match_utls.cc. > > > > Please keep the unit test you wrote (by moving it to > > titled_url_match_utls_unittest.cc). > > Actually nevermind, putting that tiny function in an anonymous namespace is > fine. No need to expose it just for testing. > > This change still lgtm. > > --mark (but I am fine if you want to expose it for testing...) ;_) --mark
On 2017/01/06 00:20:36, Mark P wrote: > On 2017/01/06 00:20:10, Mark P wrote: > > On 2017/01/06 00:19:17, Mark P wrote: > > > On 2017/01/06 00:09:28, mattreynolds wrote: > > > > On 2017/01/05 23:37:59, sky wrote: > > > > > Couldn't you keep the function in components/omnibox/browser then? Why > > > > > move to components/bookmarks when it isn't used there? > > > > > > > > SGTM, I moved it to titled_url_match_utls.cc. > > > > > > Please keep the unit test you wrote (by moving it to > > > titled_url_match_utls_unittest.cc). > > > > Actually nevermind, putting that tiny function in an anonymous namespace is > > fine. No need to expose it just for testing. > > > > This change still lgtm. > > > > --mark > > (but I am fine if you want to expose it for testing...) > ;_) > > --mark Decided to keep the test. Also removed some unnecessary bookmarks:: scopes in titled_url_match.cc (unnecessary because the functions are now in namespace bookmarks). And ran git cl format which changed a few unrelated lines.
https://codereview.chromium.org/2583763003/diff/140001/components/bookmarks/b... File components/bookmarks/browser/titled_url_match_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/140001/components/bookmarks/b... components/bookmarks/browser/titled_url_match_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2583763003/diff/140001/components/bookmarks/b... components/bookmarks/browser/titled_url_match_unittest.cc:14: class TitledUrlMatchTest : public testing::Test { How come you need a class here? Can't you use the TEST macro, e.g.: TEST(TitledUrlMatchTest, Empty...) with no class for TitledUrlMatchTest?
https://codereview.chromium.org/2583763003/diff/140001/components/bookmarks/b... File components/bookmarks/browser/titled_url_match_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/140001/components/bookmarks/b... components/bookmarks/browser/titled_url_match_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/06 00:54:43, sky wrote: > 2017 Done. https://codereview.chromium.org/2583763003/diff/140001/components/bookmarks/b... components/bookmarks/browser/titled_url_match_unittest.cc:14: class TitledUrlMatchTest : public testing::Test { On 2017/01/06 00:54:43, sky wrote: > How come you need a class here? Can't you use the TEST macro, e.g.: > > TEST(TitledUrlMatchTest, Empty...) > > with no class for TitledUrlMatchTest? Cool, didn't know this was an option. I made the change in this file and titled_url_match_utils_unittest.cc.
still lgtm modulo minor comments --mark https://codereview.chromium.org/2583763003/diff/140001/components/bookmarks/b... File components/bookmarks/browser/titled_url_match_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/140001/components/bookmarks/b... components/bookmarks/browser/titled_url_match_unittest.cc:14: class TitledUrlMatchTest : public testing::Test { On 2017/01/06 01:13:44, mattreynolds wrote: > On 2017/01/06 00:54:43, sky wrote: > > How come you need a class here? Can't you use the TEST macro, e.g.: > > > > TEST(TitledUrlMatchTest, Empty...) > > > > with no class for TitledUrlMatchTest? > > Cool, didn't know this was an option. I made the change in this file and > titled_url_match_utils_unittest.cc. Neat. I didn't know it was an option either. https://codereview.chromium.org/2583763003/diff/160001/components/bookmarks/b... File components/bookmarks/browser/titled_url_match_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/160001/components/bookmarks/b... components/bookmarks/browser/titled_url_match_unittest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. Don't change copyright years for existing files. https://codereview.chromium.org/2583763003/diff/160001/components/omnibox/bro... File components/omnibox/browser/titled_url_match_utils.h (right): https://codereview.chromium.org/2583763003/diff/160001/components/omnibox/bro... components/omnibox/browser/titled_url_match_utils.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Please use the correct year in this new file and its .cc and test.
LGTM https://codereview.chromium.org/2583763003/diff/160001/components/bookmarks/b... File components/bookmarks/browser/titled_url_match_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/160001/components/bookmarks/b... components/bookmarks/browser/titled_url_match_unittest.cc:11: using bookmarks::TitledUrlMatch; Put this code in the bookmarks namespace (as other files in this directory do too).
Thanks Mark and Scott! https://codereview.chromium.org/2583763003/diff/160001/components/bookmarks/b... File components/bookmarks/browser/titled_url_match_unittest.cc (right): https://codereview.chromium.org/2583763003/diff/160001/components/bookmarks/b... components/bookmarks/browser/titled_url_match_unittest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/01/06 06:06:09, Mark P wrote: > Don't change copyright years for existing files. Done. https://codereview.chromium.org/2583763003/diff/160001/components/bookmarks/b... components/bookmarks/browser/titled_url_match_unittest.cc:11: using bookmarks::TitledUrlMatch; On 2017/01/06 16:32:54, sky wrote: > Put this code in the bookmarks namespace (as other files in this directory do > too). Done. https://codereview.chromium.org/2583763003/diff/160001/components/omnibox/bro... File components/omnibox/browser/titled_url_match_utils.h (right): https://codereview.chromium.org/2583763003/diff/160001/components/omnibox/bro... components/omnibox/browser/titled_url_match_utils.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2017/01/06 06:06:09, Mark P wrote: > Please use the correct year in this new file and its .cc and test. Happy New Year!
The CQ bit was checked by mattreynolds@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2583763003/#ps180001 (title: "copyright years, bookmarks namespace")
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": 180001, "attempt_start_ts": 1483725840238170,
"parent_rev": "ba3bd9c5fe080d75f8d451709dc42372b9083daa", "commit_rev":
"80dc1ec706e972f489543a202f9e910e1b8c67b4"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
Description was changed from ========== Factor out AutocompleteMatch creation from BookmarkProvider Move the logic for converting from TitledUrlMatch items to AutocompleteMatch items out of BookmarkProvider into places where it can be shared with other AutocompleteProviders. BUG=630769 ========== to ========== Factor out AutocompleteMatch creation from BookmarkProvider Move the logic for converting from TitledUrlMatch items to AutocompleteMatch items out of BookmarkProvider into places where it can be shared with other AutocompleteProviders. BUG=630769 ==========
Message was sent while issue was closed.
On 2017/01/06 19:38:43, commit-bot: I haz the power wrote: > Prior attempt to commit was detected, but we were not able to check whether the > issue was successfully committed. Please check Git history manually and re-check > CQ or close this issue as needed. Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/80dc1ec706e972f489543a202f9e... Bleep bloop, I'm a bot. |
