Fix base::GetNativeLibraryName() for Mac.

Fix base::GetNativeLibraryName() for Mac.

- Also change GetNativeLibraryName() to take a StringPiece and return a
  std::string, instead of StringPiece16/string16.
- Use that in a few places to get rid of platform-specific ifdefs.

Committed: https://crrev.com/02c965bc53c59075e8f2453c6146e75782dd0c44
Cr-Commit-Position: refs/heads/master@{#399759}

[Lei Zhang, 2016-06-07 03:06:59]

thestig@chromium.org changed reviewers:
+ thomasvl@chromium.org, xhwang@chromium.org

[Lei Zhang, 2016-06-07 03:06:59]

Follow up to https://codereview.chromium.org/2042103002/

[Lei Zhang, 2016-06-07 03:19:15]

Description was changed from

==========
Fix base::GetNativeLibraryName() for Mac.

- Also add a std::string version of GetNativeLibraryName().
- Use that in a few places to get rid of platform-specific ifdefs.
==========

to

==========
Fix base::GetNativeLibraryName() for Mac.

123456789012345678901234567890123456789012345678901234567890123456789012
- Also change GetNativeLibraryName() to take a StringPiece and return a
  std::string, instead of StringPiece16/string16.
- Use that in a few places to get rid of platform-specific ifdefs.
==========

[Lei Zhang, 2016-06-07 03:19:23]

Description was changed from

==========
Fix base::GetNativeLibraryName() for Mac.

123456789012345678901234567890123456789012345678901234567890123456789012
- Also change GetNativeLibraryName() to take a StringPiece and return a
  std::string, instead of StringPiece16/string16.
- Use that in a few places to get rid of platform-specific ifdefs.
==========

to

==========
Fix base::GetNativeLibraryName() for Mac.

- Also change GetNativeLibraryName() to take a StringPiece and return a
  std::string, instead of StringPiece16/string16.
- Use that in a few places to get rid of platform-specific ifdefs.
==========

[xhwang, 2016-06-07 16:24:41]

https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h#n...
base/native_library.h:94: // "mylib.dylib" on Mac.
This comment needs to be updated.

https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h#n...
base/native_library.h:95: BASE_EXPORT std::string
GetNativeLibraryName(StringPiece name);
Don't we need to support UTF16 paths? Or since this is internal we choose not to
support it?

[TVL, 2016-06-08 00:07:58]

thomasvl@chromium.org changed reviewers:
+ mark@chromium.org
- thomasvl@chromium.org

[Lei Zhang, 2016-06-10 22:09:29]

https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h#n...
base/native_library.h:94: // "mylib.dylib" on Mac.
On 2016/06/07 16:24:41, xhwang wrote:
> This comment needs to be updated.

Done.

https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h#n...
base/native_library.h:95: BASE_EXPORT std::string
GetNativeLibraryName(StringPiece name);
On 2016/06/07 16:24:41, xhwang wrote:
> Don't we need to support UTF16 paths? Or since this is internal we choose not
to
> support it?

The callers can convert to UTF-16 if they need to. Currently it's a bit more
work because we always do conversions in GetNativeLibraryName() to make it
UTF-16, and then some callers have to convert it back.

[xhwang, 2016-06-10 23:27:27]

https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h
File base/native_library.h (right):

https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h#n...
base/native_library.h:95: BASE_EXPORT std::string
GetNativeLibraryName(StringPiece name);
On 2016/06/10 22:09:29, Lei Zhang wrote:
> On 2016/06/07 16:24:41, xhwang wrote:
> > Don't we need to support UTF16 paths? Or since this is internal we choose
not
> to
> > support it?
> 
> The callers can convert to UTF-16 if they need to. Currently it's a bit more
> work because we always do conversions in GetNativeLibraryName() to make it
> UTF-16, and then some callers have to convert it back.

But the return value is std::string now, if the original path is UTF-16, how
would this work?

[Lei Zhang, 2016-06-10 23:34:54]

On 2016/06/10 23:27:27, xhwang wrote:
> https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h
> File base/native_library.h (right):
> 
>
https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h#n...
> base/native_library.h:95: BASE_EXPORT std::string
> GetNativeLibraryName(StringPiece name);
> On 2016/06/10 22:09:29, Lei Zhang wrote:
> > On 2016/06/07 16:24:41, xhwang wrote:
> > > Don't we need to support UTF16 paths? Or since this is internal we choose
> not
> > to
> > > support it?
> > 
> > The callers can convert to UTF-16 if they need to. Currently it's a bit more
> > work because we always do conversions in GetNativeLibraryName() to make it
> > UTF-16, and then some callers have to convert it back.
> 
> But the return value is std::string now, if the original path is UTF-16, how
> would this work?

All the callers pass in ASCII right now, but if there is UTF-16, the caller does
UTF16ToUTF8(), and UTF8ToUTF16() with the result.

[Mark Mentovai, 2016-06-10 23:40:00]

https://codereview.chromium.org/2048523002/diff/60001/chrome/browser/load_lib...
File chrome/browser/load_library_perf_test.cc (right):

https://codereview.chromium.org/2048523002/diff/60001/chrome/browser/load_lib...
chrome/browser/load_library_perf_test.cc:65: // File name of the ClearKey CDM on
different platforms.
Not a filename. “Different platforms” is extraneous now.

https://codereview.chromium.org/2048523002/diff/60001/chrome/browser/load_lib...
chrome/browser/load_library_perf_test.cc:67: const char kClearKeyCdmName[] =
"clearkeycdm";
Maybe kClearKeyCdmLibraryName. But I see no reason for this to be a const char[]
out here. Just write it in-line in LoadCDMPerfTest.ExternalClearKey, either as a
const down there, or just a literal as the function call argument.

https://codereview.chromium.org/2048523002/diff/60001/media/cdm/external_clea...
File media/cdm/external_clear_key_test_helper.cc (right):

https://codereview.chromium.org/2048523002/diff/60001/media/cdm/external_clea...
media/cdm/external_clear_key_test_helper.cc:26: // File name of the External
ClearKey CDM on different platforms.
Here, too: not a file name. Not on different platforms.

https://codereview.chromium.org/2048523002/diff/60001/media/cdm/external_clea...
media/cdm/external_clear_key_test_helper.cc:27: const char
kExternalClearKeyCdmName[] = "clearkeycdm";
kExternalClearKeyCdmLibararyName, but here too: no reason for this to be a
constant out here, move it to the LoadLibrary method. Unless you’re fixing
xhwang’s TODO in load_library_perf_test.cc.

https://codereview.chromium.org/2048523002/diff/60001/third_party/widevine/cd...
File third_party/widevine/cdm/widevine_cdm_common.h (right):

https://codereview.chromium.org/2048523002/diff/60001/third_party/widevine/cd...
third_party/widevine/cdm/widevine_cdm_common.h:36: // File name of the CDM on
different platforms.
Not a filename. Not on different platforms.

https://codereview.chromium.org/2048523002/diff/60001/third_party/widevine/cd...
third_party/widevine/cdm/widevine_cdm_common.h:37: const char kWidevineCdmName[]
= "widevinecdm";
The definitions are right here in the .h?

https://codereview.chromium.org/2048523002/diff/60001/third_party/widevine/cd...
third_party/widevine/cdm/widevine_cdm_common.h:37: const char kWidevineCdmName[]
= "widevinecdm";
Maybe kWidevineCdmLibraryName is better.

[xhwang, 2016-06-11 00:09:17]

On 2016/06/10 23:34:54, Lei Zhang wrote:
> On 2016/06/10 23:27:27, xhwang wrote:
> > https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h
> > File base/native_library.h (right):
> > 
> >
>
https://codereview.chromium.org/2048523002/diff/20001/base/native_library.h#n...
> > base/native_library.h:95: BASE_EXPORT std::string
> > GetNativeLibraryName(StringPiece name);
> > On 2016/06/10 22:09:29, Lei Zhang wrote:
> > > On 2016/06/07 16:24:41, xhwang wrote:
> > > > Don't we need to support UTF16 paths? Or since this is internal we
choose
> > not
> > > to
> > > > support it?
> > > 
> > > The callers can convert to UTF-16 if they need to. Currently it's a bit
more
> > > work because we always do conversions in GetNativeLibraryName() to make it
> > > UTF-16, and then some callers have to convert it back.
> > 
> > But the return value is std::string now, if the original path is UTF-16, how
> > would this work?
> 
> All the callers pass in ASCII right now, but if there is UTF-16, the caller
does
> UTF16ToUTF8(), and UTF8ToUTF16() with the result.

UTF16ToUTF8() returns a bool and it can fail, so this won't always work.

As I commented above, if we decide to do this because no caller is using UTF-16
now and we don't expect any in the future, I am fine with this change. Just
checking :)

