Add VerifyHostFiles()

Add VerifyHostFiles()

This API allows the host of the CDM to pass in a list of file
descriptors of files (and corresponding signature files) so that the CDM
can verify them.

This new API is added in a different file since this API will be
dynamically found (e.g. using GetFunctionPointerFromNativeLibrary()).
Other existing APIs in media/cdm/api/content_decryption_module.h are
statically called by the CDM adapter.

BUG=658036
TEST=Tested with prototype CL https://codereview.chromium.org/2582463003
R=ddorwin@chromium.org, hmchen@chromium.org, jrummell@chromium.org

Committed: https://chromium.googlesource.com/chromium/cdm/+/379a18058fee03b429bd5b4a330b78add56efec4

[xhwang, 2017-01-12 20:07:04]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, hmchen@chromium.org, jrummell@chromium.org

[xhwang, 2017-01-12 20:07:05]

This is the cdm.h API change. PTAL

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
File content_decryption_module.h (right):

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module.h:8: #include "content_decryption_module_export.h"
We had the discussion about include path issue in the other code review:
https://chromiumcodereview.appspot.com/2446413003/

Here's a copy of that discussion for the reference:

----------------

* ddorwin:

Not LGTM due to path issue. This assumes the Chrome code structure, which may
not apply for other browsers.

* xhwang:

Can we just have this here:

#include "content_decryption_module_export.h"

I *think* the current folder will always be searched (I could be wrong). The
risk is that we may have another content_decryption_module_export.h somewhere in
the include path, but I  *think* the "current folder" is always searched first:

https://gcc.gnu.org/onlinedocs/cpp/Search-Path.html
https://msdn.microsoft.com/en-us/library/36k2cdd4.aspx

----------------

[xhwang, 2017-01-12 20:15:56]

jrummell / hmchen: See my reply to your comments on these files in
https://chromiumcodereview.appspot.com/2582463003/

[ddorwin, 2017-01-13 08:27:51]

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
File content_decryption_module_ext.h (right):

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:15: typedef unsigned int uint32_t;
Is this still required even with the latest VS version we use? Maybe there are
other users that support older compilers.

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:26: typedef int CdmPlatformFile;
...Id? ...Handle? As is, it would appear to be an object. I guess that's okay,
but clarity seems better. WDYT?

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:28: #endif  // defined(WIN32)
Should we #error?

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:36: : file_path(file_path), file(file),
sig_file(sig_file) {}
The members and parameters have the same exact name. Does that compile?

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:48: // |cdm_host_files|. The CDM should return
as soon as possible and process the
s/should/MUST/ here and below.

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:50: // processing is finished.
...this one-time processing...

To make it clear that they need to be closed ASAP.

[xhwang, 2017-01-13 18:25:58]

Patchset #3 (id:40001) has been deleted

[xhwang, 2017-01-13 18:33:35]

Patchset #3 (id:60001) has been deleted

[xhwang, 2017-01-13 18:34:07]

Thanks for the comments. PTAL again!

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
File content_decryption_module_ext.h (right):

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:15: typedef unsigned int uint32_t;
On 2017/01/13 08:27:51, ddorwin wrote:
> Is this still required even with the latest VS version we use? 

I am not sure.

> Maybe there are other users that support older compilers.

That's actually possible. Acknowledged.

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:26: typedef int CdmPlatformFile;
On 2017/01/13 08:27:51, ddorwin wrote:
> ...Id? ...Handle? As is, it would appear to be an object. I guess that's okay,
> but clarity seems better. WDYT?

PlatformFile comes from base/file/file.h

https://cs.chromium.org/chromium/src/base/files/file.h?rcl=0&l=32

I guess the whole point is that user should not care whether this is an object
or not. So I'd like t err on the consistency side.

Also, I moved this block into the cdm:: namespace to avoid name collisions.

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:28: #endif  // defined(WIN32)
On 2017/01/13 08:27:51, ddorwin wrote:
> Should we #error?

