Fixes Issue 7377: Regression: Omnibox trims URL ending with 0x85...

Fixes Issue 7377: Regression: Omnibox trims URL ending with 0x85
To fix this issue, this change adds a new function TrimWhitespaceUTF8(), which trims space characters (including non-printable characters and broken UTF-8 characters) from either end of a UTF-8 string.
Please feel free to give me your comments since I'm not sure this implimentation is correct. (Maybe this implementation trims too aggressively.)

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

[Evan Martin, 2009-02-10 20:38:42]

I will leave this for Jungshik to review, since he knows more about this than I
do.  I am glad you are fixing it, though!

[jungshik at Google, 2009-02-11 22:04:54]

http://codereview.chromium.org/20219/diff/18/1007
File base/string_util.h (right):

http://codereview.chromium.org/20219/diff/18/1007#newcode145
Line 145: TrimPositions positions,
I think this needs to be turned into TrimWhitespaceASCII and a prominent comment
should be added that this checks only for ASCII whitespace regardless of the
char. encoding of the input. And, two non-ASCII bytes in the implementation
should go.

http://codereview.chromium.org/20219/diff/18/1008
File base/string_util_icu.cc (right):

http://codereview.chromium.org/20219/diff/18/1008#newcode544
Line 544: }
This is different from what we do in std::wstring version of TrimWhitespace
because you bring in a new bunch of characters with |!u_isprint(c)| other than
U+200C (ZWNJ).
How much do we have to worry about the performance for TrimWhitespaceUTF8? I
guess not much. In that case, how about just making this call TrimWhiteSpace
(std::wstring version) after validating the input to be UTF-8 and converting it
to a wide string? Then, we have a single list for both UTF-8 and a wide string
versions. This does not solve the issue of keeping a hard-coded list and we have
to make sure that the list is current.

[Hironori Bono, 2009-02-12 07:10:56]

Thank you for your review and comments.

http://codereview.chromium.org/20219/diff/18/1008
File base/string_util_icu.cc (right):

http://codereview.chromium.org/20219/diff/18/1008#newcode544
Line 544: }
On 2009/02/11 22:04:54, Jungshik Shin wrote:
> This is different from what we do in std::wstring version of TrimWhitespace
because you bring in a new bunch of characters with |!u_isprint(c)| other than
U+200C (ZWNJ).

> How much do we have to worry about the performance for TrimWhitespaceUTF8? I
guess not much. In that case, how about just making this call TrimWhiteSpace
(std::wstring version) after validating the input to be UTF-8 and converting it
to a wide string? Then, we have a single list for both UTF-8 and a wide string
versions. This does not solve the issue of keeping a hard-coded list and we have
to make sure that the list is current.

My first implementation is mostly same as your suggested one: i.e.
 1. decodes an input string to std::wstring;
 2. calls the TrimWhiteSpace() function, and;
 3. re-encodes the output string in UTF-8.
(See the following code.)

TrimPositions TrimWhitespaceUTF8(const std::string& input,
                                 TrimPositions positions,
                                 std::string* output) {
  std::wstring input_wide = UTF8ToWide(input);
  std::wstring output_wide;
  TrimPositions result = TrimWhitespace(input_wide,
                                        positions,
                                        &output_wide);
  *output = WideToUTF8(output_wide);
  return result;
}

Unfortunately, I noticed this implementation did not run so fast. When I ran my
new unit-test 'StringUtilTest.TrimWhitespaceUTF8' one million times and measured
its speed, this implementation took 48.653 seconds on my Vista PC.
To improve the speed, I avoided conversions and memory allocations and wrote the
current implementation. My current implementation just took 4.992 seconds for
the same test on the same PC.

[jungshik at Google, 2009-02-12 22:23:03]

On 2009/02/12 07:10:56, hbono wrote:

> Unfortunately, I noticed this implementation did not run so fast. When I ran
my
> new unit-test 'StringUtilTest.TrimWhitespaceUTF8' one million times and
measured
> its speed, this implementation took 48.653 seconds on my Vista PC.
> To improve the speed, I avoided conversions and memory allocations and wrote
the
> current implementation. My current implementation just took 4.992 seconds for
> the same test on the same PC.

