Plumb |use_secure_codecs| through to BrowserCdmFactoryAndroid.

Plumb |use_secure_codecs| through to BrowserCdmFactoryAndroid.

This completes the path from the secure surface preference, through requestMediaKeySystemAccess() and finally to CDM creation on Android where secure codecs are enabled. With this change, configs requiring secure codes are rejected without the preference and the CDM is only configures to use hardware-secure codecs if the config requires it.

There is a separate bug (http://crbug.com/478185) for implementing similar plumbing for the use of secure surfaces.

BUG=467779
Committed: https://crrev.com/d7411976f6467bf4519f66f32b4459717e7352d7
Cr-Commit-Position: refs/heads/master@{#330040}

[sandersd (OOO until July 31), 2015-05-13 00:44:09]

sandersd@chromium.org changed reviewers:
+ ddorwin@chromium.org, jrummell@chromium.org

[sandersd (OOO until July 31), 2015-05-13 00:44:10]

Two open questions, both with working solutions in the first patch set:

  (1) Should CdmConfig be passed all the way to the actual CdmFactories? Here I
have passed only the relevant fields along (from RenderCdmFactory), because the
DCHECKS() make sense there.

  (2) Where should InitializeCdmParameters be defined? cdm_messages.h is
convenient but the result looks hacky.

[sandersd (OOO until July 31), 2015-05-13 00:44:22]

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

[commit-bot: I haz the power, 2015-05-13 00:44:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131753003/1

[commit-bot: I haz the power, 2015-05-13 01:15:04]

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

[commit-bot: I haz the power, 2015-05-13 01:15:05]

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

[sandersd (OOO until July 31), 2015-05-13 02:04:42]

Patchset #2 (id:20001) has been deleted

[ddorwin, 2015-05-13 06:03:21]

LG overall. Just minor stuff.

+xhwang FYI on BrowserCdm changes.

https://codereview.chromium.org/1131753003/diff/40001/chromecast/browser/medi...
File chromecast/browser/media/cast_browser_cdm_factory.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/chromecast/browser/medi...
chromecast/browser/media/cast_browser_cdm_factory.cc:26:
DCHECK(!use_secure_codecs) << "Chromecast does not use |use_secure_codecs|";
I don't know what it would take to make this happen, but it is probably more
appropriate to pass true and say "Chromecast always uses secure codecs."

https://codereview.chromium.org/1131753003/diff/40001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.h:114: InitializeCdmParameters&
parameters);
const

https://codereview.chromium.org/1131753003/diff/40001/content/common/media/cd...
File content/common/media/cdm_messages.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/common/media/cd...
content/common/media/cdm_messages.h:24: struct InitializeCdmParameters {
Assuming this pattern was copied from somewhere else, this seems fine. This type
is only used for the IPC, right?

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.h:28: struct CdmConfig;
not used?

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/proxy_media_keys.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/proxy_media_keys.h:24: struct CdmConfig;
not used

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.cc:78:
DCHECK(!cdm_config.use_secure_codecs);
Since we just pass around a type now instead of parameters, we _could_ ifdef
individual members. I don't know that it's any better unless we end up with lots
of members that are meaningless on various platforms. However, it would avoid
confusing statements like this one.

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.cc:85:
DCHECK(cdm_config.allow_distinctive_identifier);
These DCHECKs are consistent with the KS selector code IIRC.

It seems that we should just pass them down with the struct, but then I remember
that we can't enforce false, so this probably does make sense (until such time
as we need to support false).

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.h:50: const media::CdmConfig&
cdm_config,
nit: The config seems more related to the key_system than the origin. What was
your reasoning? Consistency for the first two parameters (because the third type
may change)?

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/renderer_cdm_manager.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:23: struct CdmConfig;
ditto

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:17: #include
"media/base/cdm_config.h"
unused

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:31: struct CdmConfig;
unused

https://codereview.chromium.org/1131753003/diff/40001/media/base/browser_cdm_...
File media/base/browser_cdm_factory.h (right):

https://codereview.chromium.org/1131753003/diff/40001/media/base/browser_cdm_...
media/base/browser_cdm_factory.h:42: // hardware-secure codecs (for platforms
that support it).
Maybe we should rename the bool to include hardware_ or hw_. That would, for
example, address the confusion in the ppapi path.

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h
File media/base/cdm_config.h (right):

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h...
media/base/cdm_config.h:12: // The runtime configuration of a CDM as computed by
...configuration for new CDM instances...

I was going to just say "...a CDM instance...", but that wouldn't come from
rMKSA. Instead, rMKSA returns an object for creating CDM instances.

OTOH, if we take the caller impl details out of this (see below), then we can
have a simpler comment.

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h...
media/base/cdm_config.h:16: // Access to a distinctive identifier was requested
or required, and the
This seems like impl details of the caller. Should we just say that it is
allowed?

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h...
media/base/cdm_config.h:21: // the session.
session?

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h...
media/base/cdm_config.h:25: // on Android, it should always be false on other
platforms.
We could ifdef it.

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h...
media/base/cdm_config.h:26: bool use_secure_codecs = false;
ditto on hardware_/hw_. At least include this in the comment.

https://codereview.chromium.org/1131753003/diff/40001/media/blink/cdm_session...
File media/blink/cdm_session_adapter.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/media/blink/cdm_session...
media/blink/cdm_session_adapter.cc:41: key_system, security_origin, cdm_config,
One great thing about the config is that the bools cannot be accidentally
swapped anymore.

https://codereview.chromium.org/1131753003/diff/40001/media/blink/webcontentd...
File media/blink/webcontentdecryptionmoduleaccess_impl.h (right):

https://codereview.chromium.org/1131753003/diff/40001/media/blink/webcontentd...
media/blink/webcontentdecryptionmoduleaccess_impl.h:44: blink::WebString
key_system_;
nit: I prefer members to be const when possible to indicate they do not change.
This seems especially relevant for the two configurations.

https://codereview.chromium.org/1131753003/diff/40001/media/blink/webcontentd...
media/blink/webcontentdecryptionmoduleaccess_impl.h:47: CdmConfig cdm_config_;
configuration and config. Hmmm. They match the types. I guess Config is fine and
Configuration comes from the spec.

https://codereview.chromium.org/1131753003/diff/40001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:70: // TODO(sandersd): Trigger permissions check
here and use it to determine
What exactly needs to be done here? Do we need to move the permission request
from GKRInternal to here (earlier)? Perhaps update the comment.

[gunsch, 2015-05-13 06:52:37]

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

[gunsch, 2015-05-13 06:52:38]

https://codereview.chromium.org/1131753003/diff/40001/chromecast/browser/medi...
File chromecast/browser/media/cast_browser_cdm_factory.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/chromecast/browser/medi...
chromecast/browser/media/cast_browser_cdm_factory.cc:26:
DCHECK(!use_secure_codecs) << "Chromecast does not use |use_secure_codecs|";
On 2015/05/13 06:03:20, ddorwin wrote:
> I don't know what it would take to make this happen, but it is probably more
> appropriate to pass true and say "Chromecast always uses secure codecs."

That would typically be more accurate, though not in all cases.

For example, we have a BrowserCdm-based ClearKey implementation, which is not a
secure codec.

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.cc:85:
DCHECK(cdm_config.allow_distinctive_identifier);
On 2015/05/13 06:03:20, ddorwin wrote:
> These DCHECKs are consistent with the KS selector code IIRC.
> 
> It seems that we should just pass them down with the struct, but then I
remember
> that we can't enforce false, so this probably does make sense (until such time
> as we need to support false).

