media: Use platform specific folders for CDMs

media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:
    <DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

For bundled CDM, the folder structure should be like:
    <DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64
Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we don't use platform specific folders.

- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp build). So we build to the default output dir first, then copy it over to the platform specific folder. See http://crbug.com/611990 for more details.

- Updated all tests to support platform specific folders.

- Update installer files to bundle the CDM (adapter) in the right folder.

TBR=gab@chromium.org
BUG=582622
TEST=All tests pass. Tested on Mac and Linux.

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

[xhwang, 2016-05-18 22:56:47]

Description was changed from

==========
media: Use platform specific folders for CDMs

To support bundled CDM using DefaultCompnentInstaller, the folder structure
of bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:

<DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

Then for bundled CDM, the folder structure should be like:

<DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64

Note that the <version> folder is not needed.

BUG=582622
TEST=All tests pass. Tested on Mac and Linux.
==========

to

==========
media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some
major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of
bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:

<DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

Then for bundled CDM, the folder structure should be like:

<DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64

Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we
have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we
don't use platform specific folders.
- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp
build). So we build to the default output dir first, then copy it over to the
platform specific folder. See http://crbug.com/611990 for more details.
- Updated all tests to support platform specific folders.
- Update installer files to bundle the CDM (adapter) in the right folder.

BUG=582622
TEST=All tests pass. Tested on Mac and Linux.
==========

[xhwang, 2016-05-18 22:59:37]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org

[xhwang, 2016-05-18 22:59:38]

I am still testing the changes in
chrome/tools/build/win/create_installer_archive.py and
chrome/installer/mini_installer/chrome.release on Windows.

Otherwise this CL should be in relatively good shape.

I'll add OWNERS shortly as well.

Please take a look!

[xhwang, 2016-05-18 23:52:22]

xhwang@chromium.org changed reviewers:
+ gab@chromium.org, rsesek@chromium.org, thestig@chromium.org

[xhwang, 2016-05-18 23:52:23]

gab: Please OWNERS review win installer changes, especially 
create_installer_archive.py and chrome.release changes. Note that the ToT is
broken for me (see crbug/612997) so I haven't got a chance to test it at all. I
am working on it.

rsesek: Please OWNERS review mac related changes. We do have a hack in the gyp
file to copy the adapter to avoid the issue with strip_save_dsym.

