Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8364001:
Strip special characters in extension omnibox suggestions. (Closed)
|
Created:
9 years, 2 months ago by Alexei Svitkine (slow) Modified:
9 years, 1 month ago CC:
chromium-reviews, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionStrip special characters in extension omnibox suggestions.
This makes the drop downs on Mac and Linux match how Windows
currently renders these suggestions when they include newline,
tab and carriage return characters.
Removes invalid characters in matches coming from extension suggestions
and apps.
BUG=100564
TEST=Manual per bug and added ExtensionAppProviderTest.CreateMatchSanitize KeywordProviderTest.SuggestionMatchSanitize tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107746
Patch Set 1 : '' #
Total comments: 1
Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 20
Patch Set 7 : '' #
Total comments: 3
Patch Set 8 : '' #
Total comments: 5
Patch Set 9 : '' #
Total comments: 5
Patch Set 10 : '' #
Messages
Total messages: 24 (0 generated)
http://codereview.chromium.org/8364001/diff/5/chrome/browser/ui/cocoa/omnibox... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): http://codereview.chromium.org/8364001/diff/5/chrome/browser/ui/cocoa/omnibox... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:114: RemoveChars(matchStringIn, kNewlineChar, &matchString); Should this remove the newline, or convert it to a space? What about tabs? Linefeeds? Random other control characters? As a more general question, it seems like this should be solvable at the point where the offending match contents are generated, rather than where they are used. Unfortunately, AutocompleteMatch is a struct where callers set contents directly. Since this problem is one where someone else controls what we display, ExtensionAppProvider::Start() might be the chokepoint you want. Are newlines legitimate contents of matches in general?
> Should this remove the newline, or convert it to a space? What about tabs? > Linefeeds? Random other control characters? The Windows path removes it (as a result of doing bidi-type segmentation of the string and drawing the parts in a loop). I've checked and it also eats tabs and carriage returns - so I've updated the code appropriately. > As a more general question, it seems like this should be solvable at the point > where the offending match contents are generated, rather than where they are > used. Unfortunately, AutocompleteMatch is a struct where callers set contents > directly. Since this problem is one where someone else controls what we > display, ExtensionAppProvider::Start() might be the chokepoint you want. Are > newlines legitimate contents of matches in general? I've though about this a bit more and now I tend to agree with you - so I've moved the logic to ExtensionAppProvider and added a test. (Originally, I had put this in Mac-specific code with the reasoning that given the same inputs - the different platform-specific implementations should display the same thing. However, now I agree that since extensions are the only place where we can get such strings we control the other code paths, it makes sense to put this logic there - also it fixes a similar inconsistency the GTK dropdown.)
On 2011/10/24 15:09:54, Alexei Svitkine wrote: > > Should this remove the newline, or convert it to a space? What about tabs? > > Linefeeds? Random other control characters? > > The Windows path removes it (as a result of doing bidi-type segmentation of the > string and drawing the parts in a loop). I've checked and it also eats tabs and > carriage returns - so I've updated the code appropriately. > > > As a more general question, it seems like this should be solvable at the point > > where the offending match contents are generated, rather than where they are > > used. Unfortunately, AutocompleteMatch is a struct where callers set contents > > directly. Since this problem is one where someone else controls what we > > display, ExtensionAppProvider::Start() might be the chokepoint you want. Are > > newlines legitimate contents of matches in general? > > I've though about this a bit more and now I tend to agree with you - so I've > moved the logic to ExtensionAppProvider and added a test. > > (Originally, I had put this in Mac-specific code with the reasoning that given > the same inputs - the different platform-specific implementations should display > the same thing. However, now I agree that since extensions are the only place > where we can get such strings we control the other code paths, it makes sense to > put this logic there - also it fixes a similar inconsistency the GTK dropdown.) Bah. I forgot to remove my previous change while testing this. It looks like ExtensionAppProvider is not the right code to change here...
I've updated the patch to address this in the actual code in KeywordProvider with another added test. I've also kept the changes to ExtensionAppProvider with the test, since potentially that's another way where a string with these characters could surface.
Things I'm worried about: * Is the list of characters to strip sufficient? Are there any Unicode characters that we need to strip for similar reasons? And on the flip side, do we really need to strip \t (and maybe \0?), or will that be drawn correctly? Finally, maybe we also want to strip leading/trailing whitespace entirely? * Making this a method on AutocompleteMatch kind of implies it can be called any time, but since it doesn't update the classifications, it's not actually safe to call after you've set one of those. I'm not sure what the ideal route is here; even things like DCHECKing that the classifications are empty doesn't save callers from calculating the classifications they're going to set using their local string instead of the stripped one. * Also, while practically the providers you've touched are the only places these sorts of characters can be introduced (I think? Unless page titles can contain them...), there's really an implicit assertion that none of the contents or descriptions for any matches will contain these. It would be nice if our code made that assertion clear. The only thing I can come up with is to maybe have a static method on AutocompleteMatch() that takes a string and returns the "stripped newline" string, with a comment that callers should call this for anything that comes from external sources (e.g. extensions). Then perhaps in the AutocompleteController, when matches are updated, DCHECK that this function is a no-op for the matches' contents and description fields. One nit is that I would generally not factor out code into a helper function if that helper is only called once, unless there are some big gains of some sort we get from doing so.
On 2011/10/24 23:43:26, Peter Kasting wrote: > Things I'm worried about: > > * Is the list of characters to strip sufficient? Are there any Unicode > characters that we need to strip for similar reasons? It might not be sufficient - but I'm not sure how to get an exhaustive list of such characters. We can always add more to the list since after - this CL provides the plumbing to easily do so in a later CL, if needed. > And on the flip side, do we really need to strip \t (and maybe \0?), or will that be drawn correctly? On Windows, \t is currently ignored whereas on Mac it's drawn as a tab (some number of spaces wide). Removing it makes the behavior consistent with what we currently see on Windows. If try to use \0 in the extensions, I get no suggestions at all - perhaps \0 is not valid in JS and is treated like a JS error? > Finally, maybe we also want to strip leading/trailing whitespace entirely? The goal is to make it consistent between platforms - leading/trailing whitespace is already consistent. I don't really see the need to remove them unless leaving them in would cause problems I'm not aware of. > The only thing I can come up with is to maybe have a static method on > AutocompleteMatch() that takes a string and returns the "stripped newline" > string, with a comment that callers should call this for anything that comes > from external sources (e.g. extensions). Then perhaps in the > AutocompleteController, when matches are updated, DCHECK that this function is a > no-op for the matches' contents and description fields. Done.
On 2011/10/25 17:05:14, Alexei Svitkine wrote: > On 2011/10/24 23:43:26, Peter Kasting wrote: > > Things I'm worried about: > > > > * Is the list of characters to strip sufficient? Are there any Unicode > > characters that we need to strip for similar reasons? > > It might not be sufficient - but I'm not sure how to get an exhaustive list of > such characters. We can always add more to the list since after - this CL > provides the plumbing to easily do so in a later CL, if needed. OK, fair enough. I'm just worried we won't notice problems, that's all. > > And on the flip side, do we really need to strip \t (and maybe \0?), or will > that be drawn correctly? > > On Windows, \t is currently ignored whereas on Mac it's drawn as a tab (some > number of spaces wide). Removing it makes the behavior consistent with what we > currently see on Windows. Sounds good. > If try to use \0 in the extensions, I get no > suggestions at all - perhaps \0 is not valid in JS and is treated like a JS > error? I dunno. If callers can't actually insert \0, I'd rather not strip it here. > > Finally, maybe we also want to strip leading/trailing whitespace entirely? > > The goal is to make it consistent between platforms - leading/trailing > whitespace is already consistent. I don't really see the need to remove them > unless leaving them in would cause problems I'm not aware of. This would be serving a second goal, which is "make all the results in the dropdown look consistent". We avoid adding leading or trailing whitespace in any of our built-in providers because it looks weird and wrong. I think we should probably go ahead and strip it here too to be safe.
> I dunno. If callers can't actually insert \0, I'd rather not strip it here. The code doesn't strip them (actually our RemoveChars() function can't!) - '\0' was for the null-terminator in the array. I've changed it to 0 now so it's more clear that it's the null terminator. > This would be serving a second goal, which is "make all the results in the > dropdown look consistent". We avoid adding leading or trailing whitespace in > any of our built-in providers because it looks weird and wrong. I think we > should probably go ahead and strip it here too to be safe. Done.
http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:656: for (size_t i = 0; i < matches.size(); i++) { Nit: This code tries to use iterators (begin() and end()) as much as possible. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:657: DCHECK_EQ(AutocompleteMatch::SanitizeString(matches[i].contents), Nit: Here and below, we should also check the description. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:668: // Check that there are no invalid characters in |match.contents|. Nit: Remove this comment, it's redundant with the code. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.cc (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:162: const char16 kRemoveChars[] = { '\n', '\r', '\t', 0 }; Nit: From looking at string_util.cc it seems like maybe we should add 0x2028 and 0x2029 (line separator and paragraph separator) to this list. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:164: TrimString(contents, kWhitespaceUTF16, &result); Nit: Use TrimWhitespace(contents, TRIM_ALL, &result); http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:128: // from external sources (such as extensions) before assigning to |contents|. Nit: ... or |description|. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:129: static string16 SanitizeString(const string16& str); Nit: For consistency with other functions, call the arg |text|. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/extension_app_provider.h (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/extension_app_provider.h:50: AutocompleteMatch CreateAutocompleteMatch(const AutocompleteInput& input, Nit: I'm not sure the benefit of this method is very large. It's a little unwieldy for the test code to call since the test only cares about sanitizing the contents but this method is worried about the whole match. Given the checks in autocomplete.cc, I would probably just revert the changes to this file and the unittest and merely add a sanitize call where the match is currently being constructed. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/extension_app_provider_unittest.cc (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/extension_app_provider_unittest.cc:119: AutocompleteInput input(ASCIIToUTF16("Test"), ASCIIToUTF16(""), Nit: ASCIIToUTF16("") -> string16() http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.h (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.h:137: AutocompleteMatch CreateAutocompleteMatchFromSuggestion( Nit: Same comments.
http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:656: for (size_t i = 0; i < matches.size(); i++) { On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: This code tries to use iterators (begin() and end()) as much as possible. Done. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:657: DCHECK_EQ(AutocompleteMatch::SanitizeString(matches[i].contents), On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: Here and below, we should also check the description. Done. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:668: // Check that there are no invalid characters in |match.contents|. On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: Remove this comment, it's redundant with the code. Done. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.cc (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:162: const char16 kRemoveChars[] = { '\n', '\r', '\t', 0 }; On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: From looking at string_util.cc it seems like maybe we should add 0x2028 and > 0x2029 (line separator and paragraph separator) to this list. Done. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:164: TrimString(contents, kWhitespaceUTF16, &result); On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: Use TrimWhitespace(contents, TRIM_ALL, &result); Done. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:128: // from external sources (such as extensions) before assigning to |contents|. On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: ... or |description|. Done. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:129: static string16 SanitizeString(const string16& str); On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: For consistency with other functions, call the arg |text|. Done. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/extension_app_provider.h (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/extension_app_provider.h:50: AutocompleteMatch CreateAutocompleteMatch(const AutocompleteInput& input, On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: I'm not sure the benefit of this method is very large. It's a little > unwieldy for the test code to call since the test only cares about sanitizing > the contents but this method is worried about the whole match. Given the checks > in autocomplete.cc, I would probably just revert the changes to this file and > the unittest and merely add a sanitize call where the match is currently being > constructed. While I understand your point view, I disagree. I think, having this in a separate function makes the calling code more readable and maintainable - so thing change is worth it just for that. I agree that there's less need for a unit test with the DCHECK()s in place, but I still feel having them is better than not (especially since they're written already). First, the test is next to the source file in question - rather than a DCHECK() in some other file that relies on someone hitting that path to be caught - which requires running e.g. an extension that actually does this. Also, who knows how the other file will get refactored in the future - it is possible that the DCHECK() may get lost. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/extension_app_provider_unittest.cc (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/extension_app_provider_unittest.cc:119: AutocompleteInput input(ASCIIToUTF16("Test"), ASCIIToUTF16(""), On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: ASCIIToUTF16("") -> string16() Done. http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.h (right): http://codereview.chromium.org/8364001/diff/13020/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.h:137: AutocompleteMatch CreateAutocompleteMatchFromSuggestion( On 2011/10/25 20:37:07, Peter Kasting wrote: > Nit: Same comments. Same reply as to the other one.
> maintainable - so thing change is worth it just for that. s/thing/the/
> > This would be serving a second goal, which is "make all the results in the > > dropdown look consistent". We avoid adding leading or trailing whitespace in > > any of our built-in providers because it looks weird and wrong. I think we > > should probably go ahead and strip it here too to be safe. > > Done. Looks like this made some unit tests fail - I'll investigate further tomorrow...
There are more problems this change doesn't solve. ExtensionOmniboxSuggestion::ApplyDefaultSuggestionForExtensionKeyword() can insert bogus characters into the AutocompleteMatch::contents that we don't sanitize anywhere. See also the comment below. http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... File chrome/browser/autocomplete/extension_app_provider.cc (right): http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... chrome/browser/autocomplete/extension_app_provider.cc:33: const AutocompleteInput& input, Nit: Indent all these 4. http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... chrome/browser/autocomplete/extension_app_provider.cc:38: // We have a match, might be a partial match. Nit: This comment belongs in the caller. http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:452: match.contents = AutocompleteMatch::SanitizeString(suggestion.description); We can't sanitize the string and not update the description styles to match. This is what I was worried about happening if we made the sanitization a member function on AutocompleteMatch but I guess making it static didn't prevent the problem. We may want to do this work in the ExtensionOmniboxSuggestion class instead.
You're right - thanks! Looks like it's actually more complicated than putting the logic in ExtensionOmniboxSuggestion class. It appears the styles are now specified by the extension and furthermore these come from an XML-like markup parsed from the description, per the following CL: http://codereview.chromium.org/5271009 So the right place to do this is actually in Javascript where the XML-like markup gets parsed before converting to styles, in this function: http://codereview.chromium.org/5271009/diff/14001/chrome/renderer/resources/e... On 2011/10/26 01:07:22, Peter Kasting wrote: > There are more problems this change doesn't solve. > ExtensionOmniboxSuggestion::ApplyDefaultSuggestionForExtensionKeyword() can > insert bogus characters into the AutocompleteMatch::contents that we don't > sanitize anywhere. > > See also the comment below. > > http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... > File chrome/browser/autocomplete/extension_app_provider.cc (right): > > http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... > chrome/browser/autocomplete/extension_app_provider.cc:33: const > AutocompleteInput& input, > Nit: Indent all these 4. > > http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... > chrome/browser/autocomplete/extension_app_provider.cc:38: // We have a match, > might be a partial match. > Nit: This comment belongs in the caller. > > http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... > File chrome/browser/autocomplete/keyword_provider.cc (right): > > http://codereview.chromium.org/8364001/diff/13033/chrome/browser/autocomplete... > chrome/browser/autocomplete/keyword_provider.cc:452: match.contents = > AutocompleteMatch::SanitizeString(suggestion.description); > We can't sanitize the string and not update the description styles to match. > This is what I was worried about happening if we made the sanitization a member > function on AutocompleteMatch but I guess making it static didn't prevent the > problem. We may want to do this work in the ExtensionOmniboxSuggestion class > instead.
I've changed the whitespace trimming to be a leading whitespace trim instead - since the other autocomplete backends keep the trailing whitespace if the text typed in the omnibox ends with whitespace. (It made many tests fail.) I've also reverted the changes to keyword_provider.cc and handled the extension suggestions in the JS code that parses the XML markup to ensure that style ranges are correct.
On 2011/10/26 15:00:57, Alexei Svitkine wrote: > I've changed the whitespace trimming to be a leading whitespace trim instead - > since the other autocomplete backends keep the trailing whitespace if the text > typed in the omnibox ends with whitespace. (It made many tests fail.) The only provider I know that does this is the search provider, and it's a bug that it does it ( http://code.google.com/p/chromium/issues/detail?id=99239 ). Can you change back to stripping both and update any tests that rely on this? This might mean that we have to comment out the DCHECKs in autocomplete.cc with a TODO to re-enable them once we fix that bug.
lgtm http://codereview.chromium.org/8364001/diff/17014/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.cc (right): http://codereview.chromium.org/8364001/diff/17014/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:164: const char16 kRemoveChars[] = { '\n', '\r', '\t', 0x2028, 0x2029, 0 }; Document the crazy code points so I don't have to search to figure out why they're in there. http://codereview.chromium.org/8364001/diff/17014/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:167: RemoveChars(result, kRemoveChars, &result); The potential for aliasing kinda frightens me, but AFAICT the implementation will work fine using result as both in and out.
On Wed, Oct 26, 2011 at 1:46 PM, <pkasting@chromium.org> wrote: > On 2011/10/26 15:00:57, Alexei Svitkine wrote: > The only provider I know that does this is the search provider, and it's a > bug > that it does it ( http://code.google.com/p/chromium/issues/detail?id=99239 > ). > Can you change back to stripping both and update any tests that rely on > this? Would you mind if I do this in a follow-up CL? -Alexei
http://codereview.chromium.org/8364001/diff/17014/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.cc (right): http://codereview.chromium.org/8364001/diff/17014/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:164: const char16 kRemoveChars[] = { '\n', '\r', '\t', 0x2028, 0x2029, 0 }; On 2011/10/26 21:08:00, shess wrote: > Document the crazy code points so I don't have to search to figure out why > they're in there. Done. http://codereview.chromium.org/8364001/diff/17014/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:167: RemoveChars(result, kRemoveChars, &result); On 2011/10/26 21:08:00, shess wrote: > The potential for aliasing kinda frightens me, but AFAICT the implementation > will work fine using result as both in and out. Yeah, this is actually documented as safe to do for RemoveChars() in string_util.h.
http://codereview.chromium.org/8364001/diff/17014/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.cc (right): http://codereview.chromium.org/8364001/diff/17014/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:167: RemoveChars(result, kRemoveChars, &result); On 2011/10/27 13:30:53, Alexei Svitkine wrote: > On 2011/10/26 21:08:00, shess wrote: > > The potential for aliasing kinda frightens me, but AFAICT the implementation > > will work fine using result as both in and out. > > Yeah, this is actually documented as safe to do for RemoveChars() in > string_util.h. Oh, cool, I embarrassingly went direct to the implementation and didn't see the documented contract.
LGTM http://codereview.chromium.org/8364001/diff/22001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.cc (right): http://codereview.chromium.org/8364001/diff/22001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:164: // 0x2028 = line separator; 0x2029 = paragraph separator. Nit: How about: const char16 kRemoveChars[] = { '\n', '\r', '\t', 0x2028, // Line separator 0x2029, // Paragraph separator 0 }; http://codereview.chromium.org/8364001/diff/22001/chrome/browser/autocomplete... File chrome/browser/autocomplete/extension_app_provider.cc (right): http://codereview.chromium.org/8364001/diff/22001/chrome/browser/autocomplete... chrome/browser/autocomplete/extension_app_provider.cc:46: AutocompleteMatch::ClassifyLocationInString(name_match_index, Nit: Wrapping to save lines would match the rest of this file better: AutocompleteMatch::ClassifyLocationInString(name_match_index, input.text().length(), name.length(), ACMatchClassification::NONE, &match.contents_class); This is really trivial, and the only other reason you might want to consider it is that it would allow you to avoid breaking the ?: in the CalculateRelevance() call below across two lines. http://codereview.chromium.org/8364001/diff/22001/chrome/browser/autocomplete... chrome/browser/autocomplete/extension_app_provider.cc:90: size_t name_match_index = matches_name ? Nit: Another shorter way of doing this, that I find equally readable: matches_.push_back(CreateAutocompleteMatch(input, name, url, matches_name ? static_cast<size_t>(name_iter - name.begin()) : string16::npos, matches_url ? static_cast<size_t>(url_iter - url.begin()) : string16::npos)); http://codereview.chromium.org/8364001/diff/22001/chrome/browser/autocomplete... File chrome/browser/autocomplete/extension_app_provider.h (right): http://codereview.chromium.org/8364001/diff/22001/chrome/browser/autocomplete... chrome/browser/autocomplete/extension_app_provider.h:79: FRIEND_TEST_ALL_PREFIXES(ExtensionAppProviderTest, CreateMatchSanitize); Nit: Normally we put this up above the typedefs although I think the style guide is silent about it. http://codereview.chromium.org/8364001/diff/22001/chrome/renderer/resources/e... File chrome/renderer/resources/extensions/extension_process_bindings.js (right): http://codereview.chromium.org/8364001/diff/22001/chrome/renderer/resources/e... chrome/renderer/resources/extensions/extension_process_bindings.js:488: result.description += sanitizeString(child.nodeValue); I don't actually know JS, so I'm going to assume this is right -- it looks as if the input here is a tree of nodes that never actually specify lengths/character ranges, so just sanitizing the text should be fine.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8364001/27002
Change committed as 107746 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