[Lei Zhang, 2016-06-13 19:10:07]

On 2016/06/11 00:09:17, xhwang wrote:
> UTF16ToUTF8() returns a bool and it can fail, so this won't always work.
> 
> As I commented above, if we decide to do this because no caller is using
UTF-16
> now and we don't expect any in the future, I am fine with this change. Just
> checking :)

I was thinking of the UTF16ToUTF8() version that returns a std::string, but
looking at the impl, it only does best effort conversion. Maybe
GetNativeLibraryName() should DCHECK that the StringPiece passed in is ASCII?

https://codereview.chromium.org/2048523002/diff/60001/chrome/browser/load_lib...
File chrome/browser/load_library_perf_test.cc (right):

https://codereview.chromium.org/2048523002/diff/60001/chrome/browser/load_lib...
chrome/browser/load_library_perf_test.cc:65: // File name of the ClearKey CDM on
different platforms.
On 2016/06/10 23:39:59, Mark Mentovai wrote:
> Not a filename. “Different platforms” is extraneous now.

Done.

https://codereview.chromium.org/2048523002/diff/60001/chrome/browser/load_lib...
chrome/browser/load_library_perf_test.cc:67: const char kClearKeyCdmName[] =
"clearkeycdm";
On 2016/06/10 23:39:59, Mark Mentovai wrote:
> Maybe kClearKeyCdmLibraryName. But I see no reason for this to be a const
char[]
> out here. Just write it in-line in LoadCDMPerfTest.ExternalClearKey, either as
a
> const down there, or just a literal as the function call argument.

