New new versions of Starts/EndsWith and SplitString in net

New new versions of Starts/EndsWith and SplitString in net.

The new calls are longer but more explicit and also more general. I didn't want change the semantics. The old SplitString maps to TRIM_WHITESPACE, SPLIT_WANT_ALL so I generally kept that even if it's not clear if the calling code wants that particular behavior.

In places that obviously didn't need copies, use the StringPiece variant to avoid copies. I didn't expend too much effort in this area, especially in unit test code. It's like that more copies could be deleted with more effort.

Many places just used SplitString to fill a vector that is iterated over. In these places I updated it to use a range-based for loop over the result of the function call.

Add StringPiece versions of TrimWhitesace to match the existing Trim functions.

Update IPLiteralToNumber to take a string piece. This is commonly used in places that have string pieces so this saves a copy.

In net/dns/dns_config_service_win.cc I replaced a UTF16TOASCII call with assign(). This is what UTF16ToASCII actually does and avoids a copy because the current UTF16ToASCII doesn't support StringPiece. Updating this function call or adding an override is a big project due to the way it's use with Blink's WebString.

Removed parts of StringPieceUtils that duplicate base code. Made the remaining functions in that file not locale dependent (tolower is basically always wrong for non-ASCII).

BUG=506920, 506255
Committed: https://crrev.com/3a2c6907279aca02c1ecfa971237068338ba49d8
Cr-Commit-Position: refs/heads/master@{#337445}

[brettw, 2015-07-04 03:28:10]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-04 03:28:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215933004/40001

[commit-bot: I haz the power, 2015-07-04 03:41:48]

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

[commit-bot: I haz the power, 2015-07-04 03:41:49]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

(exceeded global retry quota)

[brettw, 2015-07-04 05:06:40]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-04 05:06:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215933004/80001

[commit-bot: I haz the power, 2015-07-04 05:22:57]

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

[commit-bot: I haz the power, 2015-07-04 05:22:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

(exceeded global retry quota)

[brettw, 2015-07-05 06:14:51]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-05 06:14:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215933004/100001

[commit-bot: I haz the power, 2015-07-05 06:55:01]

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

[commit-bot: I haz the power, 2015-07-05 06:55:02]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[brettw, 2015-07-05 15:58:49]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-05 15:59:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215933004/120001

[commit-bot: I haz the power, 2015-07-05 17:21:41]

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

[commit-bot: I haz the power, 2015-07-05 17:21:42]

Dry run: This issue passed the CQ dry run.

[brettw, 2015-07-05 17:48:20]

brettw@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-07-06 08:52:02]

LGTM

I've left several nits in the form of either pedantry or consistency (with the
broader CL). I know how annoying nits can be on these cleanups, but if it's
trivial to fix the consistency ones, that'd be rad. The pedantry are up to your
judgement.

https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_address_nu...
File net/base/ip_address_number.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_address_nu...
net/base/ip_address_number.cc:165: std::string host_brackets = "[";
EXTREME PEDANTRY NIT: Slight performance pedantry would be

std::string host_brackets('[', 1);

(Although I think most optimizing compilers could look into this due to the
constant, this just avoids the pessimistic strlen & strcopy, especially when
using small string optimization)

