Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411543011:
Omnibox: Make Keyword Provide More Generous with Matching (Closed)
|
Created:
5 years, 1 month ago by Mark P Modified:
5 years, 1 month ago Reviewers:
Peter Kasting CC:
chromium-reviews, extensions-reviews_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, chromium-apps-reviews_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox: Make Keyword Provide More Generous with Matching
Adds fields to TemplateURLService to keep a map from keyword's domain
to a TemplateURL so that users don't have to type the beginning of the
keyword in order to match the keyword.
Also adds a notion of "effective keyword length" that allows us to
decide that it's okay if the user doesn't type the full keyword but
instead types the important part of it. e.g., this allows ignoring .com
in keywords.
Adds an omnibox field trial that controls both of these behaviors.
The field trial also has a parameter to control the score of
"nearly complete" matches.
BUG=488901
Committed: https://crrev.com/6456fb606e045e77b4efa9e8058dcd86209d992a
Cr-Commit-Position: refs/heads/master@{#359505}
Patch Set 1 #Patch Set 2 : minor cleanup #
Total comments: 1
Patch Set 3 : == -> >= #
Total comments: 82
Patch Set 4 : fix several bugs #Patch Set 5 : almost all of peter's comments #
Total comments: 27
Patch Set 6 : all peter's comments (except one), including previously-promised refactoring, and additional unit t… #
Total comments: 20
Patch Set 7 : peter's latest batch of comments #
Total comments: 2
Patch Set 8 : peter's comments #
Total comments: 4
Patch Set 9 : auto #Patch Set 10 : refactoring insert vs [] #Patch Set 11 : parens #Patch Set 12 : finish converting AddToMap() calls #
Total comments: 2
Messages
Total messages: 29 (7 generated)
Description was changed from ========== Omnibox: Make Keyword Provide More Generous with Matching Adds fields to TemplateURLService to keep a map from keyword's domain to a TemplateURL so that users don't have to type the beginning of the keyword in order to match the keyword. Also adds a notion of "effective keyword length" that allows us to decide that it's okay if the user doesn't type the full keyword but instead types the important part of it. e.g., this allows ignoring .com in keywords. Adds an omnibox field trial that controls both of these behaviors. The field trial also has a parameter to control the score of "nearly complete" matches. BUG=488901 ========== to ========== Omnibox: Make Keyword Provide More Generous with Matching Adds fields to TemplateURLService to keep a map from keyword's domain to a TemplateURL so that users don't have to type the beginning of the keyword in order to match the keyword. Also adds a notion of "effective keyword length" that allows us to decide that it's okay if the user doesn't type the full keyword but instead types the important part of it. e.g., this allows ignoring .com in keywords. Adds an omnibox field trial that controls both of these behaviors. The field trial also has a parameter to control the score of "nearly complete" matches. BUG=488901 ==========
mpearson@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter, I think this is code-complete. I haven't yet tested it. Nonetheless, I think it's worth you taking a look to tell me whether you're comfortable with this approach. --mark https://codereview.chromium.org/1411543011/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:2370: template <typename Container> Aside from introducing the template and responding to changes because the vector is now a vector of pairs, this is a direct move from the already-existing code.
For the record, I have tested this code interactively. It works correctly (after I fixed the bugs I discovered during testing). It's ready for a complete review. I will look into the necessity and feasibility of adding unit tests. --mark
The actual functionality here is relatively simple, so almost all my comments are more about naming, comments, style, etc. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... File components/omnibox/browser/keyword_provider.cc (right): https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:258: keyword, !remaining_input.empty(), &matches); As written this seems to imply that the functions here merely add to the vector outparam instead of resetting its contents, but the functions are not named that way (AddMatchingXXX()), have no comments to that effect in the .h, and that would be atypical behavior for a function. You should either not assume that here, or make it clear in the function names/comments that they behave this way. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:328: // retrieved the same keyword twice. For example, the keyword' Nit: Mismatched ' https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:329: // abc.abc.com, may be retrieved for the input abc from the full keyword Nit: No comma (but consider adding quotes around keyword names and input strings). https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:336: } Nit: Use std::find_if() with a lambda instead of this loop. Alternately you could sort/unique/erase on a dupe-containing vector but that's probably going to be much more annoying here. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:389: // describe it, so it's clear why the functions diverge. Instead of this long comment about what's in sync and why, why not just do the incomplete matching part and then call the other scoring function for the other cases? Then you can eliminate the whole last 2/3 of this comment. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... File components/omnibox/browser/keyword_provider.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.h:120: bool nearly_complete, Nit: |sufficiently_complete| seems less misleading, as that more clearly covers both complete and incomplete cases. If you agree, update all the function/type names and comments in other files to match. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.h:129: const size_t effective_keyword_length, Nit: How about |meaningful_keyword_length|? I'm not sure I would say that "foo.com" is "effectively three characters" but I would be more willing to say that it has "three meaningful characters". https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:326: // Return true if KeywordProvider (via TemplateURLService), for auto- Nit: Return -> Returns https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:328: // important part of the keyword name for matching purposes. Returns true Nit: Awkward, how about: Returns whether the KeywordProvider should require users to type any effective TLD portion ("co.uk") of autogenerated keywords to match against them. Then name the function KeywordRequiresTld(). https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:332: // Return true if KeywordProvider (via TemplateURLService), for auto- Nit: Return -> Returns https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:336: // be considered important for matching purposes. Returns true if the Nit: Awkward, how about: For autogenerated keywords containing more than just a TLD+1, returns whether the KeywordProvider should require users to type the full hostname to match against them, rather than just the TLD+1 portion. Then name the function KeywordRequiresFullHostname(). https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:340: // For the relevance score that KeywordProvider should assign to keywords Nit: For -> Returns https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:153: // Returns the length of the registry portion of a hostname. e.g., Nit: e.g. -> E.g., or just write "For example," (2 places) https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:154: // www.google.co.uk will return 5 (=length of co.uk). Nit: (=length -> (the length (2 places) https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:174: // it is can be less. For instance, for the keyword google.co.uk, this can Nit: it is -> it https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:178: if (!turl->safe_for_autoreplace() || I don't really think checking for this is right, conceptually. If I edit the keyword for "en.wikipedia.org" to "wikipedia.org", that shouldn't disable matching "wikipedia". If I add a new TURL with keyword "google.com", I should still matach against "google". In other words, I think we should simply ignore the "autogenerated" requirement here and everywhere else in your change and do this for all keywords. The RCD helper functions can deal with input strings that don't look like hostnames, so that's not a problem. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:185: return keyword.length() - ((registry_length > 0) ? (registry_length + 1) : 0); Make sure this doesn't return -1 if you feed in the string "co.uk". You can convert "(registry_length > 0)" to just "registry_length". https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:1463: AddToDomainMap(best_fallback); You do this sort of "add to |keyword_to_template_map_|, then call AddToDomainMap()" thing several times; it's probably worth its own helper. In fact, since AddToDomainMap() always seems to be called in this sort of way, maybe just replace that with a function that also does this additional bit. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:1525: // Required for VS2010: http://connect.microsoft.com/VisualStudio/feedback/details/520043/error-conve... Nit: We no longer compile with VS2010, so let's remove this while you're touching it anyway, and use nullptr. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:1535: domain, std::make_pair(kNullTemplateURL, 0)))); Nit: I feel like it should be possible to replace the explicit make_pair() call here with just "TURLAndEffectiveLength()" or something. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:1541: keyword_domain_to_template_map_.erase(it); Nit: Prefer std::erase(std::remove_if()) with a lambda. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2379: DCHECK(matches != NULL); Nit: DCHECK(matches); would be shorter https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2382: TemplateURL* const kNullTemplateURL = NULL; Nit: See earlier comment https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2392: std::make_pair(kNullTemplateURL, 0)), Nit: See earlier comment https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2401: } Nit: Prefer std::copy_if() with a lambda https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:82: // want to penalize for the user not typing those.) Nit: How about: "For keywords that end in a TLD, e.g 'foo.com', we want to record that the pre-TLD portion is more meaningful, to allow matching against just that portion of the keyword in some scenarios." https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:83: typedef std::pair<TemplateURL*, size_t> TemplateURLAndEffectiveKeywordLength; Nit: This is excessively verbose; call this TURLAndEffectiveLength https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:85: TemplateURLMatchesVector; Nit: Don't put "Vector" in the typedef name, and "matches" is somewhat confusing because it implies a usage (that this type can only be used for "matching" something) rather than just being descriptive of what's in it. If you rename according to my suggestion above, this could be "TURLsAndEffectiveLengths" instead. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:138: // with the domain name part of the keyword starts with |prefix|, sorted Nit: with -> where Be careful using "domain name". It doesn't just mean the TLD+1; "abc.def.com" is a domain name, as is "def.com", as is "com" (but only the first two are also hostnames). For this reason I try to use "TLD+1" pretty consistently for what you refer to as "domain name". The only problem with that is that it's hard to use in a type or variable name, so some creativity is required, and in the limit I might stoop to "domain" in such a name as long as the comments were clearer. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:139: // shortest domain-name-first. If |support_replacement_only| is true, only Nit: Also hyphen after "shortest". Also we should probably rename "support_replacement_only" to "supports_replacement_only" here and elsewhere for grammar reasons. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:404: typedef std::map<base::string16, TemplateURLAndEffectiveKeywordLength> Nit: The Chromium C++11 guide says to prefer type alias syntax ("using") for typedefs at this point, so maybe make all these use that? https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:407: KeywordDomainToTemplateMap; Nit: Add comments to describe each of these maps, especially for the multimap to be clear why it can have multiple results for the same key. I don't love "ToTemplateMap" as a name for both a map to a TURL* and a map to a TURLAndEffectiveLength; it seems like these should have different names, but I don't know what, and I'm hoping the comments will help make the name more obvious. In some cases this may just mean moving up the comments from the member variables to these types. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:450: void AddToDomainMap(TemplateURL* template_url); Nit: Please add comments for all these functions while you're here, especially since we now have several different kinds of maps. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:637: // whose keywords begin with |prefix|, sorted shortest keyword-first. If Nit: Also hyphen after "shortest" https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:638: // |support_replacement_only| is true, only TemplateURLs that support Nit: support_ -> supports_ https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:689: // Mapping from keyword domain to the TemplateURL. Precisely, for Nit: Blank line above https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:700: // keywords will not have entries in this map. How about this comment: Mapping from TLD+1s to corresponding TemplateURLs. Specifically, for an autogenerated keyword that is a hostname containing more than just a TLD+1, e.g. 'abc.def.com', the keyword is added to this map under the TLD+1 'def.com'. This means multiple keywords from the same TLD+1 share the same key, so this must be a multimap. If you're moving this comment up to the typedef above, then I would leave out the remaining details about autogenerated keywords and the like. Then on the comment on this particular data member, you could include that sort of thing along with the motivation for it; you could even name the member accordingly, e.g. "domain_to_autogenerated_turl_map_". But, see also comments in the source file about this whole concept.
On Mon, Nov 9, 2015 at 3:11 PM, <pkasting@chromium.org> wrote: > > > https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... > components/search_engines/template_url_service.cc:178: if > (!turl->safe_for_autoreplace() || > I don't really think checking for this is right, conceptually. > > If I edit the keyword for "en.wikipedia.org" to "wikipedia.org", that > shouldn't disable matching "wikipedia". This won't happen. You'll still get matches against wikipedia.org because it's stored in the regular map. > If I add a new TURL with > keyword "google.com", I should still matach against "google". > Ditto. The only effect of the |safe_for_autoreplace| here is that you don't get forgiven for omitting the ".com". This makes sense to me; if you want to invoke a keyword search engine on a keyword that you are in the process of renaming, and don't want to type the .com, then rename it and don't give it the .com ending. :-) |safe_for_autoreplace| also disables matching in the domain map. This also seems reasonable: if you want to invoke a particular keyword abc.def.com that you're in the process of renaming, and intend to invoke it by typing def or def.com, just rename it to def.com. Don't put something on the beginning if that's not the way you intend to invoke it. In other words, I think we should simply ignore the "autogenerated" > requirement here and everywhere else in your change and do this for all > keywords. The RCD helper functions can deal with input strings that > don't look like hostnames, so that's not a problem. > I like this autogenerated criteria because it makes it easier for people to rename keywords to get them out of the way. There's other code in template URL service that helps support this philosophy (e.g., don't create new keyword if the user already has a keyword associated with this site that was manually remained). That said, I don't think this is restriction is critical. If people are renaming keywords, maybe we can expect them to rename them "right" in order to get them out of the way. I'm willing to removing the autogenerated restriction if you still want it removed, but I thought I'd explain my reasoning about why it's there in the first place. --mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Nov 10, 2015 at 1:49 PM, Mark Pearson <mpearson@chromium.org> wrote: > On Mon, Nov 9, 2015 at 3:11 PM, <pkasting@chromium.org> wrote: > >> >> >> https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... >> components/search_engines/template_url_service.cc:178: if >> (!turl->safe_for_autoreplace() || >> I don't really think checking for this is right, conceptually. >> >> If I edit the keyword for "en.wikipedia.org" to "wikipedia.org", that >> shouldn't disable matching "wikipedia". > > This won't happen. You'll still get matches against wikipedia.org because > it's stored in the regular map. > >> If I add a new TURL with >> keyword "google.com", I should still matach against "google". >> > Ditto. > Good points. I selected bad examples :) I like this autogenerated criteria because it makes it easier for people to > rename keywords to get them out of the way. There's other code in > template URL service that helps support this philosophy (e.g., don't > create new keyword if the user already has a keyword associated with > this site that was manually remained). > > That said, I don't think this is restriction is critical. If people are > renaming > keywords, maybe we can expect them to rename them "right" in order > to get them out of the way. I'm willing to removing the autogenerated > restriction if you still want it removed, but I thought I'd explain my > reasoning > about why it's there in the first place. > I think your concerns have some merit, but in the end my instinct is that it would still be better to not have the behavior differ between autogenerated keywords and non-autogenerated ones; I think if someone wants to rename a keyword to "get it out of the way" they should rename it to something that doesn't look like all the other valid keywords. It's not obvious to me that someone would, say, rename a keyword from "foo" to " foo.com" in hopes of not matching it when typing "foo". It seems more likely, and more logical, that the person would rename it "DO_NOT_USE" or some garbage string. There's another bug (and CL) on making it easier to escape keyword mode when you didn't intend it, by having backspace out of keyword mode leave the trailing space. It was basically complete and IIRC just needed some tests. Perhaps if you're concerned this could make it too easily to accidentally trigger keyword mode for undesirable keywords, it would make sense to land that and put an UMA tracker on it, to see if this makes people try to bail out of keyword mode more? PK To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I did everything you suggested except I didn't manage to comment the AddTo/RemoveFrom DomainMap() header functions, not refactor and/or rename the general AddTo/RemoveFrom Maps functions. I'll get to that soon. In any case, I'm likely out of the office tomorrow, so I figured I'd send you this next iteration of the changelist for your examination, and leave that last bit of cleanup for later this week. It's late and I'm tired. --mark https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... File components/omnibox/browser/keyword_provider.cc (right): https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:258: keyword, !remaining_input.empty(), &matches); On 2015/11/09 23:11:14, Peter Kasting wrote: > As written this seems to imply that the functions here merely add to the vector > outparam instead of resetting its contents, but the functions are not named that > way (AddMatchingXXX()), have no comments to that effect in the .h, and that > would be atypical behavior for a function. You should either not assume that > here, or make it clear in the function names/comments that they behave this way. Revised to AddMatching...() and changed all comments appropriately. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:328: // retrieved the same keyword twice. For example, the keyword' On 2015/11/09 23:11:14, Peter Kasting wrote: > Nit: Mismatched ' Done. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:329: // abc.abc.com, may be retrieved for the input abc from the full keyword On 2015/11/09 23:11:14, Peter Kasting wrote: > Nit: No comma (but consider adding quotes around keyword names and input > strings). Did both. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:336: } On 2015/11/09 23:11:14, Peter Kasting wrote: > Nit: Use std::find_if() with a lambda instead of this loop. Done. > Alternately you could sort/unique/erase on a dupe-containing vector but that's > probably going to be much more annoying here. Yes, and more expensive. For short inputs such as "w", there may be a ton of matches. We don't have to add them all then unique them all just to get the top X. Stuck with the find_if. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.cc:389: // describe it, so it's clear why the functions diverge. On 2015/11/09 23:11:14, Peter Kasting wrote: > Instead of this long comment about what's in sync and why, why not just do the > incomplete matching part and then call the other scoring function for the other > cases? Then you can eliminate the whole last 2/3 of this comment. Done. Now search_provider.{cc,h} is part of this changelist as well. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... File components/omnibox/browser/keyword_provider.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.h:120: bool nearly_complete, On 2015/11/09 23:11:14, Peter Kasting wrote: > Nit: |sufficiently_complete| seems less misleading, as that more clearly covers > both complete and incomplete cases. Makes sense to me. > If you agree, update all the function/type names and comments in other files to > match. Done. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/keyword_provider.h:129: const size_t effective_keyword_length, On 2015/11/09 23:11:14, Peter Kasting wrote: > Nit: How about |meaningful_keyword_length|? I'm not sure I would say that > "foo.com" is "effectively three characters" but I would be more willing to say > that it has "three meaningful characters". That sounds reasonable. With that in mind, I change the template_url_service stuff to talk about MeaningfulLength, not EffectiveLength. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:326: // Return true if KeywordProvider (via TemplateURLService), for auto- On 2015/11/09 23:11:14, Peter Kasting wrote: > Nit: Return -> Returns Done. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:328: // important part of the keyword name for matching purposes. Returns true On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: Awkward, how about: > > Returns whether the KeywordProvider should require users to type any effective > TLD portion ("co.uk") of autogenerated keywords to match against them. This isn't correct. It can match against them even if this is false. It's more about scoring and whether this is considered meaningful. Revises somewhat anyway. > Then name the function KeywordRequiresTld(). I'm sticking with registry for consistency with the underlying net functions. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:332: // Return true if KeywordProvider (via TemplateURLService), for auto- On 2015/11/09 23:11:14, Peter Kasting wrote: > Nit: Return -> Returns Done. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:336: // be considered important for matching purposes. Returns true if the On 2015/11/09 23:11:14, Peter Kasting wrote: > Nit: Awkward, how about: > > For autogenerated keywords containing more than just a TLD+1, returns whether > the KeywordProvider should require users to type the full hostname to match > against them, rather than just the TLD+1 portion. Revised somewhat in the direction you suggested. > Then name the function KeywordRequiresFullHostname(). That's not true. Prefixes are okay. Instead revised to KeywordRequiresPrefixMatch. https://codereview.chromium.org/1411543011/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:340: // For the relevance score that KeywordProvider should assign to keywords On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: For -> Returns Done. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:153: // Returns the length of the registry portion of a hostname. e.g., On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: e.g. -> E.g., or just write "For example," (2 places) Did both. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:154: // www.google.co.uk will return 5 (=length of co.uk). On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: (=length -> (the length (2 places) Did both. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:174: // it is can be less. For instance, for the keyword google.co.uk, this can On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: it is -> it I removed the relevant sentence. This suggestion is now moot. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:178: if (!turl->safe_for_autoreplace() || On 2015/11/09 23:11:15, Peter Kasting wrote: > I don't really think checking for this is right, conceptually. > > If I edit the keyword for "en.wikipedia.org" to "wikipedia.org", that shouldn't > disable matching "wikipedia". If I add a new TURL with keyword "google.com", I > should still matach against "google". > > In other words, I think we should simply ignore the "autogenerated" requirement > here and everywhere else in your change and do this for all keywords. The RCD > helper functions can deal with input strings that don't look like hostnames, so > that's not a problem. Removed the auto-generated restriction and revised comments throughout. Let me know if I missed some. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:185: return keyword.length() - ((registry_length > 0) ? (registry_length + 1) : 0); On 2015/11/09 23:11:15, Peter Kasting wrote: > Make sure this doesn't return -1 if you feed in the string "co.uk". Good point. Fixed. > You can convert "(registry_length > 0)" to just "registry_length". Done. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:1525: // Required for VS2010: http://connect.microsoft.com/VisualStudio/feedback/details/520043/error-conve... On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: We no longer compile with VS2010, so let's remove this while you're > touching it anyway, and use nullptr. Now unneeded in this location. Done in the other location. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:1535: domain, std::make_pair(kNullTemplateURL, 0)))); On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: I feel like it should be possible to replace the explicit make_pair() call > here with just "TURLAndEffectiveLength()" or something. That's no shorter. Also, I usually see make_pair in the codebase even when the pair has been aliased to something else. Regardless, this point is now moot; I've figured out a code simplification that doesn't use this here (though there is still a make_pair call in AddMatchingKeywordsHelper) https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:1541: keyword_domain_to_template_map_.erase(it); On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: Prefer std::erase(std::remove_if()) with a lambda. I have simplified this in a different way that may satisfy you. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2379: DCHECK(matches != NULL); On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: DCHECK(matches); would be shorter Done. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2382: TemplateURL* const kNullTemplateURL = NULL; On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: See earlier comment Done. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2392: std::make_pair(kNullTemplateURL, 0)), On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: See earlier comment See earlier answer. :-) https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2401: } On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: Prefer std::copy_if() with a lambda Not done. I couldn't figure out the magical incantation. Here's the closest I got: // Add to vector of matching keywords. const SearchTermsData& data = search_terms_data(); std::copy_if( match_range.first, match_range.second, matches->begin(), [supports_replacement_only, &data] (const typename Container::value_type& keyword_turl_and_length) { return !supports_replacement_only || keyword_turl_and_length.second.first->url_ref().SupportsReplacement(data); }); } before I realized that we actually need to convert between input range type (an iterator over a map, i.e, an iterator over key, value pairs) to output range types (a vector simply of value pairs). I couldn't figure out how to make copy_if work when converting between different types like this. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:82: // want to penalize for the user not typing those.) On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: How about: "For keywords that end in a TLD, e.g 'foo.com', we want to > record that the pre-TLD portion is more meaningful, to allow matching against > just that portion of the keyword in some scenarios." I find this confusing. I don't know what your explanation means, and the parts I do understand seem wrong. I rewrote my comment to be a little more like your suggestion. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:83: typedef std::pair<TemplateURL*, size_t> TemplateURLAndEffectiveKeywordLength; On 2015/11/09 23:11:16, Peter Kasting wrote: > Nit: This is excessively verbose; call this TURLAndEffectiveLength Done, except used Meaningful, not Effective, per your other comment. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:85: TemplateURLMatchesVector; On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: Don't put "Vector" in the typedef name, and "matches" is somewhat confusing > because it implies a usage (that this type can only be used for "matching" > something) rather than just being descriptive of what's in it. If you rename > according to my suggestion above, this could be "TURLsAndEffectiveLengths" > instead. Makes sense to me. Done (except using meaningful, not effective, as above). https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:138: // with the domain name part of the keyword starts with |prefix|, sorted On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: with -> where Done. > Be careful using "domain name". It doesn't just mean the TLD+1; "abc.def.com" > is a domain name, as is "def.com", as is "com" (but only the first two are also > hostnames). For this reason I try to use "TLD+1" pretty consistently for what > you refer to as "domain name". The only problem with that is that it's hard to > use in a type or variable name, so some creativity is required, and in the limit > I might stoop to "domain" in such a name as long as the comments were clearer. I'd like to stick with domain name for consistency with the underlying functions I'm using from net/base/registry_controlled_domains/registry_controlled_domain.h If those functions talked about TLD+1 (in the name or comments), I'd change these comments appropriately. But for now if I think the comments should imply/hint at the implementation, that I'm using GetDomainAndRegistry() and not something else. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:139: // shortest domain-name-first. If |support_replacement_only| is true, only On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: Also hyphen after "shortest". Done. Also we should probably rename > "support_replacement_only" to "supports_replacement_only" here and elsewhere for > grammar reasons. Done throughout this file and its .cc. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:404: typedef std::map<base::string16, TemplateURLAndEffectiveKeywordLength> On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: The Chromium C++11 guide says to prefer type alias syntax ("using") for > typedefs at this point, so maybe make all these use that? Revised this whole .h file accordingly. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:407: KeywordDomainToTemplateMap; On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: Add comments to describe each of these maps, especially for the multimap to > be clear why it can have multiple results for the same key. Done. > I don't love "ToTemplateMap" as a name for both a map to a TURL* and a map to a > TURLAndEffectiveLength; it seems like these should have different names, but I > don't know what, and I'm hoping the comments will help make the name more > obvious. I can simply do KeywordToTURLAndEffectiveLength and KeywordDomainToTURLAndEffectiveLength instead of what's currently here. Those seems clear to me. What do you think? > > In some cases this may just mean moving up the comments from the member > variables to these types. Yup, that's what I did (moved all the content up, slightly revised it). https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:637: // whose keywords begin with |prefix|, sorted shortest keyword-first. If On 2015/11/09 23:11:16, Peter Kasting wrote: > Nit: Also hyphen after "shortest" Done. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:638: // |support_replacement_only| is true, only TemplateURLs that support On 2015/11/09 23:11:16, Peter Kasting wrote: > Nit: support_ -> supports_ Done. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:689: // Mapping from keyword domain to the TemplateURL. Precisely, for On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: Blank line above Done. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:700: // keywords will not have entries in this map. On 2015/11/09 23:11:16, Peter Kasting wrote: > How about this comment: > > Mapping from TLD+1s to corresponding TemplateURLs. Specifically, for an > autogenerated keyword that is a hostname containing more than just a TLD+1, e.g. > 'abc.def.com', the keyword is added to this map under the TLD+1 'def.com'. This > means multiple keywords from the same TLD+1 share the same key, so this must be > a multimap. > > If you're moving this comment up to the typedef above, then I would leave out > the remaining details about autogenerated keywords and the like. Then on the > comment on this particular data member, you could include that sort of thing > along with the motivation for it; Done. Used the first paragraph of the comment by moving it above; kept the nitty-gritty details near the variable declaration. > you could even name the member accordingly, > e.g. "domain_to_autogenerated_turl_map_". But, see also comments in the source > file about this whole concept. I'm fine renaming this. We can do domain_to_turl_ or domain_to_turl_and_meaningful_length_. Whether either of these is good depends on what you say about the type name above. Whatever we chose, the earlier variable I think should simply be this variable without the "domain_" portion.
Here are fixes for the last two comments you had for me. I'm adding unit tests now. --mark https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:1463: AddToDomainMap(best_fallback); On 2015/11/09 23:11:15, Peter Kasting wrote: > You do this sort of "add to |keyword_to_template_map_|, then call > AddToDomainMap()" thing several times; it's probably worth its own helper. Good idea. I added the helper and it looks a lot clearer. > In fact, since AddToDomainMap() always seems to be called in this sort of way, > maybe just replace that with a function that also does this additional bit. I now also use the helper in AddToDomainMap. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:450: void AddToDomainMap(TemplateURL* template_url); On 2015/11/09 23:11:15, Peter Kasting wrote: > Nit: Please add comments for all these functions while you're here, especially > since we now have several different kinds of maps. Done. (Added comments to all these Add/Remove functions.)
You didn't actually upload your new fixes, but that's fine, I'll look when you upload those + tests + responses to these comments. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2392: std::make_pair(kNullTemplateURL, 0)), On 2015/11/11 07:41:00, Mark P wrote: > On 2015/11/09 23:11:15, Peter Kasting wrote: > > Nit: See earlier comment > > See earlier answer. :-) It may not be shorter to write, but I think just writing TURLAndMeaningfulLength() here is semantically clearer. It means "I want the default value of the mapped type for this container". Using make_pair() explicitly emphasizes the importance of the values inside the make_pair() call, but you don't actually care about what those are, and if the definition of TURLAndMeaningfulLength changed in the future, you'd want this code to Just Keep Working. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2401: } On 2015/11/11 07:41:00, Mark P wrote: > I couldn't figure out how to make copy_if work when converting between different > types like this. You're right. I overlooked the difference in container types. This is doable with separate transform() and copy_if() steps, but what you have now is cleaner than that, so leave it. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:407: KeywordDomainToTemplateMap; On 2015/11/11 07:41:01, Mark P wrote: > On 2015/11/09 23:11:15, Peter Kasting wrote: > > I don't love "ToTemplateMap" as a name for both a map to a TURL* and a map to > a > > TURLAndEffectiveLength; it seems like these should have different names, but I > > don't know what, and I'm hoping the comments will help make the name more > > obvious. > > I can simply do KeywordToTURLAndEffectiveLength and > KeywordDomainToTURLAndEffectiveLength instead of what's currently here. Those > seems clear to me. What do you think? Seems OK to me. https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:333: // hostnames, users to type a prefix of the hostname to match against them, Nit: Slightly awkward; rearrange: For keywords that look like hostnames, returns whether KeywordProvider should require users... https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... File components/omnibox/browser/search_provider.cc (right): https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... components/omnibox/browser/search_provider.cc:191: int SearchProvider::CalculateRelevanceForKeywordVerbatim( Nit: Move this function to correspond with your movement in the .h file. https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... File components/omnibox/browser/search_provider.h (right): https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... components/omnibox/browser/search_provider.h:70: // input matches one of the profile's keyword). Nit: keyword -> keywords https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... components/omnibox/browser/search_provider.h:73: bool allow_exact_keyword_match, Nit: Add comments about what this arg means. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:173: // google.co.uk, this can return 6 (the length of google). Nit: google -> "google" https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:174: size_t GetEffectiveKeywordLength(const base::string16& keyword, Nit: Effective -> Meaningful https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:181: // registry. Don't return something less than 0 regardless. Nit: The point of the conditional is sort of obvious when you read it, so I'd probably omit this sentence. It might be clearer to write this code as something like: const size_t registry_length = GetRegistryLength(keyword); if (OmniboxFieldTrial::KeywordRequiresRegistry() || !registry_length) return keyword.length(); // The meaningful keyword length is the length of any portion before the // registry ("co.uk") and its preceding dot. return std::max(keyword.length() - registry_length - 1, 0); https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1526: it != keyword_domain_to_template_map_.upper_bound(domain); ) { You should pull the upper_bound() out of the loop here; it will remain valid across erase() calls, and you don't want to recalculate it on every iteration. Once this is done, going back to the equal_range() method of finding the upper and lower bounds is more efficient. I was mistaken to suggest remove_if() before since that can't be used with associative containers, and I didn't point out that your previous code could dereference a garbage iterator after erase() (but you fixed it anyway). So I think just combining equal_range() with this loop body is the best way to go. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1527: if (it->second.first == template_url) { Nit: {} unnecessary https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:2371: typename Container::const_iterator> match_range( Nit: I'd probably use auto here https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.h:82: // registry, e.g., '.com', we want to consider the registry characters not a Nit: not -> as not https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.h:83: // meaningful part of the keyword not penalize for the user not typing those.) Nit: not penalize -> and not penalize https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.h:709: // keywords in which the keyword is the domain name, no host/sub-domain Nit: no host/sub-domain -> with no subdomain
Here's an actual upload that includes - the previously-promised Remove/Add map comments and refactoring - replied to all your new comments except one [I'm going to lunch now; I'll get to it after lunch; I wanted to give you chance to start looking over this now rather than wait] - unit tests Please take a look, thanks, mark https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:333: // hostnames, users to type a prefix of the hostname to match against them, On 2015/11/12 20:36:10, Peter Kasting wrote: > Nit: Slightly awkward; rearrange: > > For keywords that look like hostnames, returns whether KeywordProvider should > require users... Done. I kindof wanted to write it like that originally too; I just wasn't sure if you required the comment to start with "Returns ..." for the sake of parallel comment structure. I guess not. :-) https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... File components/omnibox/browser/search_provider.cc (right): https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... components/omnibox/browser/search_provider.cc:191: int SearchProvider::CalculateRelevanceForKeywordVerbatim( On 2015/11/12 20:36:10, Peter Kasting wrote: > Nit: Move this function to correspond with your movement in the .h file. Done. Also moved several other functions that I noticed where out of place. No modification made. https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... File components/omnibox/browser/search_provider.h (right): https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... components/omnibox/browser/search_provider.h:70: // input matches one of the profile's keyword). On 2015/11/12 20:36:10, Peter Kasting wrote: > Nit: keyword -> keywords Done. https://codereview.chromium.org/1411543011/diff/80001/components/omnibox/brow... components/omnibox/browser/search_provider.h:73: bool allow_exact_keyword_match, On 2015/11/12 20:36:10, Peter Kasting wrote: > Nit: Add comments about what this arg means. Done. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:173: // google.co.uk, this can return 6 (the length of google). On 2015/11/12 20:36:11, Peter Kasting wrote: > Nit: google -> "google" Done. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:174: size_t GetEffectiveKeywordLength(const base::string16& keyword, On 2015/11/12 20:36:11, Peter Kasting wrote: > Nit: Effective -> Meaningful Done. Sorry I missed this during past revisions. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:181: // registry. Don't return something less than 0 regardless. On 2015/11/12 20:36:11, Peter Kasting wrote: > Nit: The point of the conditional is sort of obvious when you read it, so I'd > probably omit this sentence. > > It might be clearer to write this code as something like: > > const size_t registry_length = GetRegistryLength(keyword); > if (OmniboxFieldTrial::KeywordRequiresRegistry() || !registry_length) > return keyword.length(); Didn't restructure the beginning of this because I don't want to take the time to compute registry lengths if the field trial isn't enabled. > // The meaningful keyword length is the length of any portion before the > // registry ("co.uk") and its preceding dot. > return std::max(keyword.length() - registry_length - 1, 0); Use your comment and (approximately) your code. I needed a <int> type because otherwise the compiler has trouble figuring out how to do the comparison (pasted below in case you care). I might be to do this by putting an appropriate annotation on the "0", but I don't see what and the current <int> code is fine with me. ../../components/search_engines/template_url_service.cc:184:10: error: no matching function for call to 'max' return std::max(keyword.length() - (registry_length ? (registry_length + 1) : 0), 0); ^~~~~~~~ /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/algorithmfwd.h:356:5: note: candidate template ignored: deduced conflicting types for parameter '_Tp' ('unsigned long' vs. 'int') max(const _Tp&, const _Tp&); ^ /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_algo.h:4236:5: note: candidate template ignored: could not match 'initializer_list<type-parameter-0-0>' against 'unsigned long' max(initializer_list<_Tp> __l, _Compare __comp) ^ /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/algorithmfwd.h:360:5: note: candidate function template not viable: requires 3 arguments, but 2 were provided max(const _Tp&, const _Tp&, _Compare); ^ /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_algo.h:4231:5: note: candidate function template not viable: requires single argument '__l', but 2 arguments were provided max(initializer_list<_Tp> __l) ^ 1 error generated. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1526: it != keyword_domain_to_template_map_.upper_bound(domain); ) { On 2015/11/12 20:36:11, Peter Kasting wrote: > You should pull the upper_bound() out of the loop here; it will remain valid > across erase() calls, and you don't want to recalculate it on every iteration.. > Once this is done, going back to the equal_range() method of finding the upper > and lower bounds is more efficient. > > I was mistaken to suggest remove_if() before since that can't be used with > associative containers, and I didn't point out that your previous code could > dereference a garbage iterator after erase() (but you fixed it anyway). So I > think just combining equal_range() with this loop body is the best way to go. Still TODO. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1527: if (it->second.first == template_url) { On 2015/11/12 20:36:11, Peter Kasting wrote: > Nit: {} unnecessary Done. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:2371: typename Container::const_iterator> match_range( On 2015/11/12 20:36:11, Peter Kasting wrote: > Nit: I'd probably use auto here Done. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.h:82: // registry, e.g., '.com', we want to consider the registry characters not a On 2015/11/12 20:36:11, Peter Kasting wrote: > Nit: not -> as not Done. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.h:83: // meaningful part of the keyword not penalize for the user not typing those.) On 2015/11/12 20:36:11, Peter Kasting wrote: > Nit: not penalize -> and not penalize Done. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.h:709: // keywords in which the keyword is the domain name, no host/sub-domain On 2015/11/12 20:36:11, Peter Kasting wrote: > Nit: no host/sub-domain -> with no subdomain Done.
You missed a couple comments I made on your older patch sets; replied t them again below so you'll have links to them. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2392: std::make_pair(kNullTemplateURL, 0)), On 2015/11/12 20:36:10, Peter Kasting wrote: > On 2015/11/11 07:41:00, Mark P wrote: > > On 2015/11/09 23:11:15, Peter Kasting wrote: > > > Nit: See earlier comment > > > > See earlier answer. :-) > > It may not be shorter to write, but I think just writing > TURLAndMeaningfulLength() here is semantically clearer. It means "I want the > default value of the mapped type for this container". Using make_pair() > explicitly emphasizes the importance of the values inside the make_pair() call, > but you don't actually care about what those are, and if the definition of > TURLAndMeaningfulLength changed in the future, you'd want this code to Just Keep > Working. You didn't reply to this. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:407: KeywordDomainToTemplateMap; On 2015/11/12 20:36:10, Peter Kasting wrote: > On 2015/11/11 07:41:01, Mark P wrote: > > On 2015/11/09 23:11:15, Peter Kasting wrote: > > > I don't love "ToTemplateMap" as a name for both a map to a TURL* and a map > to > > a > > > TURLAndEffectiveLength; it seems like these should have different names, but > I > > > don't know what, and I'm hoping the comments will help make the name more > > > obvious. > > > > I can simply do KeywordToTURLAndEffectiveLength and > > KeywordDomainToTURLAndEffectiveLength instead of what's currently here. Those > > seems clear to me. What do you think? > > Seems OK to me. You didn't make these changes. https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... File components/omnibox/browser/keyword_provider_unittest.cc (right): https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:192: // Matches should be retrieved by typing the prefix of the keyword, Nit: Wrap comment all the way out to 80 columns https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:236: &AutocompleteMatch::fill_into_edit); Nit: Indenting https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:278: params[std::string(OmniboxFieldTrial::kKeywordRequiresPrefixMatchRule)] = Nit: Is the explicit std::string() here necessary? (2 places) https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:288: &AutocompleteMatch::fill_into_edit); Nit: Indenting https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:334: &AutocompleteMatch::fill_into_edit); Nit: While here: indenting https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:414: &AutocompleteMatch::contents); Nit: While here: indenting https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:182: keyword.length() - (registry_length ? (registry_length + 1) : 0), 0); Actually, I'm not convinced this will do the right thing. The compiler complaint about mismatched comparisons is highlighting that keyword.length() and registry_length are unsigned and the result of subtracting may underflow. Forcing max() to <int> will _probably_ do what you want, but I'm not totally sure of the vagaries of how the casts here work, so I'd rather avoid that. However, thinking more closely about this, there's no actual need to safe the code against the "co.uk" case. GetRegistryLength() is guaranteed to return a value less than the length of its input as long as the input is a valid host. Instead we need to safe against GetRegistryLength() returning npos, which it will do if the input string is not a valid host. So we can do something like: if (registry_length == std::string::npos) return keyword.length(); return keyword.length() - (registry_length ? (registry_length + 1) : 0); https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... components/search_engines/template_url_service.h:461: // Adds |template_url| from various internal maps Nit: from -> to https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... components/search_engines/template_url_service.h:478: // the template URL |template_url| under the key |keyword|. Nit: Awkward; maybe: Generic helper function for adding |template_url| to |keyword_to_template_map| with key |keyword|.
Here ya go. Sorry I missed those comments from earlier patchsets. I'm usually better about that. I blame it on my lack of sleep these last two days (for reasons entirely unrelated to work). I think we're almost done. Please take another look and hopefully we can wrap this up soon. thanks, mark https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2392: std::make_pair(kNullTemplateURL, 0)), On 2015/11/12 20:36:10, Peter Kasting wrote: > On 2015/11/11 07:41:00, Mark P wrote: > > On 2015/11/09 23:11:15, Peter Kasting wrote: > > > Nit: See earlier comment > > > > See earlier answer. :-) > > It may not be shorter to write, but I think just writing > TURLAndMeaningfulLength() here is semantically clearer. It means "I want the > default value of the mapped type for this container". Using make_pair() > explicitly emphasizes the importance of the values inside the make_pair() call, > but you don't actually care about what those are, and if the definition of > TURLAndMeaningfulLength changed in the future, you'd want this code to Just Keep > Working. Okay, it's plausible. Done here and everywhere there's a similar make_pair call. Did not change the make_pair calls that require(string, TURLAndMeaningfulLength), because there's no easy alias for that. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2401: } On 2015/11/12 20:36:10, Peter Kasting wrote: > On 2015/11/11 07:41:00, Mark P wrote: > > I couldn't figure out how to make copy_if work when converting between > different > > types like this. > > You're right. I overlooked the difference in container types. > > This is doable with separate transform() and copy_if() steps, but what you have > now is cleaner than that, so leave it. Acknowledged. https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/40001/components/search_engin... components/search_engines/template_url_service.h:407: KeywordDomainToTemplateMap; On 2015/11/12 22:32:11, Peter Kasting wrote: > On 2015/11/12 20:36:10, Peter Kasting wrote: > > On 2015/11/11 07:41:01, Mark P wrote: > > > On 2015/11/09 23:11:15, Peter Kasting wrote: > > > > I don't love "ToTemplateMap" as a name for both a map to a TURL* and a map > > to > > > a > > > > TURLAndEffectiveLength; it seems like these should have different names, > but > > I > > > > don't know what, and I'm hoping the comments will help make the name more > > > > obvious. > > > > > > I can simply do KeywordToTURLAndEffectiveLength and > > > KeywordDomainToTURLAndEffectiveLength instead of what's currently here. > Those > > > seems clear to me. What do you think? > > > > Seems OK to me. > > You didn't make these changes. Now done. Also changed the names of the internal variables correspondingly. Also did the same with GUIDToTemplateMap and its variable, which both seemed not-parallel and bothered me. https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:1526: it != keyword_domain_to_template_map_.upper_bound(domain); ) { On 2015/11/12 20:36:11, Peter Kasting wrote: > You should pull the upper_bound() out of the loop here; it will remain valid > across erase() calls, and you don't want to recalculate it on every iteration. Good point. Done, but then changed (see below) > > Once this is done, going back to the equal_range() method of finding the upper > and lower bounds is more efficient. > > I was mistaken to suggest remove_if() before since that can't be used with > associative containers, and I didn't point out that your previous code could > dereference a garbage iterator after erase() (but you fixed it anyway). So I > think just combining equal_range() with this loop body is the best way to go. Did equal_range() as you suggested, but it's longer than the previous code. I prefer the earlier version; I don't see what equal_range buys you that lower_bound and upper_bound does not. It's simply an extra variable. https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... File components/omnibox/browser/keyword_provider_unittest.cc (right): https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:192: // Matches should be retrieved by typing the prefix of the keyword, On 2015/11/12 22:32:11, Peter Kasting wrote: > Nit: Wrap comment all the way out to 80 columns Done. https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:236: &AutocompleteMatch::fill_into_edit); On 2015/11/12 22:32:11, Peter Kasting wrote: > Nit: Indenting Done here and all RunTest calls. https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:278: params[std::string(OmniboxFieldTrial::kKeywordRequiresPrefixMatchRule)] = On 2015/11/12 22:32:11, Peter Kasting wrote: > Nit: Is the explicit std::string() here necessary? (2 places) No, it's not necessary. Removed. This was a leftover remnant from an earlier version of this code. https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:288: &AutocompleteMatch::fill_into_edit); On 2015/11/12 22:32:11, Peter Kasting wrote: > Nit: Indenting Done. https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:334: &AutocompleteMatch::fill_into_edit); On 2015/11/12 22:32:11, Peter Kasting wrote: > Nit: While here: indenting Done. https://codereview.chromium.org/1411543011/diff/100001/components/omnibox/bro... components/omnibox/browser/keyword_provider_unittest.cc:414: &AutocompleteMatch::contents); On 2015/11/12 22:32:11, Peter Kasting wrote: > Nit: While here: indenting Done. https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:182: keyword.length() - (registry_length ? (registry_length + 1) : 0), 0); On 2015/11/12 22:32:11, Peter Kasting wrote: > Actually, I'm not convinced this will do the right thing. The compiler > complaint about mismatched comparisons is highlighting that keyword.length() and > registry_length are unsigned and the result of subtracting may underflow. > Forcing max() to <int> will _probably_ do what you want, but I'm not totally > sure of the vagaries of how the casts here work, so I'd rather avoid that. > > However, thinking more closely about this, there's no actual need to safe the > code against the "co.uk" case. GetRegistryLength() is guaranteed to return a > value less than the length of its input as long as the input is a valid host. > Instead we need to safe against GetRegistryLength() returning npos, which it > will do if the input string is not a valid host. So we can do something like: > > if (registry_length == std::string::npos) > return keyword.length(); > return keyword.length() - (registry_length ? (registry_length + 1) : 0); I don't think so. Suppose the user has a keyword "co.uk". A plausible registry length function will return 5 for this. The length of the keyword is also 5. Then we'll return 5 - (5+1) = -1 (+1 for the preceding dot, which doesn't exist). Corrected it again back to something like the earlier version that doesn't use max() and guards against this -1, plus with an additional guard against npos. https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... components/search_engines/template_url_service.h:461: // Adds |template_url| from various internal maps On 2015/11/12 22:32:11, Peter Kasting wrote: > Nit: from -> to Done. https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... components/search_engines/template_url_service.h:478: // the template URL |template_url| under the key |keyword|. On 2015/11/12 22:32:11, Peter Kasting wrote: > Nit: Awkward; maybe: > > Generic helper function for adding |template_url| to |keyword_to_template_map| > with key |keyword|. Okay. Done.
https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:182: keyword.length() - (registry_length ? (registry_length + 1) : 0), 0); On 2015/11/12 23:18:41, Mark P wrote: > On 2015/11/12 22:32:11, Peter Kasting wrote: > > Actually, I'm not convinced this will do the right thing. The compiler > > complaint about mismatched comparisons is highlighting that keyword.length() > and > > registry_length are unsigned and the result of subtracting may underflow. > > Forcing max() to <int> will _probably_ do what you want, but I'm not totally > > sure of the vagaries of how the casts here work, so I'd rather avoid that. > > > > However, thinking more closely about this, there's no actual need to safe the > > code against the "co.uk" case. GetRegistryLength() is guaranteed to return a > > value less than the length of its input as long as the input is a valid host. > > Instead we need to safe against GetRegistryLength() returning npos, which it > > will do if the input string is not a valid host. So we can do something like: > > > > if (registry_length == std::string::npos) > > return keyword.length(); > > return keyword.length() - (registry_length ? (registry_length + 1) : 0); > > I don't think so. Suppose the user has a keyword "co.uk". A plausible registry > length function will return 5 for this. No, it won't; the RCDS functions are all guaranteed not to act like this, because it misleads callers. "co.uk" is not a navigable hostname, so GetRegistryLength() returns 0. This part of the function contract is spelled out in the comments for those APIs. https://codereview.chromium.org/1411543011/diff/120001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/120001/components/search_engi... components/search_engines/template_url_service.cc:1527: })); What you want here is multimap::equal_range(): const auto match_range(keyword_domain_to_turl_and_length_.equal_range(domain)); Much more compact and readable :)
https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:182: keyword.length() - (registry_length ? (registry_length + 1) : 0), 0); On 2015/11/12 23:37:51, Peter Kasting wrote: > On 2015/11/12 23:18:41, Mark P wrote: > > On 2015/11/12 22:32:11, Peter Kasting wrote: > > > Actually, I'm not convinced this will do the right thing. The compiler > > > complaint about mismatched comparisons is highlighting that keyword.length() > > and > > > registry_length are unsigned and the result of subtracting may underflow. > > > Forcing max() to <int> will _probably_ do what you want, but I'm not totally > > > sure of the vagaries of how the casts here work, so I'd rather avoid that. > > > > > > However, thinking more closely about this, there's no actual need to safe > the > > > code against the "co.uk" case. GetRegistryLength() is guaranteed to return > a > > > value less than the length of its input as long as the input is a valid > host. > > > Instead we need to safe against GetRegistryLength() returning npos, which it > > > will do if the input string is not a valid host. So we can do something > like: > > > > > > if (registry_length == std::string::npos) > > > return keyword.length(); > > > return keyword.length() - (registry_length ? (registry_length + 1) : 0); > > > > I don't think so. Suppose the user has a keyword "co.uk". A plausible > registry > > length function will return 5 for this. > > No, it won't; the RCDS functions are all guaranteed not to act like this, > because it misleads callers. "co.uk" is not a navigable hostname, so > GetRegistryLength() returns 0. This part of the function contract is spelled > out in the comments for those APIs. Okay, revised and added a DCHECK to confirm your assertion. https://codereview.chromium.org/1411543011/diff/120001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/120001/components/search_engi... components/search_engines/template_url_service.cc:1527: })); On 2015/11/12 23:37:51, Peter Kasting wrote: > What you want here is multimap::equal_range(): > > const auto > match_range(keyword_domain_to_turl_and_length_.equal_range(domain)); > > Much more compact and readable :) Yes, that's nice. Ah, how I keep acquiring std/stl knowledge through code reviews. :-) Done.
LGTM https://codereview.chromium.org/1411543011/diff/140001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:1519: const auto match_range( I just realized "auto" might not compile here; the compiler might not know which equal_range() to select. Make sure this works first. https://codereview.chromium.org/1411543011/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:1521: for (KeywordDomainToTURLAndMeaningfulLength::const_iterator OTOH, you almost certainly _can_ use auto here. And that would fix what I think is a compile error with this: I think you need to use non-const iterators here, not const ones, since erase() requires a non-const iterator.
Thanks for the speedy reviews. --mark https://codereview.chromium.org/1411543011/diff/140001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:1519: const auto match_range( On 2015/11/13 00:00:41, Peter Kasting wrote: > I just realized "auto" might not compile here; the compiler might not know which > equal_range() to select. Make sure this works first. Acknowledged. It compiled locally on linux. I'll use the commit queue and see what happens regarding other compilers / platforms. https://codereview.chromium.org/1411543011/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:1521: for (KeywordDomainToTURLAndMeaningfulLength::const_iterator On 2015/11/13 00:00:41, Peter Kasting wrote: > OTOH, you almost certainly _can_ use auto here. And that would fix what I think > is a compile error with this: I think you need to use non-const iterators here, > not const ones, since erase() requires a non-const iterator. Funny that it didn't hit a compiler error. Fixed.
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/1411543011/#ps160001 (title: "auto")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411543011/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411543011/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1411543011/diff/220001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/220001/components/search_engi... components/search_engines/template_url_service.cc:1541: void TemplateURLService::AddToMap(TemplateURL* template_url) { FYI, I had to revise this function. To see my changes, compare this patchset with 8 or 9 (the last patchset you reviewed). Earlier, part of my earlier refactoring to make it shared between the regular map and the domain map, I made it do .insert() on the map. This doesn't work on the regular map because I wanted the replace behavior and .insert() doesn't do anything if the key already exists. Hence, I changed this function to only be used with the regular map and did the [] insert, and changed AddToDomainMap to do the .insert() method. It's too bad there isn't a common function that means insert (if not existing, or if applied to a multimap) or replace (if existing when applied to a non-multimap).
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/1411543011/#ps220001 (title: "finish converting AddToMap() calls")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411543011/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411543011/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/6456fb606e045e77b4efa9e8058dcd86209d992a Cr-Commit-Position: refs/heads/master@{#359505}
Message was sent while issue was closed.
https://codereview.chromium.org/1411543011/diff/220001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/1411543011/diff/220001/components/search_engi... components/search_engines/template_url_service.cc:1541: void TemplateURLService::AddToMap(TemplateURL* template_url) { On 2015/11/13 06:06:49, Mark P wrote: > FYI, I had to revise this function. To see my changes, compare this patchset > with 8 or 9 (the last patchset you reviewed). > > Earlier, part of my earlier refactoring to make it shared between the regular > map and the domain map, I made it do .insert() on the map. This doesn't work on > the regular map because I wanted the replace behavior and .insert() doesn't do > anything if the key already exists. Hence, I changed this function to only be > used with the regular map and did the [] insert, and changed AddToDomainMap to > do the .insert() method. It's too bad there isn't a common function that means > insert (if not existing, or if applied to a multimap) or replace (if existing > when applied to a non-multimap). For the record, I realized I could do something messy such value = TemplateURLAndMeaningfulLength(...) map->insert(std::make_pair(key, value)).second = value but that seems unnecessarily messy, and the current approach (one helper for each map) seems pretty clean to me. |
