Move two generic string split functions from sync API to their own API in base/string_split.

Move two generic string split functions from sync API to their own API in base/string_split.

BUG=None
TEST=base_unittests

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

[tfarina (gmail-do not use), 2009-12-09 00:30:35]

Hi Tim, could you review this to me?

[brettw, 2009-12-09 04:15:03]

Can you mention why this change is necessary?

http://codereview.chromium.org/464075/diff/2001/3001
File base/string_util.h (right):

http://codereview.chromium.org/464075/diff/2001/3001#newcode547
base/string_util.h:547: // TODO(timsteele): Write unittests for these two
functions.
It seems like now is the only time unit tests for these functions will ever get
written.

[tim (not reviewing), 2009-12-09 05:44:35]

On 2009/12/09 04:15:03, brettw wrote:
> Can you mention why this change is necessary?

We need these functions to parse an HTTP response containing newline delimited,
"name=value" pairs.  The function is a direct copy from google3/strings and
handles edge and error cases.  I haven't found anything else in string_util or
elsewhere that would work; ParsedCookie seems nearly appropriate but it doesn't
quite match up (and, well, these aren't cookies).  Do we already have something
to handle this?

I figured such a generic string utility belonged here, hence my TODO.  Does that
seem OK to you?  I can provide the tests from the original implementation for
thiago to place in string_util_unittest if needed.


> 
> http://codereview.chromium.org/464075/diff/2001/3001
> File base/string_util.h (right):
> 
> http://codereview.chromium.org/464075/diff/2001/3001#newcode547
> base/string_util.h:547: // TODO(timsteele): Write unittests for these two
> functions.
> It seems like now is the only time unit tests for these functions will ever
get
> written.

[brettw, 2009-12-09 23:26:29]

On 2009/12/09 05:44:35, timsteele wrote:
> On 2009/12/09 04:15:03, brettw wrote:
> > Can you mention why this change is necessary?
> 
> We need these functions to parse an HTTP response containing newline
delimited,
> "name=value" pairs.  The function is a direct copy from google3/strings and
> handles edge and error cases.  I haven't found anything else in string_util or
> elsewhere that would work; ParsedCookie seems nearly appropriate but it
doesn't
> quite match up (and, well, these aren't cookies).  Do we already have
something
> to handle this?

No. I just want to be sure we really need everything that gets thrown into
string_util since its used so commonly and I think its already too large. I
think I would prefer this to be in a separate file split.h which matches the
google3 version and add a TODO that the other splitting functions be moved there
as well. What do you think?


> I figured such a generic string utility belonged here, hence my TODO.  Does
that
> seem OK to you?  I can provide the tests from the original implementation for
> thiago to place in string_util_unittest if needed.

Yes, I think that would be great to get your unit tests in here.

[tim (not reviewing), 2009-12-10 01:48:03]

On 2009/12/09 23:26:29, brettw wrote:
> On 2009/12/09 05:44:35, timsteele wrote:
> > On 2009/12/09 04:15:03, brettw wrote:
> > > Can you mention why this change is necessary?
> > 
> > We need these functions to parse an HTTP response containing newline
> delimited,
> > "name=value" pairs.  The function is a direct copy from google3/strings and
> > handles edge and error cases.  I haven't found anything else in string_util
or
> > elsewhere that would work; ParsedCookie seems nearly appropriate but it
> doesn't
> > quite match up (and, well, these aren't cookies).  Do we already have
> something
> > to handle this?
> 
> No. I just want to be sure we really need everything that gets thrown into
> string_util since its used so commonly and I think its already too large. I
> think I would prefer this to be in a separate file split.h which matches the
> google3 version and add a TODO that the other splitting functions be moved
there
> as well. What do you think?

That sounds good to me, though let's go with string_split.h to classify it a bit
better (google3 has the 'strings' folder).
> 
> 
> > I figured such a generic string utility belonged here, hence my TODO.  Does
> that
> > seem OK to you?  I can provide the tests from the original implementation
for
> > thiago to place in string_util_unittest if needed.
> 
> Yes, I think that would be great to get your unit tests in here.

Sending an email containing these to Thiago offline.

[tim (not reviewing), 2009-12-10 01:48:53]

Oh, and Thiago please put the function (in its new string_split.h home) in the
base namespace.  Thanks.

[tfarina (gmail-do not use), 2009-12-10 14:41:14]

On 2009/12/10 01:48:53, timsteele wrote:
> Oh, and Thiago please put the function (in its new string_split.h home) in the
> base namespace.  Thanks.

Hi Tim, I added these split function into their on files (string_split.cc/.h),
also I added the unit tests at string_split_unittest.cc.

About the tests, please review it more carefully, because I had to do many
modifications and disabled one of them to make all passed through.

[tfarina (gmail-do not use), 2009-12-11 00:34:59]

On 2009/12/10 14:41:14, tfarina wrote:
> About the tests, please review it more carefully, because I had to do many
> modifications and disabled one of them to make all passed through.

Oh Tim, I think I was a bit rude here. I was just trying to make sure that my
changes in the unittests were correct.

[tfarina (gmail-do not use), 2009-12-11 16:45:19]

Tim I uploaded another patchset that added these new files to base.gypi.

Please have another look.

[tfarina (gmail-do not use), 2009-12-15 01:33:26]

Tim Ping?

[tim (not reviewing), 2009-12-18 18:21:33]

(sorry, was OOO + a day of catch up)
LGTM, will land today.

[tfarina (gmail-do not use), 2010-01-22 17:11:04]

Thanks Tim!