One odd edge case we ran into on Chromecast today: we comment out the
CanUseAesDecryptor block above downstream (Cast but non-Android) so that we can
use our browser-side clearkey implementation. At that point, this check ends up
getting hit.

[sandersd (OOO until July 31), 2015-05-13 18:17:13]

sandersd@chromium.org changed reviewers:
+ bbudge@chromium.org, dcheng@chromium.org

[sandersd (OOO until July 31), 2015-05-13 18:17:14]

dcheng@chromium.org: Please review changes in
content/common/media/cdm_messages.h

bbudge@chromium.org: Please review changes in content/renderer/pepper/

https://codereview.chromium.org/1131753003/diff/40001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.h:114: InitializeCdmParameters&
parameters);
On 2015/05/13 06:03:20, ddorwin wrote:
> const

Done.

https://codereview.chromium.org/1131753003/diff/40001/content/common/media/cd...
File content/common/media/cdm_messages.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/common/media/cd...
content/common/media/cdm_messages.h:24: struct InitializeCdmParameters {
On 2015/05/13 06:03:20, ddorwin wrote:
> Assuming this pattern was copied from somewhere else, this seems fine. This
type
> is only used for the IPC, right?

It was not copied from anywhere else, but it is only used for the IPC.

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.h:28: struct CdmConfig;
On 2015/05/13 06:03:20, ddorwin wrote:
> not used?

Done. (Detritus from the first attempt that passed CdmConfig all the way down.)

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.h:50: const media::CdmConfig&
cdm_config,
On 2015/05/13 06:03:20, ddorwin wrote:
> nit: The config seems more related to the key_system than the origin. What was
> your reasoning? Consistency for the first two parameters (because the third
type
> may change)?

If anything, |security_origin| would make sense as a field in CdmConfig. I based
the ordering from most-to-least important/descriptive.

[sandersd (OOO until July 31), 2015-05-14 00:06:58]

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/proxy_media_keys.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/proxy_media_keys.h:24: struct CdmConfig;
On 2015/05/13 06:03:20, ddorwin wrote:
> not used

Done.

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.cc:78:
DCHECK(!cdm_config.use_secure_codecs);
On 2015/05/13 06:03:20, ddorwin wrote:
> Since we just pass around a type now instead of parameters, we _could_ ifdef
> individual members. I don't know that it's any better unless we end up with
lots
> of members that are meaningless on various platforms. However, it would avoid
> confusing statements like this one.

Acknowledged.

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/renderer_cdm_manager.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:23: struct CdmConfig;
On 2015/05/13 06:03:20, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:17: #include
"media/base/cdm_config.h"
On 2015/05/13 06:03:20, ddorwin wrote:
> unused

Done.

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:31: struct CdmConfig;
On 2015/05/13 06:03:20, ddorwin wrote:
> unused

Done.

https://codereview.chromium.org/1131753003/diff/40001/media/base/browser_cdm_...
File media/base/browser_cdm_factory.h (right):

https://codereview.chromium.org/1131753003/diff/40001/media/base/browser_cdm_...
media/base/browser_cdm_factory.h:42: // hardware-secure codecs (for platforms
that support it).
On 2015/05/13 06:03:21, ddorwin wrote:
> Maybe we should rename the bool to include hardware_ or hw_. That would, for
> example, address the confusion in the ppapi path.

