media: Bundle Widevine CDM on Win/Mac.

media: Bundle Widevine CDM on Win/Mac.

This will ship Widevine CDM by default on Win/Mac, while keeping the ability to component update the CDM.

BUG=582622
Committed: https://crrev.com/6c65fb1996667b25ab74b7b9abed301f3d4e6741
Cr-Commit-Position: refs/heads/master@{#395289}

[xhwang, 2016-05-20 07:21:52]

Description was changed from

==========
media: Bundle Widevine CDM with chrome on Win/Mac.

BUG=582622
==========

to

==========
media: Bundle Widevine CDM on Win/Mac.

BUG=582622
==========

[xhwang, 2016-05-20 19:17:56]

Patchset #3 (id:40001) has been deleted

[xhwang, 2016-05-20 19:22:44]

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

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

This is the second CL that really ships the manifest and CDM on Win/Mac.

PTAL

[xhwang, 2016-05-20 19:30:00]

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/browser/c...
File chrome/browser/component_updater/widevine_cdm_component_installer.cc
(right):

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/browser/c...
chrome/browser/component_updater/widevine_cdm_component_installer.cc:357: if
(adapter_install_path != adapter_source_path &&
Being paranoid about this check... If there's an issue in this code (I don't
know how), we could end up overwritting the bundled adapter on line 369. Maybe
we should check that if the install path is within DIR_COMPONENT, don't copy,
just to be on the safer side?

[ddorwin, 2016-05-20 21:37:22]

LGTM with comments

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/browser/c...
File chrome/browser/component_updater/widevine_cdm_component_installer.cc
(right):

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/browser/c...
chrome/browser/component_updater/widevine_cdm_component_installer.cc:357: if
(adapter_install_path != adapter_source_path &&
On 2016/05/20 19:30:00, xhwang wrote:
> Being paranoid about this check... If there's an issue in this code (I don't
> know how), we could end up overwritting the bundled adapter on line 369. Maybe
> we should check that if the install path is within DIR_COMPONENT, don't copy,
> just to be on the safer side?

You should have a comment to explain this. To isolate the condition, break it
up:

if (!=) 
  NOTREACHED() << Reason;
  return;
}

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/chrome_dl...
File chrome/chrome_dll_bundle.gypi (right):

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/chrome_dl...
chrome/chrome_dll_bundle.gypi:138: 'destination':
'<(PRODUCT_DIR)/$(CONTENTS_FOLDER_PATH)/Libraries/<(widevine_cdm_path)',
Given the presence of "WidevineCdm" below (and need for both a ECK and WV path),
I wonder if this should be:
.../WidevineCdm/<(platform_specific_path)

[xhwang, 2016-05-20 23:46:11]

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

[xhwang, 2016-05-20 23:46:11]

waffles: Please OWNERS review widevine_cdm_component_installer.cc

thestig: Pleas OWNERS review chrome/

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/browser/c...
File chrome/browser/component_updater/widevine_cdm_component_installer.cc
(right):

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/browser/c...
chrome/browser/component_updater/widevine_cdm_component_installer.cc:357: if
(adapter_install_path != adapter_source_path &&
On 2016/05/20 21:37:22, ddorwin wrote:
> On 2016/05/20 19:30:00, xhwang wrote:
> > Being paranoid about this check... If there's an issue in this code (I don't
> > know how), we could end up overwritting the bundled adapter on line 369.
Maybe
> > we should check that if the install path is within DIR_COMPONENT, don't
copy,
> > just to be on the safer side?
> 
> You should have a comment to explain this. To isolate the condition, break it
> up:
> 
> if (!=) 
>   NOTREACHED() << Reason;
>   return;
> }

Updated comments.

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/chrome_dl...
File chrome/chrome_dll_bundle.gypi (right):

https://chromiumcodereview.appspot.com/2000513002/diff/60001/chrome/chrome_dl...
chrome/chrome_dll_bundle.gypi:138: 'destination':
'<(PRODUCT_DIR)/$(CONTENTS_FOLDER_PATH)/Libraries/<(widevine_cdm_path)',
On 2016/05/20 21:37:22, ddorwin wrote:
> Given the presence of "WidevineCdm" below (and need for both a ECK and WV
path),
> I wonder if this should be:
> .../WidevineCdm/<(platform_specific_path)

Yes for the future. For now, the tricky part is that on linux/cros, we are not
even using the WidevineCdm part, so this will not work.

We could have a follow-up CL later to move linux/cros CDMs to WidevineCdm
folder, then we can do what you suggested.

