Cleanup SplitStringIntoKeyaluePairs implementation.

Cleanup SplitStringIntoKeyaluePairs implementation.

This function was originally a helper function for sync and wasn't cleaned up/revisited since its move to base/ .
Since there are now several users of this function, we should care about a cleaner implementation.

While at string_split.cc, moving local functions (which partially were missing 'static') to an anonymous namespace.

BUG=NONE

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

[pneubeck (no reviews), 2014-03-05 22:30:59]

please review, thanks!

[pneubeck (no reviews), 2014-03-06 02:06:21]

This is based on the other CL https://codereview.chromium.org/184233009/
Please take a look, thanks!

[Mark Mentovai, 2014-03-06 17:55:51]

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
File base/strings/string_split.cc (right):

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
base/strings/string_split.cc:94: // Don't add empty pairs into the result.
This is empty pairs after whitespace trimming. Desirable?

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
base/strings/string_split.cc:100: if (!SplitStringIntoKeyValue(pairs[i],
key_value_delimiter, &key, &value)) {
This whole thing would be better with StringPieces, but I’m not going to ask you
to rewrite it using StringPiece because it relies on a bunch of other pieces of
this file that don’t use StringPiece either. I’m just pointing it out because it
would be an easyish area for optimization by removing multiple string copies if
this function is found to be a hot spot.

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
base/strings/string_split.cc:102: // value or key; just record that our split
failed.
our split -> the split

See the test CL. I’m not sure that this behavior is useful.

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
base/strings/string_split.cc:105: DCHECK_LE(value.size(), 1U);
?

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_split.h
File base/strings/string_split.h (left):

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
base/strings/string_split.h:34: BASE_EXPORT bool SplitStringIntoKeyValues(const
std::string& line,
The CL description should say that this is going away.

[pneubeck (no reviews), 2014-03-07 17:12:10]

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
File base/strings/string_split.cc (right):

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
base/strings/string_split.cc:102: // value or key; just record that our split
failed.
On 2014/03/06 17:55:51, Mark Mentovai wrote:
> our split -> the split
> 
> See the test CL. I’m not sure that this behavior is useful.

Done.

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
base/strings/string_split.cc:105: DCHECK_LE(value.size(), 1U);
On 2014/03/06 17:55:51, Mark Mentovai wrote:
> ?

Ouch. yes that has to be removed.
Done.

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_split.h
File base/strings/string_split.h (left):

https://codereview.chromium.org/184233010/diff/60001/base/strings/string_spli...
base/strings/string_split.h:34: BASE_EXPORT bool SplitStringIntoKeyValues(const
std::string& line,
On 2014/03/06 17:55:51, Mark Mentovai wrote:
> The CL description should say that this is going away.

Done.

[Mark Mentovai, 2014-03-07 17:15:43]

LGTM

[pneubeck (no reviews), 2014-03-07 17:16:06]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-03-07 17:17:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/184233010/120001

[commit-bot: I haz the power, 2014-03-07 17:22:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 17:22:02]

Try jobs failed on following builders: mac_chromium_compile_dbg,
mac_chromium_rel

[pneubeck (no reviews), 2014-03-07 17:32:22]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-03-07 17:33:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/184233010/140001

[commit-bot: I haz the power, 2014-03-07 17:46:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 17:46:33]

Try jobs failed on following builders: mac_chromium_compile_dbg

[pneubeck (no reviews), 2014-03-11 08:49:50]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-03-11 08:51:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/184233010/160001

[commit-bot: I haz the power, 2014-03-11 15:01:59]

Change committed as 256226