Your unit test contains about 10 calls to TrimWhitespaceUTF8. So, roughly
speaking, each call takes about 0.49 us and 4.9 us, respectively. Do we have to
worry about that? The only caller is URL_FixerUpper.  

How about comparing the perf. of url_fixer_upper_unittest with two approaches?

[jungshik at Google, 2009-02-13 22:49:46]

I'm pulling in Peter. 

My gut feeling is that 0.49 us vs 4.9 us in TrimWhitespaceUTF8 called in
url_..fixer wouldn't affect the omnibox, but it'd be nice if you could do what I
asked in the previous comment to make sure. 

Otherwise, lgtm with the comment below addressed.

http://codereview.chromium.org/20219/diff/40/44
File base/string_util_icu.cc (right):

http://codereview.chromium.org/20219/diff/40/44#newcode540
Line 540: DCHECK(IsValidUTF8(input));
base/string_util has IsStringUTF8. If you use that, you can just put it in
string_util.cc

[Peter Kasting, 2009-02-13 22:57:30]

On 2009/02/13 22:49:46, Jungshik Shin wrote:
> I'm pulling in Peter. 
> 
> My gut feeling is that 0.49 us vs 4.9 us in TrimWhitespaceUTF8 called in
> url_..fixer wouldn't affect the omnibox, but it'd be nice if you could do what
I
> asked in the previous comment to make sure.

What other choice do we have, if we want correctness?

My instinct is that this shouldn't matter, as we call this function maybe dozens
of times, not thousands (I think), when executing omnibox code.  I don't know
whether we call this anywhere else time-critical, like while loading webpages. 
I would keep an eye on the page cycler numbers after landing this to make sure
they don't go up.

[Hironori Bono, 2009-02-17 08:58:06]

To read comments from jshin and pkasting, I'm convinced this
TrimWhiteSpaceUTF8() function does not have to work so fast. So, I changed this
function to use the std::wstring version of the TrimWhiteSpace() function.
Also, I added a few test cases to the unit-test "URLFixerUpperTest.FixUpURL" to
prevent regressions.
Is it possible to review it again?

[jungshik at Google, 2009-02-17 18:28:11]

http://codereview.chromium.org/20219/diff/1048/73
File base/string_util.cc (right):

http://codereview.chromium.org/20219/diff/1048/73#newcode327
Line 327: 0x200D,  // Zero Width Joiner
kWhitespaceWide is also used in IsWhitespace(), which is in turn used in
CollapseWhitespace. CollapseWhitespace replaces  all the characters (in
kWhitespaceWide) with U+0020 even in the middle of a string. With ZWJ and ZWNJ,
it can break some Indic scripts and Arabic. We have to revisit the issue in a
separate bug (I just filed bug 7796). In the meantime, let's not add U+200D.

http://codereview.chromium.org/20219/diff/1048/73#newcode343
Line 343: '\xa0',  // No-Break Space
Sorry I forgot to mention this earlier. (I did mention in the email thread,
though :-)). 
I'd rather get rid of 0x85 and 0xa0 here, rename TrimWhitespace(std::string) to
TrimWhitespaceASCII and add DCHECK(IsStringASCII(input))

http://codereview.chromium.org/20219/diff/1048/72
File base/string_util.h (right):

http://codereview.chromium.org/20219/diff/1048/72#newcode133
Line 133: // version instead).
With TrimWhitespaceUTF8 added, this comment has to be changed. And, if you
rename TrimWhitespace (std::string) with TrimWhitespaceASCII, that needs to be
reflected in the comment as well.

http://codereview.chromium.org/20219/diff/1048/74
File base/string_util_icu.cc (right):

http://codereview.chromium.org/20219/diff/1048/74#newcode13
Line 13: #include "unicode/uchar.h"
you don't need this header any more.