https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_pattern.cc
File net/base/ip_pattern.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_pattern.cc...
net/base/ip_pattern.cc:103: for (base::StringPiece component : components) {
EXTREME PEDANTRY:

Wouldn't
for (const auto& component : components) {
or
for (const base::StringPiece& component : components) {

make more sense here? So that it can bind to the const form in compilers that
implemented the C++14 optimization in C++11 mode?

[The debate about whether tis better to copy a base::StringPiece vs refer to it
by reference seems unclear; I've seen arguments about performance go both ways,
and Google allows both, but Chromium tends to prefer the by-ref rather than
treating StringPiece as POD]

[You also tend to prefer the latter elsewhere in this CL]

https://codereview.chromium.org/1215933004/diff/120001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/base/mime_util.cc#...
net/base/mime_util.cc:473: #endif
OBSERVATION: Functionally, this behaviour if equivalent, but it's at least worth
calling out here that the previous code had the effect of calling an explicit
ctor for FilePath::StringType for these, and here, you're calling an implicit
ctor and hoping it works out.

https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_mon...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_mon...
net/cookies/cookie_monster_unittest.cc:441: base::SPLIT_WANT_ALL)) {
nit: While you're here, this (and 467) are safe to clean up. I'm OK if you
don't, but worth calling it out since I'm sure I'll forget once this CL lands :)

https://codereview.chromium.org/1215933004/diff/120001/net/dns/dns_config_ser...
File net/dns/dns_config_service_win.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/dns/dns_config_ser...
net/dns/dns_config_service_win.cc:480: value, L",", base::TRIM_WHITESPACE,
base::SPLIT_WANT_ALL)) {
CONSISTENCY PEDANTRY NIT: Elsewhere, you use base::ASCIIToUTF16(",") [and
friends]

Totally understand that because this is Windows, L"," will 'do the right thing',
but it was enough to make me look at this a little odd initially (until I
realized it was a _win file), so I thought I'd call it out, especially when
elsewhere in the CL it uses a different pattern.

Totally fine if you want ot leave it as is.

https://codereview.chromium.org/1215933004/diff/120001/net/proxy/proxy_bypass...
File net/proxy/proxy_bypass_rules.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/proxy/proxy_bypass...
net/proxy/proxy_bypass_rules.cc:337: !base::StartsWith(raw, "*",
base::CompareCase::SENSITIVE))
CONSISTENCY NIT: Why'd you change this to SENSITIVE when it used the
INSENSITIVE_ASCII

