Replaces some TrimWhitespace with TrimWhitespaceASCII or TrimWhitespaceUTF8....

Replaces some TrimWhitespace with TrimWhitespaceASCII or TrimWhitespaceUTF8.

BUG=9851
TEST=run base_unittests.exe, unit_tests.exe and ui_tests.exe

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

[tfarina (gmail-do not use), 2009-08-11 14:43:57]

Hi Hironori,

In this first patch I replace the TrimWhitespace that use std::string, the
others TrimWhitespace uses std::wstring so I couldn't make the change in those
cases.

Thanks!

[Hironori Bono, 2009-08-12 04:07:40]

tfarina,

Thank you so much for your change and sorry for the lack of my explanation.
Another purpose of Issue 9851 is adding DCHECK(IsStringASCII()) in
TrimWhiteSpaceASCII() as we added DCHECK(IsStringUTF8()) in
TrimWhiteSpaceUTF8().
So, would it be possible to add DCHECK(IsStringASCII(input)) in
TrimWhiteSpaceASCII() (as listed below) and check if all of base_unittests.exe,
unit_tests.exe and ui_tests.exe tests work without errors or crashes? (If I
recall correctly, convert_dict:TrimList() calls TrimWhiteSpace() with a UTF-8
string.)

 TrimPositions TrimWhitespaceASCII(const std::string& input,
                                   TrimPositions positions,
                                   std::string* output) {
+  DCHECK(IsStringASCII(input));
   return TrimStringT(input, kWhitespaceASCII, positions, output);
 }

Also, is it possible to update the "TEST=" line in the description of this
change as listed below?

- TEST=None
+ TEST=run base_unittests.exe, unit_tests.exe and ui_tests.exe

Regards,

Hironori Bono

On 2009/08/11 14:43:57, tfarina wrote:
> Hi Hironori,
> 
> In this first patch I replace the TrimWhitespace that use std::string, the
> others TrimWhitespace uses std::wstring so I couldn't make the change in those
> cases.
> 
> Thanks!

[Hironori Bono, 2009-08-13 09:28:00]

tfarina,

Thank you for your update.
I notice "chrome\browser\net\url_fixer_upper.cc" also calls TrimWhiteSpace() and
this file raises lots of assertion errors while running "unit_tests.exe" on
Windows (Debug). Is it possible to change TrimWhitespace() calls in this file as
shown in the following patch?

Regards,

Hironori Bono

Index: chrome/browser/net/url_fixer_upper.cc
===================================================================
--- chrome/browser/net/url_fixer_upper.cc       (revision 23261)
+++ chrome/browser/net/url_fixer_upper.cc       (working copy)
@@ -75,9 +75,11 @@
 // does some basic fixes for input that we want to test for file-ness
 static void PrepareStringForFileOps(const FilePath& text,
                                     FilePath::StringType* output) {
+#if defined(OS_WIN)
   TrimWhitespace(text.value(), TRIM_ALL, output);
-#if defined(OS_WIN)
   replace(output->begin(), output->end(), '/', '\\');
+#else
+  TrimWhitespaceUTF8(text.value(), TRIM_ALL, output);
 #endif
 }

@@ -430,7 +432,7 @@
 string URLFixerUpper::FixupURL(const string& text,
                                const string& desired_tld) {
   string trimmed;
-  TrimWhitespace(text, TRIM_ALL, &trimmed);
+  TrimWhitespaceUTF8(text, TRIM_ALL, &trimmed);
   if (trimmed.empty())
     return string();  // Nothing here.

[tfarina (gmail-do not use), 2009-08-13 17:39:48]

On 2009/08/13 09:28:00, hbono wrote:
> tfarina,
> 
> Thank you for your update.
> I notice "chrome\browser\net\url_fixer_upper.cc" also calls TrimWhiteSpace()
and
> this file raises lots of assertion errors while running "unit_tests.exe" on
> Windows (Debug). Is it possible to change TrimWhitespace() calls in this file
as
> shown in the following patch?
> 
> Regards,
> 
> Hironori Bono
> 
> Index: chrome/browser/net/url_fixer_upper.cc
> ===================================================================
> --- chrome/browser/net/url_fixer_upper.cc       (revision 23261)
> +++ chrome/browser/net/url_fixer_upper.cc       (working copy)
> @@ -75,9 +75,11 @@
>  // does some basic fixes for input that we want to test for file-ness
>  static void PrepareStringForFileOps(const FilePath& text,
>                                      FilePath::StringType* output) {
> +#if defined(OS_WIN)
>    TrimWhitespace(text.value(), TRIM_ALL, output);
> -#if defined(OS_WIN)
>    replace(output->begin(), output->end(), '/', '\\');
> +#else
> +  TrimWhitespaceUTF8(text.value(), TRIM_ALL, output);
>  #endif
>  }
> 
> @@ -430,7 +432,7 @@
>  string URLFixerUpper::FixupURL(const string& text,
>                                 const string& desired_tld) {
>    string trimmed;
> -  TrimWhitespace(text, TRIM_ALL, &trimmed);
> +  TrimWhitespaceUTF8(text, TRIM_ALL, &trimmed);
>    if (trimmed.empty())
>      return string();  // Nothing here.
Fixed the url_fixer_upper.cc.

[Hironori Bono, 2009-08-17 07:02:57]

tfarina,

Thank you for your update and sorry for my slow review.
As far as I have tested this change with my machines (Win, Mac, and Linux), all
tests finishes without errors when I changed one line from your change as listed
below.
Is it possible to update your change to apply the following comment?

Regards,

Hironori Bono

http://codereview.chromium.org/165297/diff/1035/1041
File chrome/tools/convert_dict/hunspell_reader.cc (right):

http://codereview.chromium.org/165297/diff/1035/1041#newcode23
Line 23: TrimWhitespaceASCII(*line, TRIM_ALL, line);
Each line of a hunspell dictionary is encoded in UTF-8. So, is it possible to
replace this TrimWhiteSpaceASCII() with TrimWhiteSpaceUTF8().

[tfarina (gmail-do not use), 2009-08-17 13:12:25]

Hi hbono,

Thank you so much for you review! 
I fixed the hunspell_reader.cc.

Regards

[Hironori Bono, 2009-08-18 02:19:07]

LGTM.
Thank you again for your clean-up work.
Is it possible for you to commit this change by yourself, or shall I commit it
on your behalf?

Regards,

[tfarina (gmail-do not use), 2009-08-18 02:51:53]

On 2009/08/18 02:19:07, hbono wrote:
> LGTM.
> Thank you again for your clean-up work.
> Is it possible for you to commit this change by yourself, or shall I commit it
> on your behalf?
Hi hbono,

No, I don't have commit access. If you can, commit it for me, please.

Regards.
> 
> Regards,

[Hironori Bono, 2009-08-19 09:53:38]

This change has been landed as r23703.
Thank you for your work.

Regards,