Fix AutocompleteMatch DCHECK() triggered by |RenderViewContextMenu::AppendSearchProvider()|.

Fix AutocompleteMatch DCHECK() triggered by |RenderViewContextMenu::AppendSearchProvider()|.

Makes |AppendSearchProvider()| sanitize the text by replacing unwanted whitespace
characters with spaces. Do this instead of calling |AutocompleteMatch::SanitizeString()|,
since we want to preserve the whitespace between words separated by newlines here, rather
than appending such words together.

Added |ReplaceChars()| to base/string_util to support replacing any occurence of the
given characters with a replacement string.

BUG=103338
TEST=Right click on multiline text in a text area in a Debug build. A DCHECK() shouldn't trigger.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109430

[Alexei Svitkine (slow), 2011-11-09 17:46:30]

Peter: Please review autocomplete_match and render_view_context_menu changes.
Will: Please review base/string_util changes.

[willchan no longer on Chromium, 2011-11-09 19:52:57]

Sending to mark since I think he's more familiar with string_util. I'm a bit
surprised that we don't already have some sort of ReplaceChar library function.
Perhaps something else exists or there's a reason we didn't put it in
string_util.h. Anyway, I'll defer to Mark here.

[Mark Mentovai, 2011-11-09 20:31:31]

This is a pretty specific-purpose function, but it’s probably OK to take in
string_util if you fix these things up. My last comment is more of an answer to
Will’s question than an actual comment for you, Alexei.

http://codereview.chromium.org/8502027/diff/7002/base/string_util.cc
File base/string_util.cc (right):

