media: Move Widevine signature of Framework into a bundle

media: Move Widevine signature of Framework into a bundle

Currently the Framework signature is in the version folder directly
which does not match Mac bundle format. Instead, create a new bundle
for the signature ("Widevine Signature.bundle") which is located
beside "Google Chrome Framework.framework".

Also update CdmHostFilePath to match the new folder structure.

BUG=704762
Review-Url: https://codereview.chromium.org/2776613002
Cr-Commit-Position: refs/heads/master@{#460741}
Committed: https://chromium.googlesource.com/chromium/src/+/286a79cc3d9b45f96709aa6b919ef94060b4fd92

[xhwang, 2017-03-23 23:51:38]

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

[commit-bot: I haz the power, 2017-03-23 23:52:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-03-23 23:53:58]

Description was changed from

==========
media: Move Framework signature into a bundle

Currently the Framework signature is in the version folder directly
which does not match Mac bundle format. Instead, create a new bundle
for the signature ("Widevine Signature.bundle") which is located
beside "Google Chrome Framework.framework".

Also update CdmHostFilePath to match the new folder structure.

BUG=704762
==========

to

==========
media: Move Widevine signature of Framework into a bundle

Currently the Framework signature is in the version folder directly
which does not match Mac bundle format. Instead, create a new bundle
for the signature ("Widevine Signature.bundle") which is located
beside "Google Chrome Framework.framework".

Also update CdmHostFilePath to match the new folder structure.

BUG=704762
==========

[Greg K, 2017-03-24 00:04:40]

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

[Greg K, 2017-03-24 00:04:42]

Thanks for doing this. A few comments, and then we should wait for Robert to
review this. Can you paste the Info.plist into the bug? I'm not sure what
create_bundle does for a default Info.plist. Thanks.

https://codereview.chromium.org/2776613002/diff/1/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/2776613002/diff/1/chrome/BUILD.gn#newcode664
chrome/BUILD.gn:664: sources += [ "$root_out_dir/Widevine Signature.bundle" ]
Can we call this Widevine Resources.bundle instead?

https://codereview.chromium.org/2776613002/diff/1/chrome/common/media/cdm_hos...
File chrome/common/media/cdm_host_file_path.cc (right):

https://codereview.chromium.org/2776613002/diff/1/chrome/common/media/cdm_hos...
chrome/common/media/cdm_host_file_path.cc:89: static const
base::FilePath::CharType kWidevineSignaturePath[] =
Note that you could use the CFBundle APIs to fish out the location of the .sig
file but, I suspect this is fine since there is only one file in the bundle.

[xhwang, 2017-03-24 00:23:09]

On 2017/03/24 00:04:42, Greg K wrote:
> Thanks for doing this. A few comments, and then we should wait for Robert to
> review this. Can you paste the Info.plist into the bug? I'm not sure what
> create_bundle does for a default Info.plist. Thanks.
> 
> https://codereview.chromium.org/2776613002/diff/1/chrome/BUILD.gn
> File chrome/BUILD.gn (right):
> 
> https://codereview.chromium.org/2776613002/diff/1/chrome/BUILD.gn#newcode664
> chrome/BUILD.gn:664: sources += [ "$root_out_dir/Widevine Signature.bundle" ]
> Can we call this Widevine Resources.bundle instead?
> 
>
https://codereview.chromium.org/2776613002/diff/1/chrome/common/media/cdm_hos...
> File chrome/common/media/cdm_host_file_path.cc (right):
> 
>
https://codereview.chromium.org/2776613002/diff/1/chrome/common/media/cdm_hos...
> chrome/common/media/cdm_host_file_path.cc:89: static const
> base::FilePath::CharType kWidevineSignaturePath[] =
> Note that you could use the CFBundle APIs to fish out the location of the .sig
> file but, I suspect this is fine since there is only one file in the bundle.

Comments only.

I don't see Info.plist anywhere. IIUIC, I have to generate it manually, as shown
in this example:

https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...

So, do we need it? Is it required by the bundle format?

[xhwang, 2017-03-24 00:23:27]

https://codereview.chromium.org/2776613002/diff/1/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/2776613002/diff/1/chrome/BUILD.gn#newcode664
chrome/BUILD.gn:664: sources += [ "$root_out_dir/Widevine Signature.bundle" ]
On 2017/03/24 00:04:41, Greg K wrote:
> Can we call this Widevine Resources.bundle instead?

I am totally flexible on the naming so I'll wait for Robert's comment before I
make the final change.

https://codereview.chromium.org/2776613002/diff/1/chrome/common/media/cdm_hos...
File chrome/common/media/cdm_host_file_path.cc (right):

https://codereview.chromium.org/2776613002/diff/1/chrome/common/media/cdm_hos...
chrome/common/media/cdm_host_file_path.cc:89: static const
base::FilePath::CharType kWidevineSignaturePath[] =
On 2017/03/24 00:04:41, Greg K wrote:
> Note that you could use the CFBundle APIs to fish out the location of the .sig
> file but, I suspect this is fine since there is only one file in the bundle.

Acknowledged. I'll keep this as is for now unless Robert disagrees.

[Greg K, 2017-03-24 00:28:37]

On 2017/03/24 00:23:09, xhwang_slow wrote:
> On 2017/03/24 00:04:42, Greg K wrote:
> > Thanks for doing this. A few comments, and then we should wait for Robert to
> > review this. Can you paste the Info.plist into the bug? I'm not sure what
> > create_bundle does for a default Info.plist. Thanks.
> > 
> > https://codereview.chromium.org/2776613002/diff/1/chrome/BUILD.gn
> > File chrome/BUILD.gn (right):
> > 
> > https://codereview.chromium.org/2776613002/diff/1/chrome/BUILD.gn#newcode664
> > chrome/BUILD.gn:664: sources += [ "$root_out_dir/Widevine Signature.bundle"
]
> > Can we call this Widevine Resources.bundle instead?
> > 
> >
>
https://codereview.chromium.org/2776613002/diff/1/chrome/common/media/cdm_hos...
> > File chrome/common/media/cdm_host_file_path.cc (right):
> > 
> >
>
https://codereview.chromium.org/2776613002/diff/1/chrome/common/media/cdm_hos...
> > chrome/common/media/cdm_host_file_path.cc:89: static const
> > base::FilePath::CharType kWidevineSignaturePath[] =
> > Note that you could use the CFBundle APIs to fish out the location of the
.sig
> > file but, I suspect this is fine since there is only one file in the bundle.
> 
> Comments only.
> 
> I don't see Info.plist anywhere. IIUIC, I have to generate it manually, as
shown
> in this example:
> 
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
> 
> So, do we need it? Is it required by the bundle format?

Yes, the Info.plist is one of the most important parts of a bundle.

It should have a:

CFBundleDisplayName -> "Widevine Resources"
CFBundleIdentifier -> com.google.Chrome.widevine-resources (or something like
that)
CFBundleName -> "Widevine Resources"

And hopefully the info.plist generation can take care of the build version and
everything?

- Greg

[commit-bot: I haz the power, 2017-03-24 00:55:56]

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

[commit-bot: I haz the power, 2017-03-24 00:55:56]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-03-24 05:54:03]

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