Done.

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:36: : file_path(file_path), file(file),
sig_file(sig_file) {}
On 2017/01/13 08:27:51, ddorwin wrote:
> The members and parameters have the same exact name. Does that compile?

Yes. Please see related discussion here:
http://stackoverflow.com/questions/6185020/initializing-member-variables-usin...

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:48: // |cdm_host_files|. The CDM should return
as soon as possible and process the
On 2017/01/13 08:27:51, ddorwin wrote:
> s/should/MUST/ here and below.

Done.

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:50: // processing is finished.
On 2017/01/13 08:27:51, ddorwin wrote:
> ...this one-time processing...
> 
> To make it clear that they need to be closed ASAP.

Done.

[ddorwin, 2017-01-13 18:47:38]

The code LGTM.

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
File content_decryption_module_ext.h (right):

https://codereview.chromium.org/2631463003/diff/20001/content_decryption_modu...
content_decryption_module_ext.h:26: typedef int CdmPlatformFile;
On 2017/01/13 18:34:06, xhwang wrote:
> On 2017/01/13 08:27:51, ddorwin wrote:
> > ...Id? ...Handle? As is, it would appear to be an object. I guess that's
okay,
> > but clarity seems better. WDYT?
> 
> PlatformFile comes from base/file/file.h
> 
> https://cs.chromium.org/chromium/src/base/files/file.h?rcl=0&l=32
> 
> I guess the whole point is that user should not care whether this is an object
> or not. So I'd like t err on the consistency side.
> 
> Also, I moved this block into the cdm:: namespace to avoid name collisions.

Acknowledged.

[xhwang, 2017-01-13 18:51:46]

hmchen and jrummell: PTAL!

[xhwang, 2017-01-13 18:52:03]

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

[xhwang, 2017-01-13 18:52:04]

tinskip: FYI

[Haoming Chen, 2017-01-13 19:13:58]

lgtm

[jrummell, 2017-01-13 21:00:17]

lgtm

https://codereview.chromium.org/2631463003/diff/80001/content_decryption_modu...
File content_decryption_module_ext.h (right):

https://codereview.chromium.org/2631463003/diff/80001/content_decryption_modu...
content_decryption_module_ext.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
nit: It's now 2017.

[xhwang, 2017-01-13 21:13:51]

https://codereview.chromium.org/2631463003/diff/80001/content_decryption_modu...
File content_decryption_module_ext.h (right):

https://codereview.chromium.org/2631463003/diff/80001/content_decryption_modu...
content_decryption_module_ext.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
On 2017/01/13 21:00:17, jrummell wrote:
> nit: It's now 2017.

Done.

[xhwang, 2017-01-13 21:14:12]

Description was changed from

==========
Add VerifyHostFiles()

This API allows the host of the CDM to pass in a list of file
descriptors of files (and corresponding signature files) so that the CDM
can verify them.

This new API is added in a different file since this API will be
dynamically found (e.g. using GetFunctionPointerFromNativeLibrary()).
Other existing APIs in media/cdm/api/content_decryption_module.h are
statically called by the CDM adapter.

BUG=658036
TEST=Tested with prototype CL https://codereview.chromium.org/2582463003
==========

to

==========
Add VerifyHostFiles()

This API allows the host of the CDM to pass in a list of file
descriptors of files (and corresponding signature files) so that the CDM
can verify them.

This new API is added in a different file since this API will be
dynamically found (e.g. using GetFunctionPointerFromNativeLibrary()).
Other existing APIs in media/cdm/api/content_decryption_module.h are
statically called by the CDM adapter.

BUG=658036
TEST=Tested with prototype CL https://codereview.chromium.org/2582463003
R=ddorwin@chromium.org, hmchen@chromium.org, jrummell@chromium.org

Committed:
https://chromium.googlesource.com/chromium/cdm/+/379a18058fee03b429bd5b4a330b...
==========

[xhwang, 2017-01-13 21:14:13]

Committed patchset #4 (id:100001) manually as
379a18058fee03b429bd5b4a330b78add56efec4 (presubmit successful).