thestig: Please OWNERS review chrome/*. Also, now I am not changing the CDM path
for linux/cros, which makes this CL smaller and my life easier for now.

Note: To see the diff between my old CL and this one:
https://chromiumcodereview.appspot.com/1993973002/. There are a lot of rebase
noise + changes, so that won't help too much IMHO.

[ddorwin, 2016-05-19 00:04:27]

I did not review the cdm_paths files in detail (since they may move) nor the
last four files.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser/DEPS#...
chrome/browser/DEPS:36: "+media/cdm",  # For CDM paths
:( There is a lot of stuff there that shouldn't be used. Can we allow a single
header instead?

From a layering POV, paths seems like a chrome/ thing, though perhaps not if
they are related to where the files are built (and thus needed for media/
tests).

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser/load_...
File chrome/browser/load_library_perf_test.cc (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser/load_...
chrome/browser/load_library_perf_test.cc:28: void
MeasureSizeAndTimeToLoadNativeLibrary(const base::FilePath& library_dir,
s/dir/relative_path/?

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser_tests...
File chrome/browser_tests.isolate (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser_tests...
chrome/browser_tests.isolate:193: ['OS=="mac" and target_arch=="ia32"', {
Chrome no longer supports this.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser_tests...
chrome/browser_tests.isolate:209: ['OS=="mac" and branding=="Chrome" and
enable_pepper_cdms==1 and target_arch=="ia32"', {
ditto

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/chrome_tests....
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/chrome_tests....
chrome/chrome_tests.gypi:3162: '../media/media.gyp:media',
Is this for the CDM paths file?

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/chrome_tests_...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/chrome_tests_...
chrome/chrome_tests_unit.gypi:2156: 'browser/media/pepper_cdm_test_helper.cc',
This is in a different place from the GN version. Just confirming that this is
intentional.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/common/BUILD....
chrome/common/BUILD.gn:348: "//media",
Yikes. Common constants now depends on all of media?

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/tools/build/w...
File chrome/tools/build/win/create_installer_archive.py (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/tools/build/w...
chrome/tools/build/win/create_installer_archive.py:184:
variables['ComponentDir'] = '_platform_specific\win_' + target_arch
It's a relative path.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/BUILD.gn
File media/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/BUILD.gn#newco...
media/BUILD.gn:92: "cdm/cdm_paths.cc",
Might be worth making this its own component if we have to keep it in media/.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/cdm_paths.cc
File media/cdm/cdm_paths.cc (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/cdm_paths....
media/cdm/cdm_paths.cc:51: // TODO(xhwang): Use this function in Widevine CDM
component installer.
Shouldn't this code be common to all (such) components? Can we move it somewhere
that makes sense for that? Then we wouldn't have the media dependencies either.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/cdm_paths.h
File media/cdm/cdm_paths.h (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/cdm_paths....
media/cdm/cdm_paths.h:15: // Returns the platform specific relative path of a
component CDM. When CDM is
...path of the CDM. When...

It's always the path, regardless of component.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/external_c...
File media/cdm/external_clear_key_test_helper.cc (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/external_c...
media/cdm/external_clear_key_test_helper.cc:48: library_dir =
library_dir.Append(
(relative?) path.

Naming in line 47 might be misleading if that is also a path.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/ppapi/cdm_...
File media/cdm/ppapi/cdm_paths.gni (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/ppapi/cdm_...
media/cdm/ppapi/cdm_paths.gni:34: clearkey_cdm_path =
"ClearKeyCdm/$_platform_specific_path"
Really dir paths

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/ppapi/cdm_...
media/cdm/ppapi/cdm_paths.gni:35: widevine_cdm_path =
"WidevineCdm/$_platform_specific_path"
Could we then add a define that gets used in C++ files rather than rebuilding
this in the .cc file?

Mayb we can also make any code that wants to use the path depend on this file,
addressing the concern about broad media dependencies.

[xhwang, 2016-05-19 23:21:53]

As discussed offline, I am moving cdm_paths.h/cc to a separate target
(cdm_paths) to avoid having to depend on "media" to use cdm_paths.h.

I'll address other comments shortly.

[xhwang, 2016-05-20 00:22:36]

I addressed most comments. Still testing create_installer_archive.py on windows.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser/DEPS#...
chrome/browser/DEPS:36: "+media/cdm",  # For CDM paths
On 2016/05/19 00:04:26, ddorwin wrote:
> :( There is a lot of stuff there that shouldn't be used. Can we allow a single
> header instead?
> 
> From a layering POV, paths seems like a chrome/ thing, though perhaps not if
> they are related to where the files are built (and thus needed for media/
> tests).

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser/load_...
File chrome/browser/load_library_perf_test.cc (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser/load_...
chrome/browser/load_library_perf_test.cc:28: void
MeasureSizeAndTimeToLoadNativeLibrary(const base::FilePath& library_dir,
On 2016/05/19 00:04:26, ddorwin wrote:
> s/dir/relative_path/?

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser_tests...
File chrome/browser_tests.isolate (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser_tests...
chrome/browser_tests.isolate:193: ['OS=="mac" and target_arch=="ia32"', {
On 2016/05/19 00:04:26, ddorwin wrote:
> Chrome no longer supports this.

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/browser_tests...
chrome/browser_tests.isolate:209: ['OS=="mac" and branding=="Chrome" and
enable_pepper_cdms==1 and target_arch=="ia32"', {
On 2016/05/19 00:04:26, ddorwin wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/chrome_tests....
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/chrome_tests....
chrome/chrome_tests.gypi:3162: '../media/media.gyp:media',
On 2016/05/19 00:04:26, ddorwin wrote:
> Is this for the CDM paths file?

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/chrome_tests_...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/chrome_tests_...
chrome/chrome_tests_unit.gypi:2156: 'browser/media/pepper_cdm_test_helper.cc',
On 2016/05/19 00:04:26, ddorwin wrote:
> This is in a different place from the GN version. Just confirming that this is
> intentional.

In gyp it's a big test_support_common target. In GN it's split into smaller
ones.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/common/BUILD....
chrome/common/BUILD.gn:348: "//media",
On 2016/05/19 00:04:26, ddorwin wrote:
> Yikes. Common constants now depends on all of media?

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/tools/build/w...
File chrome/tools/build/win/create_installer_archive.py (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/chrome/tools/build/w...
chrome/tools/build/win/create_installer_archive.py:184:
variables['ComponentDir'] = '_platform_specific\win_' + target_arch
On 2016/05/19 00:04:26, ddorwin wrote:
> It's a relative path.

I am still testing this part. Will update this part later.

What name do you recommend?

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/BUILD.gn
File media/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/BUILD.gn#newco...
media/BUILD.gn:92: "cdm/cdm_paths.cc",
On 2016/05/19 00:04:26, ddorwin wrote:
> Might be worth making this its own component if we have to keep it in media/.

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/cdm_paths.cc
File media/cdm/cdm_paths.cc (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/cdm_paths....
media/cdm/cdm_paths.cc:51: // TODO(xhwang): Use this function in Widevine CDM
component installer.
On 2016/05/19 00:04:26, ddorwin wrote:
> Shouldn't this code be common to all (such) components? Can we move it
somewhere
> that makes sense for that? Then we wouldn't have the media dependencies
either.

Actually different components could choose to use different folders. A lot of
components actually use _platform_specific/all, which doesn't make sense.

We definitely should refactor this part to make thing more sane. Now I hate
_platform_specific so much :) I'll file a bug for this.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/cdm_paths.h
File media/cdm/cdm_paths.h (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/cdm_paths....
media/cdm/cdm_paths.h:15: // Returns the platform specific relative path of a
component CDM. When CDM is
On 2016/05/19 00:04:26, ddorwin wrote:
> ...path of the CDM. When...
> 
> It's always the path, regardless of component.

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/external_c...
File media/cdm/external_clear_key_test_helper.cc (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/external_c...
media/cdm/external_clear_key_test_helper.cc:48: library_dir =
library_dir.Append(
On 2016/05/19 00:04:26, ddorwin wrote:
> (relative?) path.
> 
> Naming in line 47 might be misleading if that is also a path.

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/ppapi/cdm_...
File media/cdm/ppapi/cdm_paths.gni (right):

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/ppapi/cdm_...
media/cdm/ppapi/cdm_paths.gni:34: clearkey_cdm_path =
"ClearKeyCdm/$_platform_specific_path"
On 2016/05/19 00:04:26, ddorwin wrote:
> Really dir paths

As discussed offline, we'll keep the current names here.

https://chromiumcodereview.appspot.com/1989893004/diff/1/media/cdm/ppapi/cdm_...
media/cdm/ppapi/cdm_paths.gni:35: widevine_cdm_path =
"WidevineCdm/$_platform_specific_path"
On 2016/05/19 00:04:26, ddorwin wrote:
> Could we then add a define that gets used in C++ files rather than rebuilding
> this in the .cc file?
> 
> Mayb we can also make any code that wants to use the path depend on this file,
> addressing the concern about broad media dependencies.

I thought about that but there are issues with that approach, e.g.
- We'll end up needing to use pulic_config to populate that macro/define up.
- We'll use macros in source code which isn't recommended
- We have to deal with path logic in the macro, e.g. window/posix path with
slash/backslash.

[xhwang, 2016-05-20 00:52:48]

I fixed create_installer_archive.py and now Windows is working. 

PTAL

[xhwang, 2016-05-20 04:03:42]

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

[commit-bot: I haz the power, 2016-05-20 04:03:53]

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

[ddorwin, 2016-05-20 04:08:56]

LGTM % nits.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/browser_t...
File chrome/browser_tests.isolate (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/browser_t...
chrome/browser_tests.isolate:202: ['OS=="mac" and branding=="Chrome" and
enable_pepper_cdms==1 and target_arch=="x64"', {
nit: Should we start with 'OS=="mac" and target_arch=="x64"... as we do above?

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/common/BU...
File chrome/common/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/common/BU...
chrome/common/BUILD.gn:348: "//media:cdm_paths",
Add "Needed by chrome_paths.cc." like the .gyp, especially since this does look
a bit out of place.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/installer...
File chrome/installer/mini_installer/chrome.release (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/installer...
chrome/installer/mini_installer/chrome.release:61:
WidevineCdm\_platform_specific\*\widevinecdmadapter.dll:
%(VersionDir)s\WidevineCdm\_platform_specific\win_%(Arch)s\
'*': You accept _any_/_all_ directories? What if you have built both ia32 and
x64? Will $(Arch) not work?

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
File media/cdm/cdm_paths.cc (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
media/cdm/cdm_paths.cc:14: // TODO(xhwang): Auto generate paths from build
variables so we don't need this
Do you still plan to do this?
Alternatively, you could refer to
https://bugs.chromium.org/p/chromium/issues/detail?id=468584.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
media/cdm/cdm_paths.cc:51: // TODO(xhwang): Use this function in Widevine CDM
component installer.
You probably don't need this in both files.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
File media/cdm/cdm_paths.h (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
media/cdm/cdm_paths.h:14: // Returns the relative path of a component CDM. On
platforms where a platform
Drop "component" from the first sentence.

Perhaps also reword to "Returns the path of the |cdm_base_path| CDM relative to
<modules dir?>."

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/ppapi/...
File media/cdm/ppapi/cdm_paths.gni (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/ppapi/...
media/cdm/ppapi/cdm_paths.gni:7: # Naming and folder structure below are
following the recommendation for chrome
nit: Chrome

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/ppapi/...
media/cdm/ppapi/cdm_paths.gni:11: # TODO(xhwang): Auto generate paths in C++
from build variables so we don't
ditto - still plan to do?

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/ppapi/...
media/cdm/ppapi/cdm_paths.gni:28: if (is_win || is_mac) {
You should probably explain this and reference
https://bugs.chromium.org/p/chromium/issues/detail?id=468584 to fix it.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm_paths....
File media/cdm_paths.gypi (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm_paths....
media/cdm_paths.gypi:7: # Naming and folder structure below are following the
recommendation for chrome
dittos

[xhwang, 2016-05-20 06:31:55]

Description was changed from

==========
media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some
major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of
bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:

<DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

Then for bundled CDM, the folder structure should be like:

<DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64

Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we
have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we
don't use platform specific folders.
- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp
build). So we build to the default output dir first, then copy it over to the
platform specific folder. See http://crbug.com/611990 for more details.
- Updated all tests to support platform specific folders.
- Update installer files to bundle the CDM (adapter) in the right folder.

BUG=582622
TEST=All tests pass. Tested on Mac and Linux.
==========

to

==========
media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some
major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of
bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:
    <DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

For bundled CDM, the folder structure should be like:
    <DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64
Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we
have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we
don't use platform specific folders.

- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp
build). So we build to the default output dir first, then copy it over to the
platform specific folder. See http://crbug.com/611990 for more details.

- Updated all tests to support platform specific folders.

- Update installer files to bundle the CDM (adapter) in the right folder.

BUG=582622
TEST=All tests pass. Tested on Mac and Linux.
==========

[commit-bot: I haz the power, 2016-05-20 06:49:33]

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

[commit-bot: I haz the power, 2016-05-20 06:49:34]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-05-20 07:18:20]

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/browser_t...
File chrome/browser_tests.isolate (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/browser_t...
chrome/browser_tests.isolate:202: ['OS=="mac" and branding=="Chrome" and
enable_pepper_cdms==1 and target_arch=="x64"', {
On 2016/05/20 04:08:56, ddorwin wrote:
> nit: Should we start with 'OS=="mac" and target_arch=="x64"... as we do above?

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/common/BU...
File chrome/common/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/common/BU...
chrome/common/BUILD.gn:348: "//media:cdm_paths",
On 2016/05/20 04:08:56, ddorwin wrote:
> Add "Needed by chrome_paths.cc." like the .gyp, especially since this does
look
> a bit out of place.

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/installer...
File chrome/installer/mini_installer/chrome.release (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/installer...
chrome/installer/mini_installer/chrome.release:61:
WidevineCdm\_platform_specific\*\widevinecdmadapter.dll:
%(VersionDir)s\WidevineCdm\_platform_specific\win_%(Arch)s\
On 2016/05/20 04:08:56, ddorwin wrote:
> '*': You accept _any_/_all_ directories? What if you have built both ia32 and
> x64? Will $(Arch) not work?

+gab

It seems the "key" doesn't support "interpolations" [1] :( So this is pretty
much the best I can have.

I just noted that we have both nacl_irt_x86_32.nexe and nacl_irt_x86_64.nexe
above. Does this mean we can actually put files on different "arch" here in this
list, and we'll pick what ever is available? If so, we actually don't need the
%(Arch)s here, and can just hard code both cases here, e.g.

WidevineCdm\_platform_specific\win_x64\widevinecdmadapter.dll:
%(VersionDir)s\Widevine
    Cdm\_platform_specific\win_x64\
WidevineCdm\_platform_specific\win_x86\widevinecdmadapter.dll:
%(VersionDir)s\Widevine
    Cdm\_platform_specific\win_x86\

This doesn't solve the corss-compile-using-same-folder issue, but that'll also
happen for nacl.

[1] https://docs.python.org/2/library/configparser.html

On top of the core functionality, SafeConfigParser supports interpolation. This
means VALUES can contain format strings which refer to other values in the same
section, or values in a special DEFAULT section. Additional defaults can be
provided on initialization.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
File media/cdm/cdm_paths.cc (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
media/cdm/cdm_paths.cc:14: // TODO(xhwang): Auto generate paths from build
variables so we don't need this
On 2016/05/20 04:08:56, ddorwin wrote:
> Do you still plan to do this?
> Alternatively, you could refer to
> https://bugs.chromium.org/p/chromium/issues/detail?id=468584.

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
media/cdm/cdm_paths.cc:51: // TODO(xhwang): Use this function in Widevine CDM
component installer.
On 2016/05/20 04:08:56, ddorwin wrote:
> You probably don't need this in both files.

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
File media/cdm/cdm_paths.h (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/cdm_pa...
media/cdm/cdm_paths.h:14: // Returns the relative path of a component CDM. On
platforms where a platform
On 2016/05/20 04:08:56, ddorwin wrote:
> Drop "component" from the first sentence.
> 
> Perhaps also reword to "Returns the path of the |cdm_base_path| CDM relative
to
> <modules dir?>."

Done, though DIR_COMPONENTS is really a "chrome" concept.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/ppapi/...
File media/cdm/ppapi/cdm_paths.gni (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/ppapi/...
media/cdm/ppapi/cdm_paths.gni:7: # Naming and folder structure below are
following the recommendation for chrome
On 2016/05/20 04:08:56, ddorwin wrote:
> nit: Chrome

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/ppapi/...
media/cdm/ppapi/cdm_paths.gni:11: # TODO(xhwang): Auto generate paths in C++
from build variables so we don't
On 2016/05/20 04:08:56, ddorwin wrote:
> ditto - still plan to do?

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm/ppapi/...
media/cdm/ppapi/cdm_paths.gni:28: if (is_win || is_mac) {
On 2016/05/20 04:08:56, ddorwin wrote:
> You should probably explain this and reference
> https://bugs.chromium.org/p/chromium/issues/detail?id=468584 to fix it.

Done.

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm_paths....
File media/cdm_paths.gypi (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/media/cdm_paths....
media/cdm_paths.gypi:7: # Naming and folder structure below are following the
recommendation for chrome
On 2016/05/20 04:08:56, ddorwin wrote:
> dittos

Done.

[xhwang, 2016-05-20 07:20:25]

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/installer...
File chrome/installer/mini_installer/chrome.release (right):

https://chromiumcodereview.appspot.com/1989893004/diff/60001/chrome/installer...
chrome/installer/mini_installer/chrome.release:61:
WidevineCdm\_platform_specific\*\widevinecdmadapter.dll:
%(VersionDir)s\WidevineCdm\_platform_specific\win_%(Arch)s\
On 2016/05/20 07:18:19, xhwang wrote:
> On 2016/05/20 04:08:56, ddorwin wrote:
> > '*': You accept _any_/_all_ directories? What if you have built both ia32
and
> > x64? Will $(Arch) not work?
> 
> +gab
> 
> It seems the "key" doesn't support "interpolations" [1] :( So this is pretty
> much the best I can have.
> 
> I just noted that we have both nacl_irt_x86_32.nexe and nacl_irt_x86_64.nexe
> above. Does this mean we can actually put files on different "arch" here in
this
> list, and we'll pick what ever is available? If so, we actually don't need the
> %(Arch)s here, and can just hard code both cases here, e.g.
> 
> WidevineCdm\_platform_specific\win_x64\widevinecdmadapter.dll:
> %(VersionDir)s\Widevine
>     Cdm\_platform_specific\win_x64\
> WidevineCdm\_platform_specific\win_x86\widevinecdmadapter.dll:
> %(VersionDir)s\Widevine
>     Cdm\_platform_specific\win_x86\
> 
> This doesn't solve the corss-compile-using-same-folder issue, but that'll also
> happen for nacl.
> 
> [1] https://docs.python.org/2/library/configparser.html
> 
> On top of the core functionality, SafeConfigParser supports interpolation.
This
> means VALUES can contain format strings which refer to other values in the
same
> section, or values in a special DEFAULT section. Additional defaults can be
> provided on initialization.

I tried it and it seems to be working. This will at least prevent copying the
wrong file into a platform specific folder. In the worst case (building 32/64 in
the same folder) we'll have both folders picked up, and only the right one will
be used at run time.

[xhwang, 2016-05-20 16:44:41]

gab / rsesek / thestig: 

Kindly ping. We are still considering merging this to M52. Your prompt review
would be highly appreciated!

gab: Please OWNERS review win installer changes, especially
create_installer_archive.py and chrome.release changes.

rsesek: Please OWNERS review mac related changes. We do have a hack in the gyp
file to copy the adapter to avoid the issue with strip_save_dsym.

thestig: Please OWNERS review chrome/*. Also, now I am not changing the CDM path
for linux/cros, which makes this CL smaller and my life easier for now.

[gab, 2016-05-20 19:47:27]

A few comments, latest PS doesn't have create_installer_archive.py ?

https://codereview.chromium.org/1989893004/diff/100001/chrome/installer/linux...
File chrome/installer/linux/BUILD.gn (right):

https://codereview.chromium.org/1989893004/diff/100001/chrome/installer/linux...
chrome/installer/linux/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All
rights reserved.
You'll need to ping a chrome/installer/linux/OWNERS for this file.

https://codereview.chromium.org/1989893004/diff/100001/chrome/installer/mini_...
File chrome/installer/mini_installer/chrome.release (right):

https://codereview.chromium.org/1989893004/diff/100001/chrome/installer/mini_...
chrome/installer/mini_installer/chrome.release:62:
WidevineCdm\_platform_specific\win_x64\widevinecdmadapter.dll:
%(VersionDir)s\WidevineCdm\_platform_specific\win_x64\
Add these to chrome/test/mini_installer/config/... 's configs to test they're
deployed properly?

https://codereview.chromium.org/1989893004/diff/100001/chrome/tools/build/win...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/1989893004/diff/100001/chrome/tools/build/win...
chrome/tools/build/win/FILES.cfg:360: 'buildtype': ['official'],
Should clarify with a FILES.cfg owner  (e.g. kerz/mmoss) but I think you'll need
  'optional': ['official'] 
here and above

[xhwang, 2016-05-20 19:54:33]

Comments only. Looking into mini_installer test now.

https://codereview.chromium.org/1989893004/diff/100001/chrome/installer/linux...
File chrome/installer/linux/BUILD.gn (right):

https://codereview.chromium.org/1989893004/diff/100001/chrome/installer/linux...
chrome/installer/linux/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All
rights reserved.
On 2016/05/20 19:47:27, gab wrote:
> You'll need to ping a chrome/installer/linux/OWNERS for this file.

I have thestig@ on the reviewer list. Actually the changes in this file are not
absolutely necessary since I am not changing the path on linux/cros. But keep
them here just for completeness.

https://codereview.chromium.org/1989893004/diff/100001/chrome/installer/mini_...
File chrome/installer/mini_installer/chrome.release (right):

https://codereview.chromium.org/1989893004/diff/100001/chrome/installer/mini_...
chrome/installer/mini_installer/chrome.release:62:
WidevineCdm\_platform_specific\win_x64\widevinecdmadapter.dll:
%(VersionDir)s\WidevineCdm\_platform_specific\win_x64\
On 2016/05/20 19:47:27, gab wrote:
> Add these to chrome/test/mini_installer/config/... 's configs to test they're
> deployed properly?

This is new to me. Looking...

https://codereview.chromium.org/1989893004/diff/100001/chrome/tools/build/win...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/1989893004/diff/100001/chrome/tools/build/win...
chrome/tools/build/win/FILES.cfg:360: 'buildtype': ['official'],
On 2016/05/20 19:47:27, gab wrote:
> Should clarify with a FILES.cfg owner  (e.g. kerz/mmoss) but I think you'll
need
>   'optional': ['official'] 
> here and above

oh, is it because we don't have both files at the same time? That kinda makes
sense.

Hmm, but won't the "arch" part be sufficient?

[Michael Moss, 2016-05-20 20:01:00]

mmoss@chromium.org changed reviewers:
+ mmoss@chromium.org

[Michael Moss, 2016-05-20 20:01:02]

https://codereview.chromium.org/1989893004/diff/100001/chrome/tools/build/win...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/1989893004/diff/100001/chrome/tools/build/win...
chrome/tools/build/win/FILES.cfg:360: 'buildtype': ['official'],
On 2016/05/20 19:54:33, xhwang wrote:
> On 2016/05/20 19:47:27, gab wrote:
> > Should clarify with a FILES.cfg owner  (e.g. kerz/mmoss) but I think you'll
> need
> >   'optional': ['official'] 
> > here and above
> 
> oh, is it because we don't have both files at the same time? That kinda makes
> sense.
> 
> Hmm, but won't the "arch" part be sufficient?

LGTM

[gab, 2016-05-20 20:01:19]

gab@chromium.org changed reviewers:
- mmoss@chromium.org

[gab, 2016-05-20 20:01:20]

https://codereview.chromium.org/1989893004/diff/100001/chrome/tools/build/win...
File chrome/tools/build/win/FILES.cfg (right):

https://codereview.chromium.org/1989893004/diff/100001/chrome/tools/build/win...
chrome/tools/build/win/FILES.cfg:360: 'buildtype': ['official'],
On 2016/05/20 19:54:33, xhwang wrote:
> On 2016/05/20 19:47:27, gab wrote:
> > Should clarify with a FILES.cfg owner  (e.g. kerz/mmoss) but I think you'll
> need
> >   'optional': ['official'] 
> > here and above
> 
> oh, is it because we don't have both files at the same time? That kinda makes
> sense.
> 
> Hmm, but won't the "arch" part be sufficient?

Ah, probably yes :-) -- but again, verify with kerz or mmoss who own these.

[Lei Zhang, 2016-05-20 20:23:57]

lgtm

[xhwang, 2016-05-20 22:45:49]

Description was changed from

==========
media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some
major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of
bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:
    <DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

For bundled CDM, the folder structure should be like:
    <DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64
Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we
have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we
don't use platform specific folders.

- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp
build). So we build to the default output dir first, then copy it over to the
platform specific folder. See http://crbug.com/611990 for more details.

- Updated all tests to support platform specific folders.

- Update installer files to bundle the CDM (adapter) in the right folder.

BUG=582622
TEST=All tests pass. Tested on Mac and Linux.
==========

to

==========
media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some
major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of
bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:
    <DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

For bundled CDM, the folder structure should be like:
    <DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64
Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we
have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we
don't use platform specific folders.

- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp
build). So we build to the default output dir first, then copy it over to the
platform specific folder. See http://crbug.com/611990 for more details.

- Updated all tests to support platform specific folders.

- Update installer files to bundle the CDM (adapter) in the right folder.

TBR=gab@chromium.org
BUG=582622
TEST=All tests pass. Tested on Mac and Linux.
==========

[xhwang, 2016-05-20 22:47:42]

TBRing gab as discussed offline. Meanwhile I'll try to get test_installer
working for widevine CDMs.

For the record:

Including files for multiple arch in the same chrome.release should be okay
since it's best effort.

[xhwang, 2016-05-21 00:48:09]

rsesek@: For your convenience, these are the three places that have mac specific
code :)

https://chromiumcodereview.appspot.com/1989893004/diff/100001/chrome/chrome_d...

https://chromiumcodereview.appspot.com/1989893004/diff/100001/media/media_cdm...

https://chromiumcodereview.appspot.com/1989893004/diff/100001/third_party/wid...

I tend to land it soon to get more bake time on official bots. I'll watch the
bots closely after that. Let me know if there are concerns. Thanks!

[xhwang, 2016-05-22 05:03:42]

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

[commit-bot: I haz the power, 2016-05-22 05:03:49]

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

[commit-bot: I haz the power, 2016-05-22 06:14:43]

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

[commit-bot: I haz the power, 2016-05-22 06:14:45]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-05-22 13:39:10]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-05-22 13:39:11]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1989893004/#ps100001 (title: "specify
both paths in chrome.release")

[commit-bot: I haz the power, 2016-05-22 13:39:14]

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

[commit-bot: I haz the power, 2016-05-22 13:45:01]

Description was changed from

==========
media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some
major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of
bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:
    <DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

For bundled CDM, the folder structure should be like:
    <DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64
Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we
have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we
don't use platform specific folders.

- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp
build). So we build to the default output dir first, then copy it over to the
platform specific folder. See http://crbug.com/611990 for more details.

- Updated all tests to support platform specific folders.

- Update installer files to bundle the CDM (adapter) in the right folder.

TBR=gab@chromium.org
BUG=582622
TEST=All tests pass. Tested on Mac and Linux.
==========

to

==========
media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some
major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of
bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:
    <DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

For bundled CDM, the folder structure should be like:
    <DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64
Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we
have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we
don't use platform specific folders.

- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp
build). So we build to the default output dir first, then copy it over to the
platform specific folder. See http://crbug.com/611990 for more details.

- Updated all tests to support platform specific folders.

- Update installer files to bundle the CDM (adapter) in the right folder.

TBR=gab@chromium.org
BUG=582622
TEST=All tests pass. Tested on Mac and Linux.
==========

[commit-bot: I haz the power, 2016-05-22 13:45:03]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-22 13:46:45]

Description was changed from

==========
media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some
major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of
bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:
    <DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

For bundled CDM, the folder structure should be like:
    <DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64
Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we
have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we
don't use platform specific folders.

- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp
build). So we build to the default output dir first, then copy it over to the
platform specific folder. See http://crbug.com/611990 for more details.

- Updated all tests to support platform specific folders.

- Update installer files to bundle the CDM (adapter) in the right folder.

TBR=gab@chromium.org
BUG=582622
TEST=All tests pass. Tested on Mac and Linux.
==========

to

==========
media: Use platform specific folders for CDMs

This is a reland of commit c8450e051cb3cc4705f698428ae42631aabe3672 with some
major refactoring and fixes.

* Purpose of this CL:

To support bundled CDM using DefaultCompnentInstaller, the folder structure of
bundled CDMs should be the same as component updated CDMs.

For component CDMs, the folder structure is like:
    <DIR_USER_DATA>/WidevineCdm/<version>/_platform_specific/win_x64

For bundled CDM, the folder structure should be like:
    <DIR_COMPONENTS>/WidevineCdm/_platform_specific/win_x64
Note that the <version> folder is not needed in this case.

* What this CL does:

- Introduce cdm_paths.* to put CDMs in platform specific folders. On Win/Mac, we
have something like WidevineCdm/_platform_specific/win_x64. On Linux/Cros, we
don't use platform specific folders.

- On Mac, strip_save_dsym doesn't work with targets with "product_dir" (in gyp
build). So we build to the default output dir first, then copy it over to the
platform specific folder. See http://crbug.com/611990 for more details.

- Updated all tests to support platform specific folders.

- Update installer files to bundle the CDM (adapter) in the right folder.

TBR=gab@chromium.org
BUG=582622
TEST=All tests pass. Tested on Mac and Linux.

Committed: https://crrev.com/e13213d846ff7bdd794588b7efd152c3b5080a8d
Cr-Commit-Position: refs/heads/master@{#395275}
==========

[commit-bot: I haz the power, 2016-05-22 13:46:47]

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

[xhwang, 2016-05-22 14:29:26]

A revert of this CL (patchset #6 id:100001) has been created in
https://chromiumcodereview.appspot.com/2001953002/ by xhwang@chromium.org.

The reason for reverting is: This is breaking the Mac bot. Apparently I need to
use the "copy" trick for clearkeycdm as well to avoid the strip_save_dsym issue.

https://build.chromium.org/p/chromium/waterfall?builder=Mac.