base: Finish moving the SplitString functions from string_util.h to string_split.h

base: Finish moving the SplitString functions from string_util.h to string_split.h

BUG=None
TEST=trybos

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

[tfarina, 2010-09-17 15:56:09]

Brett, could you review this to me?

To fix the following failure on rlz library
rlz\win\lib\machine_deal.cc(111) : error C3861: 'SplitString': identifier not
found

My idea is to copy the SplitString implementation in the rlz library and use it
instead the version from the base. Then I will have to roll the deps for rlz
before landing this. After that I can roll the deps for base in rlz library and
remove the copied version of SplitString and finally I will roll the deps in
chromium for the rlz library fix.

What you think?

[tfarina, 2010-09-18 04:16:43]

It seems a simple solution would be include string_split in string_util (like we
already do for stringprintf).

[brettw, 2010-09-20 18:07:03]

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

http://codereview.chromium.org/3447008/diff/7002/35009#newcode28
base/string_util.h:28: #include "base/string_split.h"
If you look down below for RLZ_WIN_LIB_RLZ_LIB_H_, you can see how I temporarily
worked around this for ASCIITo *. Maybe you can just add a declaration for the
function you want in that block, or add a similar block for the file you want.
This will keep RLZ compiling until we can fix it, and will also prevent people
from depending on this include that you'll have to fix later.

In your later pass, if you're able to submit a fix for the ASCIITo* problem to
the RLZ library at the same time, it would be great. Then we can remove all the
hacks.

[jungshik at Google, 2010-09-20 21:26:03]

http://codereview.chromium.org/3447008/diff/7002/35006
File base/string_split.h (right):

http://codereview.chromium.org/3447008/diff/7002/35006#newcode23
base/string_split.h:23: // Every substring is trimmed of any leading or trailing
white space.
What you did for SplitStringDontTrim should be done for SplitString as well
(DCHECK and comments, etc).

[tfarina, 2010-09-21 04:53:12]

http://codereview.chromium.org/3447008/diff/7002/35006
File base/string_split.h (right):

http://codereview.chromium.org/3447008/diff/7002/35006#newcode23
base/string_split.h:23: // Every substring is trimmed of any leading or trailing
white space.
On 2010/09/20 21:26:04, Jungshik Shin wrote:
> What you did for SplitStringDontTrim should be done for SplitString as well
> (DCHECK and comments, etc). 
> 

Done.

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

http://codereview.chromium.org/3447008/diff/7002/35009#newcode28
base/string_util.h:28: #include "base/string_split.h"
On 2010/09/20 18:07:04, brettw wrote:
> If you look down below for RLZ_WIN_LIB_RLZ_LIB_H_, you can see how I
temporarily
> worked around this for ASCIITo *. Maybe you can just add a declaration for the
> function you want in that block, or add a similar block for the file you want.
> This will keep RLZ compiling until we can fix it, and will also prevent people
> from depending on this include that you'll have to fix later.
> 
Added the ifdef around the file include. Thanks for spotting that.

> In your later pass, if you're able to submit a fix for the ASCIITo* problem to
> the RLZ library at the same time, it would be great. Then we can remove all
the
> hacks.

Submitted a fix for that. See http://codereview.appspot.com/2200045/.

[tfarina, 2010-09-21 19:03:36]

On 2010/09/20 18:07:03, brettw wrote:
> In your later pass, if you're able to submit a fix for the ASCIITo* problem to
> the RLZ library at the same time, it would be great.

Brett, the include fix for rlz library was committed at
http://code.google.com/p/rlz/source/detail?r=23. Erik Wright said he is going to
roll the deps for rlz on chrome today.

[brettw, 2010-09-21 20:46:40]

LGTM

[brettw, 2010-09-21 20:48:19]

BTW if you're looking for something to do next, it would be nice if string_split
was in the base:: namespace. If you're going to do this, it might be easier to
do it before fixing RLZ (you can add a "using base::StringSplit;" in the magic
ifdef to keep it compiling).

[jungshik at Google, 2010-09-22 23:05:11]

http://codereview.chromium.org/3447008/diff/57001/58006
File base/string_split.h (right):