Done.

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h
File media/base/cdm_config.h (right):

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h...
media/base/cdm_config.h:16: // Access to a distinctive identifier was requested
or required, and the
On 2015/05/13 06:03:21, ddorwin wrote:
> This seems like impl details of the caller. Should we just say that it is
> allowed?

Done.

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h...
media/base/cdm_config.h:21: // the session.
On 2015/05/13 06:03:21, ddorwin wrote:
> session?

Done.

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h...
media/base/cdm_config.h:25: // on Android, it should always be false on other
platforms.
On 2015/05/13 06:03:21, ddorwin wrote:
> We could ifdef it.

We could, but then KeySystemConfigChooser needs to be platform-aware. Either way
is fine, but this avoids a pair of ifdefs.

https://codereview.chromium.org/1131753003/diff/40001/media/base/cdm_config.h...
media/base/cdm_config.h:26: bool use_secure_codecs = false;
On 2015/05/13 06:03:21, ddorwin wrote:
> ditto on hardware_/hw_. At least include this in the comment.

Done.

https://codereview.chromium.org/1131753003/diff/40001/media/blink/cdm_session...
File media/blink/cdm_session_adapter.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/media/blink/cdm_session...
media/blink/cdm_session_adapter.cc:41: key_system, security_origin, cdm_config,
On 2015/05/13 06:03:21, ddorwin wrote:
> One great thing about the config is that the bools cannot be accidentally
> swapped anymore.