[xhwang, 2016-05-20 23:46:51]

Description was changed from

==========
media: Bundle Widevine CDM on Win/Mac.

BUG=582622
==========

to

==========
media: Bundle Widevine CDM on Win/Mac.

This will ship Widevine CDM by default on Win/Mac, while keeping the ability to
component update the CDM.

BUG=582622
==========

[Lei Zhang, 2016-05-20 23:54:28]

lgtm

https://chromiumcodereview.appspot.com/2000513002/diff/100001/chrome/browser/...
File chrome/browser/component_updater/widevine_cdm_component_installer.cc
(right):

https://chromiumcodereview.appspot.com/2000513002/diff/100001/chrome/browser/...
chrome/browser/component_updater/widevine_cdm_component_installer.cc:361:
(!base::ReadFileToString(adapter_version_path, &adapter_version) ||
Maybe break this off into:
AdapterVersionMatchesChromeVersion(adapter_version_path, chrome_version) ?

https://chromiumcodereview.appspot.com/2000513002/diff/100001/chrome/chrome_d...
File chrome/chrome_dll_bundle.gypi (right):

https://chromiumcodereview.appspot.com/2000513002/diff/100001/chrome/chrome_d...
chrome/chrome_dll_bundle.gypi:144:
'<(PRODUCT_DIR)/<(widevine_cdm_path)/libwidevinecdm.dylib',
nit: alphabetical order

[waffles, 2016-05-20 23:56:59]

lgtm % existing comments

[xhwang, 2016-05-21 00:25:58]

https://chromiumcodereview.appspot.com/2000513002/diff/100001/chrome/browser/...
File chrome/browser/component_updater/widevine_cdm_component_installer.cc
(right):

https://chromiumcodereview.appspot.com/2000513002/diff/100001/chrome/browser/...
chrome/browser/component_updater/widevine_cdm_component_installer.cc:361:
(!base::ReadFileToString(adapter_version_path, &adapter_version) ||
On 2016/05/20 23:54:28, Lei Zhang wrote:
> Maybe break this off into:
> AdapterVersionMatchesChromeVersion(adapter_version_path, chrome_version) ?

Done.

https://chromiumcodereview.appspot.com/2000513002/diff/100001/chrome/chrome_d...
File chrome/chrome_dll_bundle.gypi (right):

https://chromiumcodereview.appspot.com/2000513002/diff/100001/chrome/chrome_d...
chrome/chrome_dll_bundle.gypi:144:
'<(PRODUCT_DIR)/<(widevine_cdm_path)/libwidevinecdm.dylib',
On 2016/05/20 23:54:28, Lei Zhang wrote:
> nit: alphabetical order

Done.

[xhwang, 2016-05-21 00:26:38]

Patchset #6 (id:120001) has been deleted

[xhwang, 2016-05-23 05:02:06]

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

[commit-bot: I haz the power, 2016-05-23 05:02:23]

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

[xhwang, 2016-05-23 06:23:54]

The CQ bit was unchecked by xhwang@chromium.org

[xhwang, 2016-05-23 06:23:55]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-05-23 06:23:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
ddorwin@chromium.org, waffles@chromium.org, thestig@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2000513002/#ps140001 (title: "comments")

[commit-bot: I haz the power, 2016-05-23 06:23:59]

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

[commit-bot: I haz the power, 2016-05-23 06:45:40]

Description was changed from

==========
media: Bundle Widevine CDM on Win/Mac.

This will ship Widevine CDM by default on Win/Mac, while keeping the ability to
component update the CDM.

BUG=582622
==========

to

==========
media: Bundle Widevine CDM on Win/Mac.

This will ship Widevine CDM by default on Win/Mac, while keeping the ability to
component update the CDM.

BUG=582622
==========

[commit-bot: I haz the power, 2016-05-23 06:45:41]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-05-23 06:47:17]

Description was changed from

==========
media: Bundle Widevine CDM on Win/Mac.

This will ship Widevine CDM by default on Win/Mac, while keeping the ability to
component update the CDM.

BUG=582622
==========

to

==========
media: Bundle Widevine CDM on Win/Mac.

This will ship Widevine CDM by default on Win/Mac, while keeping the ability to
component update the CDM.

BUG=582622

Committed: https://crrev.com/6c65fb1996667b25ab74b7b9abed301f3d4e6741
Cr-Commit-Position: refs/heads/master@{#395289}
==========

[commit-bot: I haz the power, 2016-05-23 06:47:19]

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