http://codereview.chromium.org/3447008/diff/57001/58006#newcode36
base/string_split.h:36: // UTF-8, and other single/multi-byte ASCII-compatible
encodings are OK.
Sorry I wrote the following, but forgot to submit. 
The above comment and DCHECK in the implementation is inconsistent (it's partly
my fault). 

Can you just say "|str| should be in UTF-8" in both places?  

With that, LGTM

[tfarina, 2010-09-23 01:00:50]

http://codereview.chromium.org/3447008/diff/57001/58006
File base/string_split.h (right):

http://codereview.chromium.org/3447008/diff/57001/58006#newcode36
base/string_split.h:36: // UTF-8, and other single/multi-byte ASCII-compatible
encodings are OK.
On 2010/09/22 23:05:11, Jungshik Shin wrote:
> Sorry I wrote the following, but forgot to submit. 
> The above comment and DCHECK in the implementation is inconsistent (it's
partly
> my fault). 
> 
> Can you just say "|str| should be in UTF-8" in both places?  
> 
> With that, LGTM 

Done.

[tfarina, 2010-09-23 05:28:25]

http://codereview.chromium.org/3447008/diff/34002/65005
File base/string_split.cc (right):

http://codereview.chromium.org/3447008/diff/34002/65005#newcode54
base/string_split.cc:54: DCHECK(IsStringUTF8(str));
Jungshik, this check is raising in ExtractCurrentFile from zip.cc, see bt below:

[11615:11615:0922/220345:4964042953:FATAL:base/string_split.cc(54)] Check
failed: IsStringUTF8(str). 
Backtrace:
	StackTrace::StackTrace() [0x985f546]
	logging::LogMessage::~LogMessage() [0x9879e33]
	SplitString() [0x98a1763]
	ExtractCurrentFile() [0x928b596]
	Unzip() [0x928ba1c]
	(anonymous namespace)::ZipTest_UnzipEvil2_Test::TestBody() [0x88f544c]
	testing::Test::Run() [0x94c9bc0]
	testing::internal::TestInfoImpl::Run() [0x94cd689]
	testing::TestCase::Run() [0x94cd7cb]
	testing::internal::UnitTestImpl::RunAllTests() [0x94ce369]
	testing::UnitTest::Run() [0x94ce4f6]
	base::TestSuite::Run() [0xab4df6b]
	main [0x93cb9ae]
	0x40cd4450
	0x80481a1

http://build.chromium.org/buildbot/try-server/builders/linux/builds/47430/ste...

[jungshik at Google, 2010-09-23 19:37:43]

On 2010/09/23 05:28:25, tfarina wrote:
> http://codereview.chromium.org/3447008/diff/34002/65005
> File base/string_split.cc (right):
> 
> http://codereview.chromium.org/3447008/diff/34002/65005#newcode54
> base/string_split.cc:54: DCHECK(IsStringUTF8(str));
> Jungshik, this check is raising in ExtractCurrentFile from zip.cc, see bt
below:

Hmm... UnzipEvil2 tests 'evil_via_invalid_utf8.zip'.   

Perhaps, you just have to restore the old comment to SplitString and a
warning. And, you have to remove DCHECK in SplitString. You can keep
SplitStringDontTrim as is in your latest patch. 




> 
> [11615:11615:0922/220345:4964042953:FATAL:base/string_split.cc(54)] Check
> failed: IsStringUTF8(str). 
> Backtrace:
> 	StackTrace::StackTrace() [0x985f546]
> 	logging::LogMessage::~LogMessage() [0x9879e33]
> 	SplitString() [0x98a1763]
> 	ExtractCurrentFile() [0x928b596]
> 	Unzip() [0x928ba1c]
> 	(anonymous namespace)::ZipTest_UnzipEvil2_Test::TestBody() [0x88f544c]
> 	testing::Test::Run() [0x94c9bc0]
> 	testing::internal::TestInfoImpl::Run() [0x94cd689]
> 	testing::TestCase::Run() [0x94cd7cb]
> 	testing::internal::UnitTestImpl::RunAllTests() [0x94ce369]
> 	testing::UnitTest::Run() [0x94ce4f6]
> 	base::TestSuite::Run() [0xab4df6b]
> 	main [0x93cb9ae]
> 	0x40cd4450
> 	0x80481a1
> 
>
http://build.chromium.org/buildbot/try-server/builders/linux/builds/47430/ste...