Acknowledged.

https://codereview.chromium.org/1131753003/diff/40001/media/blink/webcontentd...
File media/blink/webcontentdecryptionmoduleaccess_impl.h (right):

https://codereview.chromium.org/1131753003/diff/40001/media/blink/webcontentd...
media/blink/webcontentdecryptionmoduleaccess_impl.h:44: blink::WebString
key_system_;
On 2015/05/13 06:03:21, ddorwin wrote:
> nit: I prefer members to be const when possible to indicate they do not
change.
> This seems especially relevant for the two configurations.

Done.

https://codereview.chromium.org/1131753003/diff/40001/media/blink/webcontentd...
media/blink/webcontentdecryptionmoduleaccess_impl.h:47: CdmConfig cdm_config_;
On 2015/05/13 06:03:21, ddorwin wrote:
> configuration and config. Hmmm. They match the types. I guess Config is fine
and
> Configuration comes from the spec.

Acknowledged.

https://codereview.chromium.org/1131753003/diff/40001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:70: // TODO(sandersd): Trigger permissions check
here and use it to determine
On 2015/05/13 06:03:21, ddorwin wrote:
> What exactly needs to be done here? Do we need to move the permission request
> from GKRInternal to here (earlier)? Perhaps update the comment.

Yes, although there are synchronization issues we need to solve to do that. The
truth is that we will never do it, we'll just delete prefixed instead.

[ddorwin, 2015-05-14 00:17:31]

Thanks. LGTM.

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.cc:85:
DCHECK(cdm_config.allow_distinctive_identifier);
On 2015/05/13 06:52:37, gunsch wrote:
> On 2015/05/13 06:03:20, ddorwin wrote:
> > These DCHECKs are consistent with the KS selector code IIRC.
> > 
> > It seems that we should just pass them down with the struct, but then I
> remember
> > that we can't enforce false, so this probably does make sense (until such
time
> > as we need to support false).
> 
> One odd edge case we ran into on Chromecast today: we comment out the
> CanUseAesDecryptor block above downstream (Cast but non-Android) so that we
can
> use our browser-side clearkey implementation. At that point, this check ends
up
> getting hit.

Hmm. Let's discuss. There is definitely an assumption (unrelated to this CL)
about Clear Key.
Rather than commenting out the CanUseAesDecryptor() call, you should change its
definition, which is closer to that assumption.

[sandersd (OOO until July 31), 2015-05-14 00:20:44]

sandersd@chromium.org changed reviewers:
- bbudge@chromium.org

[gunsch, 2015-05-14 01:35:52]

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/1131753003/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.cc:85:
DCHECK(cdm_config.allow_distinctive_identifier);
On 2015/05/14 00:17:31, ddorwin wrote:
> On 2015/05/13 06:52:37, gunsch wrote:
> > On 2015/05/13 06:03:20, ddorwin wrote:
> > > These DCHECKs are consistent with the KS selector code IIRC.
> > > 
> > > It seems that we should just pass them down with the struct, but then I
> > remember
> > > that we can't enforce false, so this probably does make sense (until such
> time
> > > as we need to support false).
> > 
> > One odd edge case we ran into on Chromecast today: we comment out the
> > CanUseAesDecryptor block above downstream (Cast but non-Android) so that we
> can
> > use our browser-side clearkey implementation. At that point, this check ends
> up
> > getting hit.
> 
> Hmm. Let's discuss. There is definitely an assumption (unrelated to this CL)
> about Clear Key.
> Rather than commenting out the CanUseAesDecryptor() call, you should change
its
> definition, which is closer to that assumption.