[I'm guessing just as a performance optimization, since there is no 'lower case'
*, but in that case, shouldn't 331 have also changed?]

https://codereview.chromium.org/1215933004/diff/120001/net/test/embedded_test...
File net/test/embedded_test_server/embedded_test_server.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/test/embedded_test...
net/test/embedded_test_server/embedded_test_server.cc:266:
DCHECK(base::StartsWith(relative_url, "/", base::CompareCase::SENSITIVE))
PEDANTRY NIT: This is another example of case sensitive vs not that *shrug*

[brettw, 2015-07-06 16:52:25]

The CQ bit was checked by brettw@chromium.org

[brettw, 2015-07-06 16:52:26]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1215933004/#ps140001
(title: " ")

[brettw, 2015-07-06 16:52:34]

https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_address_nu...
File net/base/ip_address_number.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_address_nu...
net/base/ip_address_number.cc:165: std::string host_brackets = "[";
I find the suggested form less clear, personally, and it won't matter in
relation to the string allocation.

https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_pattern.cc
File net/base/ip_pattern.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/base/ip_pattern.cc...
net/base/ip_pattern.cc:103: for (base::StringPiece component : components) {
This particular loop is weird because it actually modifies the loop variable
(!). This is why the previous iterator wasn't const.

My opinion is that const& is better for loops for StringPiece, but I often pass
by value to functions. This makes some intuitive sense: the compiler can't
normally optimize across function boundaries, and putting the two words on the
stack next to the rest of the parameters is faster than putting a pointer and
then dereferencing it twice to an area the function isn't touching otherwise. In
this case, this compiler has full visibility to optimize dereferences and the
vector variables are sure to be in L1 cache anyway.

https://codereview.chromium.org/1215933004/diff/120001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/base/mime_util.cc#...
net/base/mime_util.cc:473: #endif
On 2015/07/06 08:52:01, Ryan Sleevi (slow through 7-15 wrote:
> OBSERVATION: Functionally, this behaviour if equivalent, but it's at least
worth
> calling out here that the previous code had the effect of calling an explicit
> ctor for FilePath::StringType for these, and here, you're calling an implicit
> ctor and hoping it works out.

I'm not calling any implicit constructors, I'm supplying the exact type required
for the given platform. Granted, this does make assumptions about what type
FilePath::StringType is on Windows-vs-non-Windows but I don't have a problem
with that.

I am changing UTF8ToWide to UTF8ToUTF16. I want to delete all wstrings.

https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_mon...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_mon...
net/cookies/cookie_monster_unittest.cc:441: base::SPLIT_WANT_ALL)) {
On 2015/07/06 08:52:01, Ryan Sleevi (slow through 7-15 wrote:
> nit: While you're here, this (and 467) are safe to clean up. I'm OK if you
> don't, but worth calling it out since I'm sure I'll forget once this CL lands
:)

I have no idea what you're asking me to clean up here. I'm happy to do any
updates in a second patch.

https://codereview.chromium.org/1215933004/diff/120001/net/dns/dns_config_ser...
File net/dns/dns_config_service_win.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/dns/dns_config_ser...
net/dns/dns_config_service_win.cc:480: value, L",", base::TRIM_WHITESPACE,
base::SPLIT_WANT_ALL)) {
This is pretty common in Windows files, at least in the ones I see often. I'll
leave it.

https://codereview.chromium.org/1215933004/diff/120001/net/proxy/proxy_bypass...
File net/proxy/proxy_bypass_rules.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/proxy/proxy_bypass...
net/proxy/proxy_bypass_rules.cc:337: !base::StartsWith(raw, "*",
base::CompareCase::SENSITIVE))
On 2015/07/06 08:52:02, Ryan Sleevi (slow through 7-15 wrote:
> CONSISTENCY NIT: Why'd you change this to SENSITIVE when it used the
> INSENSITIVE_ASCII
> 
> [I'm guessing just as a performance optimization, since there is no 'lower
case'
> *, but in that case, shouldn't 331 have also changed?]

Correct. I updated 331 also.

https://codereview.chromium.org/1215933004/diff/120001/net/test/embedded_test...
File net/test/embedded_test_server/embedded_test_server.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/test/embedded_test...
net/test/embedded_test_server/embedded_test_server.cc:266:
DCHECK(base::StartsWith(relative_url, "/", base::CompareCase::SENSITIVE))
On 2015/07/06 08:52:02, Ryan Sleevi (slow through 7-15 wrote:
> PEDANTRY NIT: This is another example of case sensitive vs not that *shrug*

Kept, I did this on purpose. One problem with the previous API was that people
tended to pass "false" since it feels like the default value. They're not
actually making statements about the semantics they want. I expect more than a
few of the ones taking actual text don't actually want/need case insensitivity
either, but I dare not change anything.

[commit-bot: I haz the power, 2015-07-06 16:53:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215933004/140001

[Ryan Sleevi, 2015-07-06 16:58:12]

https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_mon...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_mon...
net/cookies/cookie_monster_unittest.cc:441: base::SPLIT_WANT_ALL)) {
On 2015/07/06 16:52:34, brettw wrote:
> On 2015/07/06 08:52:01, Ryan Sleevi (slow through 7-15 wrote:
> > nit: While you're here, this (and 467) are safe to clean up. I'm OK if you
> > don't, but worth calling it out since I'm sure I'll forget once this CL
lands
> :)
> 
> I have no idea what you're asking me to clean up here. I'm happy to do any
> updates in a second patch.

Sorry, meant

const base::StringPiece& token : base::SplitStringPiece(...)

Instead of copying the string.

[brettw, 2015-07-06 17:02:37]

On 2015/07/06 16:58:12, Ryan Sleevi (slow through 7-15 wrote:
>
https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_mon...
> File net/cookies/cookie_monster_unittest.cc (right):
> 
>
https://codereview.chromium.org/1215933004/diff/120001/net/cookies/cookie_mon...
> net/cookies/cookie_monster_unittest.cc:441: base::SPLIT_WANT_ALL)) {
> On 2015/07/06 16:52:34, brettw wrote:
> > On 2015/07/06 08:52:01, Ryan Sleevi (slow through 7-15 wrote:
> > > nit: While you're here, this (and 467) are safe to clean up. I'm OK if you
> > > don't, but worth calling it out since I'm sure I'll forget once this CL
> lands
> > :)
> > 
> > I have no idea what you're asking me to clean up here. I'm happy to do any
> > updates in a second patch.
> 
> Sorry, meant
> 
> const base::StringPiece& token : base::SplitStringPiece(...)
> 
> Instead of copying the string.

Ah, It's a unit test, I'm wasn't sweating these cases.

[commit-bot: I haz the power, 2015-07-06 19:43:40]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-06 19:44:41]

Patchset 8 (id:??) landed as
https://crrev.com/3a2c6907279aca02c1ecfa971237068338ba49d8
Cr-Commit-Position: refs/heads/master@{#337445}