[commit-bot: I haz the power, 2017-03-24 05:54:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-03-24 05:55:05]

Greg, I updated the CL to include the Info.plist file. Here's what I get after
building everything:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN"
"http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
        <key>BuildMachineOSBuild</key>
        <string>16D32</string>
        <key>CFBundleDevelopmentRegion</key>
        <string>en</string>
        <key>CFBundleDisplayName</key>
        <string>Widevine Resources</string>
        <key>CFBundleExecutable</key>
        <string>Widevine Resources</string>
        <key>CFBundleIdentifier</key>
        <string>com.google.Chrome.widevine_resources</string>
        <key>CFBundleInfoDictionaryVersion</key>
        <string>6.0</string>
        <key>CFBundleName</key>
        <string>Widevine Resources</string>
        <key>CFBundlePackageType</key>
        <string>BNDL</string>
        <key>CFBundleSignature</key>
        <string>????</string>
        <key>DTCompiler</key>
        <string>com.apple.compilers.llvm.clang.1_0</string>
        <key>DTSDKBuild</key>
        <string>10.10</string>
        <key>DTSDKName</key>
        <string>macosx10.10</string>
        <key>DTXcode</key>
        <string>0511</string>
        <key>DTXcodeBuild</key>
        <string>5B1008</string>
</dict>
</plist>