Agreed, we realize commenting it out isn't valid at all but we don't need the
"normal" path (on Cast Linux). What are your thoughts on a way to approach it?
My initial thinking (for browser-CDM-based embedders, incl. Android) would be to
simply guard the use of renderer-side AesDecryptor by
!defined(ENABLE_BROWSER_CDMS) and reuse the decryption code both
browser/renderer-side. WDYT?

[gunsch-google, 2015-05-14 04:46:05]

gunsch@google.com changed reviewers:
+ gunsch@google.com

[gunsch-google, 2015-05-14 04:46:06]

lgtm for chromecast/

[gunsch, 2015-05-14 17:04:05]

actual lgtm for chromecast

[sandersd (OOO until July 31), 2015-05-14 18:14:34]

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

[commit-bot: I haz the power, 2015-05-14 18:15:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131753003/60001

[commit-bot: I haz the power, 2015-05-14 19:59:57]

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

[commit-bot: I haz the power, 2015-05-14 19:59:59]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2015-05-14 20:10:04]

sandersd@chromium.org changed reviewers:
+ nasko@chromium.org
- dcheng@chromium.org

[sandersd (OOO until July 31), 2015-05-14 20:10:23]

nasko@: Please review changes in content/common/media/cdm_messages.h.

[nasko, 2015-05-14 21:53:19]

https://codereview.chromium.org/1131753003/diff/60001/content/common/media/cd...
File content/common/media/cdm_messages.h (right):

https://codereview.chromium.org/1131753003/diff/60001/content/common/media/cd...
content/common/media/cdm_messages.h:24: struct InitializeCdmParameters {
structs should be defined using
IPC_STRUCT_BEGIN/IPC_STRUCT_MEMBER/IPC_STRUCT_END if they are only used within
the message file. Using IPC_STRUCT_TRAITS* is applicable when the struct is
declared in an external header file, such as media::CdmKeyInformation for
example.

[sandersd (OOO until July 31), 2015-05-14 22:05:17]

https://codereview.chromium.org/1131753003/diff/60001/content/common/media/cd...
File content/common/media/cdm_messages.h (right):

https://codereview.chromium.org/1131753003/diff/60001/content/common/media/cd...
content/common/media/cdm_messages.h:24: struct InitializeCdmParameters {
On 2015/05/14 21:53:18, nasko wrote:
> structs should be defined using
> IPC_STRUCT_BEGIN/IPC_STRUCT_MEMBER/IPC_STRUCT_END if they are only used within
> the message file. Using IPC_STRUCT_TRAITS* is applicable when the struct is
> declared in an external header file, such as media::CdmKeyInformation for
> example.

Oh excellent, that looks much better.

[nasko, 2015-05-14 22:13:58]

IPC LGTM with a nit.

https://codereview.chromium.org/1131753003/diff/80001/content/common/media/cd...
File content/common/media/cdm_messages.h (right):

https://codereview.chromium.org/1131753003/diff/80001/content/common/media/cd...
content/common/media/cdm_messages.h:23:
IPC_STRUCT_BEGIN(CdmHostMsg_InitializeCdm_Params)
nit: Move this to be in the same section of the file as other structs.

[sandersd (OOO until July 31), 2015-05-14 22:41:32]

https://codereview.chromium.org/1131753003/diff/80001/content/common/media/cd...
File content/common/media/cdm_messages.h (right):

https://codereview.chromium.org/1131753003/diff/80001/content/common/media/cd...
content/common/media/cdm_messages.h:23:
IPC_STRUCT_BEGIN(CdmHostMsg_InitializeCdm_Params)
On 2015/05/14 22:13:58, nasko wrote:
> nit: Move this to be in the same section of the file as other structs.

Done.

[sandersd (OOO until July 31), 2015-05-14 22:41:39]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2015-05-14 22:41:40]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org,
ddorwin@chromium.org, gunsch@google.com, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1131753003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-05-14 22:42:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131753003/100001

[commit-bot: I haz the power, 2015-05-14 23:15:45]

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

[commit-bot: I haz the power, 2015-05-14 23:15:46]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[sandersd (OOO until July 31), 2015-05-14 23:20:22]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2015-05-14 23:20:23]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org,
ddorwin@chromium.org, nasko@chromium.org, gunsch@google.com
Link to the patchset: https://codereview.chromium.org/1131753003/#ps120001
(title: "Android build.")