http://codereview.chromium.org/20219/diff/1048/74#newcode536
Line 536: std::string* output) {
This is not ICU-specific any more. You can put it in string_util.cc

http://codereview.chromium.org/20219/diff/1048/76
File chrome/browser/net/url_fixer_upper_unittest.cc (right):

http://codereview.chromium.org/20219/diff/1048/76#newcode180
Line 180: // URLs which end with an ISO-8859 NEL (0x95).
URLs in UTF-8 which end with 0x85 and 0xA0. Add a test with a URL ending with
0xA0 as well because we used to break that as well.

[Hironori Bono, 2009-02-19 05:12:07]

Thank you for your review and sorry for my slow update.
When I applied all requested changes, I noticed this change somehow broke Mac
build because of the lack of "libicui18n.a". To fix Mac build, I added a
reference to "libicui18n.a".

http://codereview.chromium.org/20219/diff/1048/73
File base/string_util.cc (right):

http://codereview.chromium.org/20219/diff/1048/73#newcode327
Line 327: 0x200D,  // Zero Width Joiner
On 2009/02/17 18:28:11, Jungshik Shin wrote:
> kWhitespaceWide is also used in IsWhitespace(), which is in turn used in
> CollapseWhitespace. CollapseWhitespace replaces  all the characters (in
> kWhitespaceWide) with U+0020 even in the middle of a string. With ZWJ and
ZWNJ,
> it can break some Indic scripts and Arabic. We have to revisit the issue in a
> separate bug (I just filed bug 7796). In the meantime, let's not add U+200D. 

Done.

http://codereview.chromium.org/20219/diff/1048/73#newcode343
Line 343: '\xa0',  // No-Break Space
On 2009/02/17 18:28:11, Jungshik Shin wrote:
> Sorry I forgot to mention this earlier. (I did mention in the email thread,
> though :-)). 
> I'd rather get rid of 0x85 and 0xa0 here, rename TrimWhitespace(std::string)
to
> TrimWhitespaceASCII and add DCHECK(IsStringASCII(input))

I have changed TrimWhitespace() to TrimWhiteSpaceASCII(). Now TrimWhitespace()
just calls TrimWhitespaceASCII() for backward-compatibility.
On the other hand, I abandoned adding DCHECK() because it caused crashes on some
hunspell-related unit tests. I noticed I needed to change some more files to fix
the crashes. When I can fix all crashes, I will add DCHECK() here.

http://codereview.chromium.org/20219/diff/1048/74
File base/string_util_icu.cc (right):

http://codereview.chromium.org/20219/diff/1048/74#newcode13
Line 13: #include "unicode/uchar.h"
On 2009/02/17 18:28:11, Jungshik Shin wrote:
> you don't need this header any more.

Done.

http://codereview.chromium.org/20219/diff/1048/74#newcode536
Line 536: std::string* output) {
On 2009/02/17 18:28:11, Jungshik Shin wrote:
> This is not ICU-specific any more. You can put it in string_util.cc

Done.

http://codereview.chromium.org/20219/diff/1048/76
File chrome/browser/net/url_fixer_upper_unittest.cc (right):

http://codereview.chromium.org/20219/diff/1048/76#newcode180
Line 180: // URLs which end with an ISO-8859 NEL (0x95).
On 2009/02/17 18:28:11, Jungshik Shin wrote:
> URLs in UTF-8 which end with 0x85 and 0xA0. Add a test with a URL ending with
> 0xA0 as well because we used to break that as well.

Done. I also added UTF-8 strings which end with 0xA0 to the
StringUtilTest.TrimWhitespaceUTF8.

[jungshik at Google, 2009-02-19 18:04:05]

LGTM !

When landing, please take care of two minor nits (in comments). Thank you.

http://codereview.chromium.org/20219/diff/2001/2002
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/20219/diff/2001/2002#newcode96
Line 96: // UTF-8 strings that end with an ISO-8859 next line (0x85).
UTF-8 strings that end with 0x85 (NEL in ISO-8859).

http://codereview.chromium.org/20219/diff/2001/2002#newcode101
Line 101: // UTF-8 strings that end with a non-break space (0xA0).
UTF-8 strings that end with 0xA0 (non-break space in ISO-8859-1).