Update ToLower/UpperASCII API

Replace StringToLowerASCII with base::ToLowerASCII

Standardize on using string pieces and returning strings. Remove in-place version (this was only used in a couple places and they were not performance-critical).

De-templatize the character versions of ToUpperASCII/ToLowerASCII. This would lead to bizarre errors if you pass other things (like a string). This is so little code, it's now just duplicated.

I renamed StringToLowerASCII to just be ToLowerASCII so you can pass whatever you want to ToLowerASCII and it does the right thing. This seems simpler to me.

This replaces all calls of StringToUpperASCII to the new form. The lowercase version is more common and will be done in a separate pass.

Committed: https://crrev.com/c15100ce1266e52a77f2974ffe9dfb833f5e58b3
Cr-Commit-Position: refs/heads/master@{#342219}

[brettw, 2015-08-05 22:35:04]

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

[brettw, 2015-08-05 22:37:36]

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

[commit-bot: I haz the power, 2015-08-05 22:38:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280473002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280473002/20001

[commit-bot: I haz the power, 2015-08-05 22:59:25]

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

[commit-bot: I haz the power, 2015-08-05 22:59:26]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[yzshen1, 2015-08-05 23:21:04]

https://codereview.chromium.org/1280473002/diff/20001/base/strings/string_uti...
File base/strings/string_util_unittest.cc (right):

https://codereview.chromium.org/1280473002/diff/20001/base/strings/string_uti...
base/strings/string_util_unittest.cc:511: EXPECT_EQ(L'c', ToLowerASCII(L'C'));
The compiler complains about ambiguity. It doesn't know which one to call:

../../base/strings/string_util.h:96:13: note: candidate function
inline char ToLowerASCII(char c) {
            ^
../../base/strings/string_util.h:99:15: note: candidate function
inline char16 ToLowerASCII(char16 c) {

That might be the reason why we use template before. What do you think?

[brettw, 2015-08-06 17:13:53]

https://codereview.chromium.org/1280473002/diff/20001/base/strings/string_uti...
File base/strings/string_util_unittest.cc (right):

https://codereview.chromium.org/1280473002/diff/20001/base/strings/string_uti...
base/strings/string_util_unittest.cc:511: EXPECT_EQ(L'c', ToLowerASCII(L'C'));
Actually this test just needed updating. I decided not to write a wchar_t
version of this function, and that's what was being called here (I developed on
Windows where wchar_t == char16). I updated the test to pass char16.

There might be a few other places that need updating, but I think we should
always be using char16 so I don't see this as a problem.

[yzshen1, 2015-08-06 18:07:19]

LGTM with two nits. Thanks!

https://codereview.chromium.org/1280473002/diff/40001/base/strings/string_uti...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/1280473002/diff/40001/base/strings/string_uti...
base/strings/string_util.cc:152: StringType
ToLowerASCIIT(BasicStringPiece<StringType> str) {
Please consider putting this and ToUpperASCIIT into the unnamed namespace.
Besides, the 'T' at the end seems to blend into 'ASCII'. Maybe  we could
consider naming them To{Lower|Upper}ASCIIImpl.

https://codereview.chromium.org/1280473002/diff/40001/base/strings/string_util.h
File base/strings/string_util.h (right):

https://codereview.chromium.org/1280473002/diff/40001/base/strings/string_uti...
base/strings/string_util.h:116: // Converts the given string to it's
ASCII-lowercase equivalent.
nit: ASCII-uppercase

[brettw, 2015-08-06 18:23:04]

https://codereview.chromium.org/1280473002/diff/40001/base/strings/string_uti...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/1280473002/diff/40001/base/strings/string_uti...
base/strings/string_util.cc:152: StringType
ToLowerASCIIT(BasicStringPiece<StringType> str) {
Done, used "Impl"

https://codereview.chromium.org/1280473002/diff/40001/base/strings/string_util.h
File base/strings/string_util.h (right):

https://codereview.chromium.org/1280473002/diff/40001/base/strings/string_uti...
base/strings/string_util.h:116: // Converts the given string to it's
ASCII-lowercase equivalent.
On 2015/08/06 18:07:19, yzshen1 wrote:
> nit: ASCII-uppercase

Done.

[brettw, 2015-08-06 18:23:07]

The CQ bit was checked by brettw@chromium.org

[brettw, 2015-08-06 18:23:08]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1280473002/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2015-08-06 18:23:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280473002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280473002/60001

[commit-bot: I haz the power, 2015-08-06 18:43:19]

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

[commit-bot: I haz the power, 2015-08-06 18:43:21]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[brettw, 2015-08-06 19:03:40]

The CQ bit was checked by brettw@chromium.org

[brettw, 2015-08-06 19:03:40]

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

[commit-bot: I haz the power, 2015-08-06 19:04:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280473002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280473002/80001

[commit-bot: I haz the power, 2015-08-06 19:29:19]

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

[commit-bot: I haz the power, 2015-08-06 19:29:20]

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_...)

[brettw, 2015-08-06 19:34:17]

The CQ bit was checked by brettw@chromium.org

[brettw, 2015-08-06 19:34:17]

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

[commit-bot: I haz the power, 2015-08-06 19:34:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280473002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280473002/100001

[commit-bot: I haz the power, 2015-08-06 20:21:11]

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

[commit-bot: I haz the power, 2015-08-06 20:21:12]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[brettw, 2015-08-06 20:24:54]

The CQ bit was checked by brettw@chromium.org

[brettw, 2015-08-06 20:24:54]

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

[commit-bot: I haz the power, 2015-08-06 20:26:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280473002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280473002/120001

[commit-bot: I haz the power, 2015-08-06 22:54:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-06 22:55:29]

Patchset 7 (id:??) landed as
https://crrev.com/c15100ce1266e52a77f2974ffe9dfb833f5e58b3
Cr-Commit-Position: refs/heads/master@{#342219}