[xhwang, 2017-03-24 06:29:35]

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

[xhwang, 2017-03-24 06:29:36]

rsesek / kerrnel: PTAL!
hmchen / jrummell: FYI

[xhwang, 2017-03-24 06:29:47]

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

[commit-bot: I haz the power, 2017-03-24 06:30:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 08:00:09]

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

[commit-bot: I haz the power, 2017-03-24 08:00:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xhwang, 2017-03-24 16:27:48]

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

[commit-bot: I haz the power, 2017-03-24 16:28:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 17:24:10]

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

[commit-bot: I haz the power, 2017-03-24 17:24:10]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-03-29 23:42:07]

LGTM. Thanks!

[xhwang, 2017-03-29 23:56:52]

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

[xhwang, 2017-03-29 23:56:52]

jam@: Please OWNERS review chrome/* 

We may need to merge this back to M58, and your prompt review will be highly
appreciated!

[jam, 2017-03-30 00:26:38]

jam@chromium.org changed reviewers:
+ thakis@chromium.org

[jam, 2017-03-30 00:26:39]

redirecting to Nico

[Nico, 2017-03-30 01:19:07]

https://codereview.chromium.org/2776613002/diff/60001/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/2776613002/diff/60001/chrome/BUILD.gn#newcode664
chrome/BUILD.gn:664: sources += [ "$root_out_dir/Widevine Resources.bundle" ]
Does having a directory in sources work? I'd expect that this makes something
depend on the directory's timestamp, and if files in subdirectories of that
directory change, the top-level timestamp won't be updated and incremental
builds won't work correctly. Is there any way to list all the input files here?

[Robert Sesek, 2017-03-30 03:33:32]

https://codereview.chromium.org/2776613002/diff/60001/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/2776613002/diff/60001/chrome/BUILD.gn#newcode664
chrome/BUILD.gn:664: sources += [ "$root_out_dir/Widevine Resources.bundle" ]
On 2017/03/30 01:19:07, Nico wrote:
> Does having a directory in sources work? I'd expect that this makes something
> depend on the directory's timestamp, and if files in subdirectories of that
> directory change, the top-level timestamp won't be updated and incremental
> builds won't work correctly. Is there any way to list all the input files
here?

Yes, this works because of the deps as well. See lines 638 above.

[Nico, 2017-03-30 11:47:16]

The CQ bit was checked by thakis@chromium.org

[Nico, 2017-03-30 11:47:17]

lgtm

[commit-bot: I haz the power, 2017-03-30 11:47:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 13:30:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490874436623780,
"parent_rev": "757f5dc9a5cd6c3679b809701d09ba6aa6ff812b", "commit_rev":
"286a79cc3d9b45f96709aa6b919ef94060b4fd92"}

[commit-bot: I haz the power, 2017-03-30 13:30:51]

Description was changed from

==========
media: Move Widevine signature of Framework into a bundle

Currently the Framework signature is in the version folder directly
which does not match Mac bundle format. Instead, create a new bundle
for the signature ("Widevine Signature.bundle") which is located
beside "Google Chrome Framework.framework".

Also update CdmHostFilePath to match the new folder structure.

BUG=704762
==========

to

==========
media: Move Widevine signature of Framework into a bundle

Currently the Framework signature is in the version folder directly
which does not match Mac bundle format. Instead, create a new bundle
for the signature ("Widevine Signature.bundle") which is located
beside "Google Chrome Framework.framework".

Also update CdmHostFilePath to match the new folder structure.

BUG=704762

Review-Url: https://codereview.chromium.org/2776613002
Cr-Commit-Position: refs/heads/master@{#460741}
Committed:
https://chromium.googlesource.com/chromium/src/+/286a79cc3d9b45f96709aa6b919e...
==========

[commit-bot: I haz the power, 2017-03-30 13:30:52]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/286a79cc3d9b45f96709aa6b919e...