I renamed the variable, and merged it with the copy in media/

https://codereview.chromium.org/2048523002/diff/60001/third_party/widevine/cd...
File third_party/widevine/cdm/widevine_cdm_common.h (right):

https://codereview.chromium.org/2048523002/diff/60001/third_party/widevine/cd...
third_party/widevine/cdm/widevine_cdm_common.h:36: // File name of the CDM on
different platforms.
On 2016/06/10 23:39:59, Mark Mentovai wrote:
> Not a filename. Not on different platforms.

Done.

https://codereview.chromium.org/2048523002/diff/60001/third_party/widevine/cd...
third_party/widevine/cdm/widevine_cdm_common.h:37: const char kWidevineCdmName[]
= "widevinecdm";
On 2016/06/10 23:39:59, Mark Mentovai wrote:
> Maybe kWidevineCdmLibraryName is better.

Done.

https://codereview.chromium.org/2048523002/diff/60001/third_party/widevine/cd...
third_party/widevine/cdm/widevine_cdm_common.h:37: const char kWidevineCdmName[]
= "widevinecdm";
On 2016/06/10 23:39:59, Mark Mentovai wrote:
> The definitions are right here in the .h?

There's no .cc file. *innocent look*

[xhwang, 2016-06-13 19:16:47]

Thanks for fixing all the CDM names!

Yeah, a DCHECK and some comment about the expectation would be nice.

Otherwise lgtm