http://codereview.chromium.org/8502027/diff/7002/base/string_util.cc#newcode194
base/string_util.cc:194: bool ReplaceChars(const std::string& input,
This is really easy to write a test for, but you haven’t written any tests.

http://codereview.chromium.org/8502027/diff/7002/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/8502027/diff/7002/base/string_util.h#newcode187
base/string_util.h:187: // |replace_with|.  Returns true if any characters were
removed.
Removed? Replaced!

http://codereview.chromium.org/8502027/diff/7002/base/string_util.h#newcode187
base/string_util.h:187: // |replace_with|.  Returns true if any characters were
removed.
This doesn’t really explain whether ReplaceChars replaces by transliteration
(requiring replace_chars[] and replace_with to be the same length or something),
by replacing each char with the entire replace_with string, or something else.
The correct answer is #2, but I presumed that this behaved as #1 until I read
the code. This comment should make the precise behavior clear.

http://codereview.chromium.org/8502027/diff/7002/base/string_util.h#newcode448
base/string_util.h:448: BASE_EXPORT void ReplaceSubstringsAfterOffset(
We’ve got this, but it replaces substrings, not entire sets of characters. Maybe
this is what Will was thinking of. There’s also <algorithm> std::replace
(referenced by the comment above), but that, too, can only replace a single
character with another single character. Neither can replace sets of characters.

[Alexei Svitkine (slow), 2011-11-09 21:03:58]

Thanks Mark!

http://codereview.chromium.org/8502027/diff/7002/base/string_util.cc
File base/string_util.cc (right):

http://codereview.chromium.org/8502027/diff/7002/base/string_util.cc#newcode194
base/string_util.cc:194: bool ReplaceChars(const std::string& input,
On 2011/11/09 20:31:32, Mark Mentovai wrote:
> This is really easy to write a test for, but you haven’t written any tests.

Done.

http://codereview.chromium.org/8502027/diff/7002/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/8502027/diff/7002/base/string_util.h#newcode187
base/string_util.h:187: // |replace_with|.  Returns true if any characters were
removed.
On 2011/11/09 20:31:32, Mark Mentovai wrote:
> Removed? Replaced!

Done.

http://codereview.chromium.org/8502027/diff/7002/base/string_util.h#newcode187
base/string_util.h:187: // |replace_with|.  Returns true if any characters were
removed.
On 2011/11/09 20:31:32, Mark Mentovai wrote:
> This doesn’t really explain whether ReplaceChars replaces by transliteration
> (requiring replace_chars[] and replace_with to be the same length or
something),
> by replacing each char with the entire replace_with string, or something else.
> The correct answer is #2, but I presumed that this behaved as #1 until I read
> the code. This comment should make the precise behavior clear.

Done.

[Mark Mentovai, 2011-11-09 21:08:38]

LGTM with this change.

http://codereview.chromium.org/8502027/diff/2006/base/string_util_unittest.cc
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/8502027/diff/2006/base/string_util_unittest.cc...
base/string_util_unittest.cc:1097: DCHECK_EQ(cases[i].result, result);
You don’t want DCHECK_EQ, you want EXPECT_EQ. Same on the next line.

[Alexei Svitkine (slow), 2011-11-09 21:13:17]

http://codereview.chromium.org/8502027/diff/2006/base/string_util_unittest.cc
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/8502027/diff/2006/base/string_util_unittest.cc...
base/string_util_unittest.cc:1097: DCHECK_EQ(cases[i].result, result);
On 2011/11/09 21:08:38, Mark Mentovai wrote:
> You don’t want DCHECK_EQ, you want EXPECT_EQ. Same on the next line.

Doh, I knew that. Done.

[Mark Mentovai, 2011-11-09 21:15:42]

LGTM

[Peter Kasting, 2011-11-09 23:20:29]

My parts LGTM.

Passing question: Can we change all these string_util functions that take
null-terminated arrays to instead take a const std::vector<char>&, const
std::string& (with each character being one invalid character), or const char[]
+ size_t?  All three of these methods seem safer to me than the current method.

I don't expect you to do this in this change.  If you think it's worth doing but
don't want to do it yourself one of us can file a bug on it.

[commit-bot: I haz the power, 2011-11-10 04:32:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8502027/11002

[Alexei Svitkine (slow), 2011-11-10 04:54:22]

>
> Passing question: Can we change all these string_util functions that take
> null-terminated arrays to instead take a const std::vector<char>&, const
> std::string& (with each character being one invalid character), or const
> char[]
> + size_t?  All three of these methods seem safer to me than the current
> method.
>

I'd vote for array + size_t length, since the other suggestions would make
it difficult to use static constants for these without introducing evil
static initializers.

-Alexei

[commit-bot: I haz the power, 2011-11-10 06:48:57]

Try job failure for 8502027-11002 (retry) (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-10 13:33:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8502027/11002

[commit-bot: I haz the power, 2011-11-10 15:08:56]

Change committed as 109430

[Peter Kasting, 2011-11-15 00:55:05]

On 2011/11/10 04:54:22, Alexei Svitkine wrote:
> >
> > Passing question: Can we change all these string_util functions that take
> > null-terminated arrays to instead take a const std::vector<char>&, const
> > std::string& (with each character being one invalid character), or const
> > char[]
> > + size_t?  All three of these methods seem safer to me than the current
> > method.
> >
> 
> I'd vote for array + size_t length, since the other suggestions would make
> it difficult to use static constants for these without introducing evil
> static initializers.

const std::string& would work fine for that too, as it would construct the
string at the time of the call.

Were you planning to make this change, file a bug, or should I file?  I didn't
want this to get lost.

[Alexei Svitkine (slow), 2011-11-15 05:07:06]

On Mon, Nov 14, 2011 at 7:55 PM, <pkasting@chromium.org> wrote:

> On 2011/11/10 04:54:22, Alexei Svitkine wrote:
>
>> >
>> > Passing question: Can we change all these string_util functions that
>> take
>> > null-terminated arrays to instead take a const std::vector<char>&, const
>> > std::string& (with each character being one invalid character), or const
>> > char[]
>> > + size_t?  All three of these methods seem safer to me than the current
>> > method.
>> >
>>
>
>  I'd vote for array + size_t length, since the other suggestions would make
>> it difficult to use static constants for these without introducing evil
>> static initializers.
>>
>
> const std::string& would work fine for that too, as it would construct the
> string at the time of the call.
>

In that case, a src/base/string_piece.h would be a better choice to avoid
std::string needlessly allocing a buffer and copying chars for the
temporary.


> Were you planning to make this change, file a bug, or should I file?  I
> didn't
> want this to get lost.
>

I'll let you file the bug.

-Alexei