[commit-bot: I haz the power, 2015-05-14 23:21:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131753003/120001

[commit-bot: I haz the power, 2015-05-14 23:53:11]

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

[commit-bot: I haz the power, 2015-05-14 23:53:13]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[sandersd (OOO until July 31), 2015-05-14 23:57:30]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2015-05-14 23:57:30]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org,
ddorwin@chromium.org, nasko@chromium.org, gunsch@google.com
Link to the patchset: https://codereview.chromium.org/1131753003/#ps140001
(title: ":-(")

[commit-bot: I haz the power, 2015-05-14 23:58:07]

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

[commit-bot: I haz the power, 2015-05-15 00:29:46]

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

[commit-bot: I haz the power, 2015-05-15 00:29:47]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[sandersd (OOO until July 31), 2015-05-15 00:31:04]

Patchset #6 (id:120001) has been deleted

[sandersd (OOO until July 31), 2015-05-15 00:31:14]

Patchset #6 (id:140001) has been deleted

[sandersd (OOO until July 31), 2015-05-15 00:31:32]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2015-05-15 00:31:32]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org,
ddorwin@chromium.org, nasko@chromium.org, gunsch@google.com
Link to the patchset: https://codereview.chromium.org/1131753003/#ps160001
(title: ":-(")

[commit-bot: I haz the power, 2015-05-15 00:32:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131753003/160001

[commit-bot: I haz the power, 2015-05-15 01:24:07]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-05-15 01:24:47]

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

[jam, 2015-05-15 01:46:34]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/1132773003/ by jam@chromium.org.

The reason for reverting is: Broke Linux GN clobber:
http://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20Clobber/buil...
.

[sandersd (OOO until July 31), 2015-05-15 03:31:45]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2015-05-15 03:31:46]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org,
ddorwin@chromium.org, nasko@chromium.org, gunsch@google.com
Link to the patchset: https://codereview.chromium.org/1131753003/#ps180001
(title: "Update unittest.")

[commit-bot: I haz the power, 2015-05-15 03:32:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131753003/180001

[ddorwin, 2015-05-15 03:40:42]

PS7 LGTM

[commit-bot: I haz the power, 2015-05-15 04:22:07]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-05-15 04:23:05]

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

[dcheng, 2015-05-15 15:31:31]

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

[dcheng, 2015-05-15 15:31:32]

https://codereview.chromium.org/1131753003/diff/180001/content/browser/media/...
File content/browser/media/cdm/browser_cdm_manager.h (right):

https://codereview.chromium.org/1131753003/diff/180001/content/browser/media/...
content/browser/media/cdm/browser_cdm_manager.h:34: struct
InitializeCdmParameters;
Is this leftover from an earlier patch?

[sandersd (OOO until July 31), 2015-05-15 17:52:18]

https://codereview.chromium.org/1131753003/diff/180001/content/browser/media/...
File content/browser/media/cdm/browser_cdm_manager.h (right):

https://codereview.chromium.org/1131753003/diff/180001/content/browser/media/...
content/browser/media/cdm/browser_cdm_manager.h:34: struct
InitializeCdmParameters;
On 2015/05/15 15:31:32, dcheng wrote:
> Is this leftover from an earlier patch?

Oops.

https://codereview.chromium.org/1136663006