Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2033923002:
[Mac/GN] Include WidevineCdm for Chrome builds. (Closed)
|
Created:
4 years, 6 months ago by Robert Sesek Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac/GN] Include WidevineCdm for Chrome builds.
BUG=615486
R=xhwang@chromium.org
Committed: https://crrev.com/34a7667c244f3d912d7dbbdfde37331b09f88efc
Cr-Commit-Position: refs/heads/master@{#397830}
Patch Set 1 #
Total comments: 6
Patch Set 2 : plugin -> library #Patch Set 3 : #
Total comments: 3
Messages
Total messages: 17 (3 generated)
LGTM with nits https://chromiumcodereview.appspot.com/2033923002/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://chromiumcodereview.appspot.com/2033923002/diff/1/chrome/BUILD.gn#newc... chrome/BUILD.gn:721: bundle_data("widevine_plugin_libs") { nit: here and below, s/plugin/cdm https://chromiumcodereview.appspot.com/2033923002/diff/1/chrome/BUILD.gn#newc... chrome/BUILD.gn:735: bundle_data("widevine_plugin_manifest") { ditto https://chromiumcodereview.appspot.com/2033923002/diff/1/chrome/BUILD.gn#newc... chrome/BUILD.gn:751: group("widevine_plugin") { ditto
https://codereview.chromium.org/2033923002/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2033923002/diff/1/chrome/BUILD.gn#newcode721 chrome/BUILD.gn:721: bundle_data("widevine_plugin_libs") { On 2016/06/02 21:04:44, xhwang wrote: > nit: here and below, s/plugin/cdm I'm going to use "library" instead because that's the name of the destination folder in the bundle.
https://codereview.chromium.org/2033923002/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2033923002/diff/1/chrome/BUILD.gn#newcode721 chrome/BUILD.gn:721: bundle_data("widevine_plugin_libs") { On 2016/06/03 15:43:01, Robert Sesek wrote: > On 2016/06/02 21:04:44, xhwang wrote: > > nit: here and below, s/plugin/cdm > > I'm going to use "library" instead because that's the name of the destination > folder in the bundle. Well, library_libs seems redundant. I feel the "libs" part already covers the "Libraries" folder in the bundle. The sub-folder under Libraries is called WidevineCdm, so "widevine" without "cdm" doesn't make too much sense in Chrome.
https://codereview.chromium.org/2033923002/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2033923002/diff/1/chrome/BUILD.gn#newcode721 chrome/BUILD.gn:721: bundle_data("widevine_plugin_libs") { On 2016/06/03 16:09:34, xhwang wrote: > On 2016/06/03 15:43:01, Robert Sesek wrote: > > On 2016/06/02 21:04:44, xhwang wrote: > > > nit: here and below, s/plugin/cdm > > > > I'm going to use "library" instead because that's the name of the destination > > folder in the bundle. > > Well, library_libs seems redundant. I feel the "libs" part already covers the > "Libraries" folder in the bundle. It isn't redundant. The "libs" covers the fact that these are binaries (which is what I switched this to in ps3) versus the manifest, and "library" covers the destination in the bundle. > The sub-folder under Libraries is called > WidevineCdm, so "widevine" without "cdm" doesn't make too much sense in Chrome. Added cdm to the name because you seem to want it.
lgtm++, thanks!
+thakis for OWNERS
rsesek@chromium.org changed reviewers: + thakis@chromium.org
+thakis for realz
lgtm if answer below is "yes" https://codereview.chromium.org/2033923002/diff/40001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2033923002/diff/40001/chrome/BUILD.gn#newcode724 chrome/BUILD.gn:724: "$root_out_dir/$widevine_cdm_path/widevinecdmadapter.plugin", These don't need an install_name_tool thing 'cause they're loadable_modules and not shared_libraries and loaded via dlopen instead of by dyld, yes?
https://codereview.chromium.org/2033923002/diff/40001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2033923002/diff/40001/chrome/BUILD.gn#newcode724 chrome/BUILD.gn:724: "$root_out_dir/$widevine_cdm_path/widevinecdmadapter.plugin", On 2016/06/03 20:58:17, Nico wrote: > These don't need an install_name_tool thing 'cause they're loadable_modules and > not shared_libraries and loaded via dlopen instead of by dyld, yes? The .plugin is a loadable_module found via dlopen but the .dylib is a shared_library (but it's already set up as @loader_path).
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033923002/40001
https://codereview.chromium.org/2033923002/diff/40001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/2033923002/diff/40001/chrome/BUILD.gn#newcode724 chrome/BUILD.gn:724: "$root_out_dir/$widevine_cdm_path/widevinecdmadapter.plugin", On 2016/06/03 21:01:22, Robert Sesek wrote: > On 2016/06/03 20:58:17, Nico wrote: > > These don't need an install_name_tool thing 'cause they're loadable_modules > and > > not shared_libraries and loaded via dlopen instead of by dyld, yes? > > The .plugin is a loadable_module found via dlopen but the .dylib is a > shared_library (but it's already set up as @loader_path). I'd add a "# the dylib has a @loader_path that makes Chromium Framework expect the dylib to be at the path we're moving it to" type of comment above the dylib then
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Mac/GN] Include WidevineCdm for Chrome builds. BUG=615486 R=xhwang@chromium.org ========== to ========== [Mac/GN] Include WidevineCdm for Chrome builds. BUG=615486 R=xhwang@chromium.org Committed: https://crrev.com/34a7667c244f3d912d7dbbdfde37331b09f88efc Cr-Commit-Position: refs/heads/master@{#397830} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/34a7667c244f3d912d7dbbdfde37331b09f88efc Cr-Commit-Position: refs/heads/master@{#397830} |