[Mark Mentovai, 2016-06-13 19:29:03]

LGTM

https://codereview.chromium.org/2048523002/diff/80001/base/scoped_native_libr...
File base/scoped_native_library_unittest.cc (right):

https://codereview.chromium.org/2048523002/diff/80001/base/scoped_native_libr...
base/scoped_native_library_unittest.cc:31: FilePath
path(base::ASCIIToUTF16(GetNativeLibraryName("ddraw")));
FilePath::FromUTF16Unsafe()?

https://codereview.chromium.org/2048523002/diff/80001/net/udp/udp_socket_posi...
File net/udp/udp_socket_posix.cc (right):

https://codereview.chromium.org/2048523002/diff/80001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:355: base::FilePath
file(base::FilePath::FromUTF8Unsafe(
Is FromUTF8Unsafe() even needed in this Android-only block?

[Lei Zhang, 2016-06-13 21:30:34]

On 2016/06/13 19:16:47, xhwang wrote:
> Yeah, a DCHECK and some comment about the expectation would be nice.

Done.

https://codereview.chromium.org/2048523002/diff/80001/base/scoped_native_libr...
File base/scoped_native_library_unittest.cc (right):

https://codereview.chromium.org/2048523002/diff/80001/base/scoped_native_libr...
base/scoped_native_library_unittest.cc:31: FilePath
path(base::ASCIIToUTF16(GetNativeLibraryName("ddraw")));
On 2016/06/13 19:29:03, Mark Mentovai wrote:
> FilePath::FromUTF16Unsafe()?

I might as well skip the ASCIIToUTF16() bit and use FromUTF8Unsafe().

https://codereview.chromium.org/2048523002/diff/80001/net/udp/udp_socket_posi...
File net/udp/udp_socket_posix.cc (right):

https://codereview.chromium.org/2048523002/diff/80001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:355: base::FilePath
file(base::FilePath::FromUTF8Unsafe(
On 2016/06/13 19:29:03, Mark Mentovai wrote:
> Is FromUTF8Unsafe() even needed in this Android-only block?

Nope.

[Lei Zhang, 2016-06-13 22:09:27]

thestig@chromium.org changed reviewers:
+ pauljensen@chromium.org

[Lei Zhang, 2016-06-13 22:09:47]

+pauljensen for net/

[pauljensen, 2016-06-14 11:11:46]

net/ lgtm

[Lei Zhang, 2016-06-14 17:20:33]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-06-14 17:20:33]

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

[commit-bot: I haz the power, 2016-06-14 17:20:53]

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

[commit-bot: I haz the power, 2016-06-14 18:52:39]

Description was changed from

==========
Fix base::GetNativeLibraryName() for Mac.

- Also change GetNativeLibraryName() to take a StringPiece and return a
  std::string, instead of StringPiece16/string16.
- Use that in a few places to get rid of platform-specific ifdefs.
==========

to

==========
Fix base::GetNativeLibraryName() for Mac.

- Also change GetNativeLibraryName() to take a StringPiece and return a
  std::string, instead of StringPiece16/string16.
- Use that in a few places to get rid of platform-specific ifdefs.
==========

[commit-bot: I haz the power, 2016-06-14 18:52:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-14 18:55:24]

Description was changed from

==========
Fix base::GetNativeLibraryName() for Mac.

- Also change GetNativeLibraryName() to take a StringPiece and return a
  std::string, instead of StringPiece16/string16.
- Use that in a few places to get rid of platform-specific ifdefs.
==========

to

==========
Fix base::GetNativeLibraryName() for Mac.

- Also change GetNativeLibraryName() to take a StringPiece and return a
  std::string, instead of StringPiece16/string16.
- Use that in a few places to get rid of platform-specific ifdefs.

Committed: https://crrev.com/02c965bc53c59075e8f2453c6146e75782dd0c44
Cr-Commit-Position: refs/heads/master@{#399759}
==========

[commit-bot: I haz the power, 2016-06-14 18:55:26]

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