Strip carriage return from find text; show newline symbols.

Strip carriage return from find text; show newline symbols.

Blink won't find carriage returns from Windows clipboards.
Strip those characters before calling WebContents::Find.

The newline character is displayed wrong in textfields.
Swap it out for the Unicode newline symbol: ␤ (0x2424).
(the glyph isn't available on Win Views for me now...)

See before/after pics at http://crbug.com/371450#c7

BUG=371450
TEST=Find-in-page works well for string containing line breaks.
R=pkasting@chromium.org
TBR=asvitkine@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282102

[msw, 2014-07-09 00:20:11]

Hey Peter and Oshima, please take a look; thanks!

[Peter Kasting, 2014-07-09 00:27:05]

LGTM

https://codereview.chromium.org/374253002/diff/1/chrome/browser/ui/find_bar/f...
File chrome/browser/ui/find_bar/find_tab_helper.cc (right):

https://codereview.chromium.org/374253002/diff/1/chrome/browser/ui/find_bar/f...
chrome/browser/ui/find_bar/find_tab_helper.cc:44:
base::RemoveChars(search_string, kInvalidChars, &search_string);
You can just pass L"\r" here directly, no need to declare a separate array.  (I
consider this not only shorter and more readable but safer, since that way it's
impossible to forget the trailing \0 or have someone come along and accidentally
remove it later.)

Add a comment about why we're doing this (namely, that text in the renderer
doesn't have carriage returns).  (Actually, is it possible for authors to
explicitly force CRs into their text somehow?  Or will Blink always strip them?)

https://codereview.chromium.org/374253002/diff/1/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/374253002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1179: base::ReplaceChars(layout_text_, kNewline,
kNewlineSymbol, &layout_text_);
Again, just call with string constants directly instead of character arrays.

[msw, 2014-07-09 00:40:12]

Oshima, please take a look; thanks!

https://codereview.chromium.org/374253002/diff/1/chrome/browser/ui/find_bar/f...
File chrome/browser/ui/find_bar/find_tab_helper.cc (right):

https://codereview.chromium.org/374253002/diff/1/chrome/browser/ui/find_bar/f...
chrome/browser/ui/find_bar/find_tab_helper.cc:44:
base::RemoveChars(search_string, kInvalidChars, &search_string);
On 2014/07/09 00:27:05, Peter Kasting wrote:
> You can just pass L"\r" here directly, no need to declare a separate array. 
(I
> consider this not only shorter and more readable but safer, since that way
it's
> impossible to forget the trailing \0 or have someone come along and
accidentally
> remove it later.)

Done.

> Add a comment about why we're doing this (namely, that text in the renderer
> doesn't have carriage returns).

Done.

> (Actually, is it possible for authors to
> explicitly force CRs into their text somehow?  Or will Blink always strip
them?)

Both of these work with my CL, not sure what else to try:
data:text/html,hello<br>world
data:text/html,hello<p>world

https://codereview.chromium.org/374253002/diff/1/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/374253002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:1179: base::ReplaceChars(layout_text_, kNewline,
kNewlineSymbol, &layout_text_);
On 2014/07/09 00:27:05, Peter Kasting wrote:
> Again, just call with string constants directly instead of character arrays.

Done.

[Peter Kasting, 2014-07-09 00:43:26]

https://codereview.chromium.org/374253002/diff/1/chrome/browser/ui/find_bar/f...
File chrome/browser/ui/find_bar/find_tab_helper.cc (right):

https://codereview.chromium.org/374253002/diff/1/chrome/browser/ui/find_bar/f...
chrome/browser/ui/find_bar/find_tab_helper.cc:44:
base::RemoveChars(search_string, kInvalidChars, &search_string);
On 2014/07/09 00:40:12, msw wrote:
> Both of these work with my CL, not sure what else to try:
> data:text/html,hello<br>world
> data:text/html,hello<p>world

I'm thinking of pages that actually contain, say, a <pre> block with CR and/or
CRLF characters in it.

Really a Blink person would be more of an authority here.

[msw, 2014-07-09 15:39:34]

I had to revert L"\r", etc. back to char16 arrays for posix support.
(lmk if you'd prefer base::ASCIIToUTF16("\r"), etc.)

https://codereview.chromium.org/374253002/diff/1/chrome/browser/ui/find_bar/f...
File chrome/browser/ui/find_bar/find_tab_helper.cc (right):

https://codereview.chromium.org/374253002/diff/1/chrome/browser/ui/find_bar/f...
chrome/browser/ui/find_bar/find_tab_helper.cc:44:
base::RemoveChars(search_string, kInvalidChars, &search_string);
On 2014/07/09 00:43:26, Peter Kasting wrote:
> On 2014/07/09 00:40:12, msw wrote:
> > Both of these work with my CL, not sure what else to try:
> > data:text/html,hello<br>world
> > data:text/html,hello<p>world
> 
> I'm thinking of pages that actually contain, say, a <pre> block with CR and/or
> CRLF characters in it.
> 
> Really a Blink person would be more of an authority here.

The patch works well in my tests of the following (also omitting <pre>):
data:text/html,<pre>hello%0Aworld</pre>
data:text/html,<pre>hello%0Dworld</pre>
data:text/html,<pre>hello%0A%0Dworld</pre>
data:text/html,<pre>hello%0D%0Aworld</pre>

+CC fsamuel, jam, nasko for any other advice, per r243709.

[oshima, 2014-07-09 18:05:37]

I don't think you need my review for this CL. I defer it to peter.

[msw, 2014-07-09 19:18:35]

TBR'ing Alexei instead for ui/gfx/OWNERS; landing.

[msw, 2014-07-09 19:18:42]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-09 19:21:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/374253002/40001

[commit-bot: I haz the power, 2014-07-09 19:53:57]

Change committed as 282102