Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2136983002:
Bundle fake Widevine component manifest for stub CDM (Closed)
|
Created:
4 years, 5 months ago by wdzierzanowski Modified:
4 years, 3 months ago CC:
chromium-reviews, eme-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBundle fake Widevine component manifest for stub CDM
Encrypted media browser tests involving Widevine use the stub CDM when
branding != Chrome. This change allows them to register the
preinstalled component containing the stub CDM.
BUG=622273
TEST=Widevine browser tests 'browser_tests --gtest_filter=*Widevine*' pass
Committed: https://crrev.com/119e34c6ce824a2ecd21e0aa3bc401ae604110fa
Cr-Commit-Position: refs/heads/master@{#414706}
Patch Set 1 #Patch Set 2 : Avoid linking libwidevinecdm.dylib into Chromium Framework #Patch Set 3 : Use commas consistently #
Total comments: 16
Patch Set 4 : Chrome branding doesn't imply enable_widevine. Tune stub manifest #
Total comments: 2
Patch Set 5 : Revert GYP changes #Patch Set 6 : Rebase #Patch Set 7 : Rebase #
Messages
Total messages: 29 (11 generated)
wdzierzanowski@opera.com changed reviewers: + ddorwin@chromium.org, dpranke@chromium.org, xhwang@chromium.org
PTAL
The CQ bit was checked by wdzierzanowski@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm
Added a workaround for linking libwidevinecdm into Chromium Framework directly in patchset 2. ddorwin: Could you PTAL at third_party/widevine/?
ddorwin: Would you be able to take a look at third_party/widevine/? Or should I wait for xhwang with this?
https://codereview.chromium.org/2136983002/diff/40001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2136983002/diff/40001/chrome/BUILD.gn#newcode804 chrome/BUILD.gn:804: if (enable_widevine) { enable_widevine does not appear to be set for Chrome builds. (This is something we need to clean up once the switch to GN is complete.) I think this should be: if ((is_chrome_branded || enable_widevine) && enable_pepper_cdms) as in https://cs.chromium.org/chromium/src/third_party/widevine/cdm/BUILD.gn?q=enab... enable_pepper_cdms seems to be implied in this file, but it might not hurt to be explicit. https://codereview.chromium.org/2136983002/diff/40001/chrome/BUILD.gn#newcode817 chrome/BUILD.gn:817: # Need this intermediate dependency because "widevinecdm" is a Does this only apply to !is_chrome_branded? It appears we shouldn't be doing this for !is_chrome_branded, which would make line 804 correct, but then where does the correct thing happen for is_chrome_branded? https://codereview.chromium.org/2136983002/diff/40001/chrome/chrome_dll_bundl... File chrome/chrome_dll_bundle.gypi (right): https://codereview.chromium.org/2136983002/diff/40001/chrome/chrome_dll_bundl... chrome/chrome_dll_bundle.gypi:143: 'files': [ Don't we still need a condition for Chrome || enable_widevine? https://codereview.chromium.org/2136983002/diff/40001/chrome/chrome_dll_bundl... chrome/chrome_dll_bundle.gypi:144: '<(PRODUCT_DIR)/<(widevine_cdm_path)/libwidevinecdm.dylib', It's not clear to me that putting the stub CDM or manifest in the bundle is the correct thing to do since this will actually be in the app bundle. This works fine for testing but you wouldn't want to ship it. https://codereview.chromium.org/2136983002/diff/40001/third_party/widevine/cd... File third_party/widevine/cdm/stub/manifest.json (right): https://codereview.chromium.org/2136983002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/stub/manifest.json:7: "minimum_chrome_version": "43.0.2340.0", Maybe this should be clearly garbage. Also, M54, which is when we added this. So, something like "54.0.0.0". https://codereview.chromium.org/2136983002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/stub/manifest.json:8: "x-cdm-module-versions": "4", We'll need to remember to update the next 4 lines when making changes. https://codereview.chromium.org/2136983002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/stub/manifest.json:25: "arch": "x86", This is no longer supported, so we can remove it.
https://codereview.chromium.org/2136983002/diff/40001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2136983002/diff/40001/chrome/BUILD.gn#newcode804 chrome/BUILD.gn:804: if (enable_widevine) { On 2016/07/18 18:35:17, ddorwin wrote: > enable_widevine does not appear to be set for Chrome builds. (This is something > we need to clean up once the switch to GN is complete.) > > I think this should be: > if ((is_chrome_branded || enable_widevine) && enable_pepper_cdms) > as in > https://cs.chromium.org/chromium/src/third_party/widevine/cdm/BUILD.gn?q=enab... > > enable_pepper_cdms seems to be implied in this file, but it might not hurt to be > explicit. Done. https://codereview.chromium.org/2136983002/diff/40001/chrome/BUILD.gn#newcode817 chrome/BUILD.gn:817: # Need this intermediate dependency because "widevinecdm" is a On 2016/07/18 18:35:17, ddorwin wrote: > Does this only apply to !is_chrome_branded? > > It appears we shouldn't be doing this for !is_chrome_branded, which would make > line 804 correct, but then where does the correct thing happen for > is_chrome_branded? The intermediate dependency is not required for !is_chrome_branded, but it doesn't hurt, either. The gn file is easier to manage if there is one dependency chain for both cases, I think. You are right that line 804 needs fixing -- I forgot that is_chrome_branded doesn't imply enable_widevine. https://codereview.chromium.org/2136983002/diff/40001/chrome/chrome_dll_bundl... File chrome/chrome_dll_bundle.gypi (right): https://codereview.chromium.org/2136983002/diff/40001/chrome/chrome_dll_bundl... chrome/chrome_dll_bundle.gypi:143: 'files': [ On 2016/07/18 18:35:17, ddorwin wrote: > Don't we still need a condition for Chrome || enable_widevine? Ah, of course. Thanks for catching it! https://codereview.chromium.org/2136983002/diff/40001/chrome/chrome_dll_bundl... chrome/chrome_dll_bundle.gypi:143: 'files': [ On 2016/07/18 18:35:17, ddorwin wrote: > Don't we still need a condition for Chrome || enable_widevine? Done. https://codereview.chromium.org/2136983002/diff/40001/chrome/chrome_dll_bundl... chrome/chrome_dll_bundle.gypi:144: '<(PRODUCT_DIR)/<(widevine_cdm_path)/libwidevinecdm.dylib', On 2016/07/18 18:35:17, ddorwin wrote: > It's not clear to me that putting the stub CDM or manifest in the bundle is the > correct thing to do since this will actually be in the app bundle. This works > fine for testing but you wouldn't want to ship it. You're right, this must not be shipped. I'm a bit on the fence about it myself. On the one hand, Opera doesn't really need this change. The target that builds the encrypted media browser tests doesn't even use this and we need to take care of bundling Widevine for tests separately anyway. I don't know about the other browsers. On the other hand, adding the bundling here for enable_widevine==1 has the advantage of making this and similar changes testable in a Chromium-branded build. Currently, it's only testable in an official Chrome build AFAIK. Given that we already have to make sure we don't ship the stub CDM that is already put in the bundle by Opera-specific build files to enable its use in tests, I'm leaning towards the latter though. https://codereview.chromium.org/2136983002/diff/40001/third_party/widevine/cd... File third_party/widevine/cdm/stub/manifest.json (right): https://codereview.chromium.org/2136983002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/stub/manifest.json:7: "minimum_chrome_version": "43.0.2340.0", On 2016/07/18 18:35:17, ddorwin wrote: > Maybe this should be clearly garbage. Also, M54, which is when we added this. > So, something like "54.0.0.0". Done. https://codereview.chromium.org/2136983002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/stub/manifest.json:8: "x-cdm-module-versions": "4", On 2016/07/18 18:35:17, ddorwin wrote: > We'll need to remember to update the next 4 lines when making changes. Unfortunately, yes. https://codereview.chromium.org/2136983002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/stub/manifest.json:25: "arch": "x86", On 2016/07/18 18:35:17, ddorwin wrote: > This is no longer supported, so we can remove it. Done.
https://codereview.chromium.org/2136983002/diff/40001/chrome/chrome_dll_bundl... File chrome/chrome_dll_bundle.gypi (right): https://codereview.chromium.org/2136983002/diff/40001/chrome/chrome_dll_bundl... chrome/chrome_dll_bundle.gypi:144: '<(PRODUCT_DIR)/<(widevine_cdm_path)/libwidevinecdm.dylib', I feel like I need to clarify some bits of what I wrote: > On the one hand, Opera doesn't really > need this change. The target that builds the encrypted media browser tests > doesn't even use this and we need to take care of bundling Widevine for tests > separately anyway. I don't know about the other browsers. Here, by "this change" I actually meant just the changes to the build files in chrome/. > On the other hand, adding the bundling here for enable_widevine==1 has the > advantage of making this and similar changes testable in a Chromium-branded > build. Currently, it's only testable in an official Chrome build AFAIK. This time around, "this change" refers to the entire CL. Sorry for the confusing language :)
Hi xhwang, ddorwin, I'm back from vacation :) When you are available too and have the time, please take a look at the updates.
Sorry for the delay. I'll start looking at this shortly.
GYP is being deprecated and probably you don't need worry about the gyp file anymore (unless you are still using it). Looking good. I just have one question. https://chromiumcodereview.appspot.com/2136983002/diff/60001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://chromiumcodereview.appspot.com/2136983002/diff/60001/chrome/BUILD.gn#... chrome/BUILD.gn:823: ":widevine_cdm_library_copy", Can this be a data_deps? https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
I've reverted the GYP changes. https://chromiumcodereview.appspot.com/2136983002/diff/60001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://chromiumcodereview.appspot.com/2136983002/diff/60001/chrome/BUILD.gn#... chrome/BUILD.gn:823: ":widevine_cdm_library_copy", On 2016/08/25 00:05:45, xhwang (slow) wrote: > Can this be a data_deps? > > https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen... Tried it. Unfortunately, GN now doesn't generate the CDM dylib. It builds a dependency chain from //chrome:chrome_framework to //third_party/widevine/cdm:widevinecdm, but: "If you have generated inputs, there needs to be a dependency path between the two targets in addition to just listing the files. For indirect dependencies, the intermediate ones must be public_deps. data_deps don't count since they're only runtime dependencies."
Rebased the CL.
lgtm
Thanks for reviewing!
The CQ bit was checked by wdzierzanowski@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2136983002/#ps100001 (title: "Rebase")
The CQ bit was unchecked by wdzierzanowski@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by wdzierzanowski@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, xhwang@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2136983002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Bundle fake Widevine component manifest for stub CDM Encrypted media browser tests involving Widevine use the stub CDM when branding != Chrome. This change allows them to register the preinstalled component containing the stub CDM. BUG=622273 TEST=Widevine browser tests 'browser_tests --gtest_filter=*Widevine*' pass ========== to ========== Bundle fake Widevine component manifest for stub CDM Encrypted media browser tests involving Widevine use the stub CDM when branding != Chrome. This change allows them to register the preinstalled component containing the stub CDM. BUG=622273 TEST=Widevine browser tests 'browser_tests --gtest_filter=*Widevine*' pass Committed: https://crrev.com/119e34c6ce824a2ecd21e0aa3bc401ae604110fa Cr-Commit-Position: refs/heads/master@{#414706} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/119e34c6ce824a2ecd21e0aa3bc401ae604110fa Cr-Commit-Position: refs/heads/master@{#414706} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
