Allows support for persistent session types in BrowserCdm.

Allows support for persistent session types in BrowserCdm.

Enables persistent session types for some Linux-based Cast builds.

BUG=492823
Committed: https://crrev.com/0eaa37b5c618503c1ea1d1b467481714bcf92595
Cr-Commit-Position: refs/heads/master@{#332105}

[gunsch, 2015-05-28 22:50:39]

@dcheng: please review changes to cdm_messages.h
@xhwang: please review the rest of the CL :)

[xhwang, 2015-05-28 23:17:39]

Also use bug 492823.

LGTM!

https://codereview.chromium.org/1160983002/diff/1/content/browser/media/cdm/b...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1160983002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:628: // TODO(xhwang): Add
SessionType support if needed.
Drop this comment now.

[gunsch, 2015-05-28 23:20:00]

https://codereview.chromium.org/1160983002/diff/1/content/browser/media/cdm/b...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1160983002/diff/1/content/browser/media/cdm/b...
content/browser/media/cdm/browser_cdm_manager.cc:628: // TODO(xhwang): Add
SessionType support if needed.
On 2015/05/28 23:17:39, xhwang wrote:
> Drop this comment now.

Done.

[ddorwin, 2015-05-28 23:51:50]

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

[ddorwin, 2015-05-28 23:51:51]

I skimmed through the CL (except the IPC stuff).

https://codereview.chromium.org/1160983002/diff/40001/chromecast/media/cdm/br...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://codereview.chromium.org/1160983002/diff/40001/chromecast/media/cdm/br...
chromecast/media/cdm/browser_cdm_cast.cc:56: 
Why are you deleting LoadSession()? This won't compile will it? (There is no
definition of the MediaKeys interface.)

https://codereview.chromium.org/1160983002/diff/40001/chromecast/renderer/key...
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/1160983002/diff/40001/chromecast/renderer/key...
chromecast/renderer/key_systems_cast.cc:35:
::media::EmeSessionTypeSupport::SUPPORTED;
Is all the implementation ready? Perhaps this should be in a separate CL?

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

https://codereview.chromium.org/1160983002/diff/40001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:639: 
Should we DCHECK session_type != temporary?
Or, DCHECK(IsPersistentSessionType(session_type).

https://codereview.chromium.org/1160983002/diff/40001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:651:
cdm->LoadSession(session_type, session_id, promise.Pass());
Blink checks the session type, right?

MediaDrmBridge::LoadSession(), etc. should probably have NOTREACHED() (or at
least NOTIMPLEMENTED) because they should not be reached without a change to the
Android KeySystems code. The message could even mention this.

Ditto for session_type in MediaDrmBridge::CreateSessionAndGenerateRequest()

[gunsch, 2015-05-28 23:59:13]

https://codereview.chromium.org/1160983002/diff/40001/chromecast/media/cdm/br...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://codereview.chromium.org/1160983002/diff/40001/chromecast/media/cdm/br...
chromecast/media/cdm/browser_cdm_cast.cc:56: 
On 2015/05/28 23:51:50, ddorwin wrote:
> Why are you deleting LoadSession()? This won't compile will it? (There is no
> definition of the MediaKeys interface.)

We have internal implementations that subclass BrowserCdmCast. BrowserCdmCast
also contains its own pure virtual methods.

https://codereview.chromium.org/1160983002/diff/40001/chromecast/renderer/key...
File chromecast/renderer/key_systems_cast.cc (right):

https://codereview.chromium.org/1160983002/diff/40001/chromecast/renderer/key...
chromecast/renderer/key_systems_cast.cc:35:
::media::EmeSessionTypeSupport::SUPPORTED;
On 2015/05/28 23:51:50, ddorwin wrote:
> Is all the implementation ready? Perhaps this should be in a separate CL?

Well we're certainly not shipping anything between me checking in this CL and
having a ready implementation.

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

https://codereview.chromium.org/1160983002/diff/40001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:639: 
On 2015/05/28 23:51:50, ddorwin wrote:
> Should we DCHECK session_type != temporary?
> Or, DCHECK(IsPersistentSessionType(session_type).

Done.

https://codereview.chromium.org/1160983002/diff/40001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:651:
cdm->LoadSession(session_type, session_id, promise.Pass());
On 2015/05/28 23:51:50, ddorwin wrote:
> Blink checks the session type, right?
> 
> MediaDrmBridge::LoadSession(), etc. should probably have NOTREACHED() (or at
> least NOTIMPLEMENTED) because they should not be reached without a change to
the
> Android KeySystems code. The message could even mention this.
> 
> Ditto for session_type in MediaDrmBridge::CreateSessionAndGenerateRequest()

Sure, promises were already rejected but NOTREACHED seems reasonable now that
the configuration code is in place.

[ddorwin, 2015-05-29 02:13:59]

LGTM % comment.

https://codereview.chromium.org/1160983002/diff/40001/chromecast/media/cdm/br...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://codereview.chromium.org/1160983002/diff/40001/chromecast/media/cdm/br...
chromecast/media/cdm/browser_cdm_cast.cc:56: 
On 2015/05/28 23:59:12, gunsch wrote:
> On 2015/05/28 23:51:50, ddorwin wrote:
> > Why are you deleting LoadSession()? This won't compile will it? (There is no
> > definition of the MediaKeys interface.)
> 
> We have internal implementations that subclass BrowserCdmCast. BrowserCdmCast
> also contains its own pure virtual methods.

Oh, I thought it was possible to build Cast, albeit stubbed, using only Chromium
code.

https://codereview.chromium.org/1160983002/diff/60001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1160983002/diff/60001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:319: NOTREACHED() << "EME persistent
sessions not yet supported on Android.";
On second thought, since that code could in theory be called from Mojo, which
would bypass these checks, it should probably be NOTIMPLEMENTED(). Thoughts?

[ddorwin, 2015-05-29 02:59:53]

I suggest changing the description to something like:
Support persistent session types in BrowserCdm.

Probably add a note that you are enabling them on Cast as well.

I removed the first BUG= entry (only one is supported I believe) and duped that
bug against the second.

[gunsch, 2015-05-29 16:40:10]

https://codereview.chromium.org/1160983002/diff/40001/chromecast/media/cdm/br...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://codereview.chromium.org/1160983002/diff/40001/chromecast/media/cdm/br...
chromecast/media/cdm/browser_cdm_cast.cc:56: 
On 2015/05/29 02:13:59, ddorwin wrote:
> On 2015/05/28 23:59:12, gunsch wrote:
> > On 2015/05/28 23:51:50, ddorwin wrote:
> > > Why are you deleting LoadSession()? This won't compile will it? (There is
no
> > > definition of the MediaKeys interface.)
> > 
> > We have internal implementations that subclass BrowserCdmCast.
BrowserCdmCast
> > also contains its own pure virtual methods.
> 
> Oh, I thought it was possible to build Cast, albeit stubbed, using only
Chromium
> code.

It is. We don't instantiate any BrowserCdmCast instances in our public code
today.

If you want to test EME today on Chromecast, you have to either:
a) Use a Cast Android build (since the DRM plugins are already in the Android
platform)
b) Do a full Chromecast build with internal code and test on Chromecast hardware

Ideally we'd have a BrowserCdm-based ClearKey support to test Chromecast's EME
flow in the public code. crbug.com/441957 is tracking but I don't think it's
actively being worked on.

https://codereview.chromium.org/1160983002/diff/60001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1160983002/diff/60001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:319: NOTREACHED() << "EME persistent
sessions not yet supported on Android.";
On 2015/05/29 02:13:59, ddorwin wrote:
> On second thought, since that code could in theory be called from Mojo, which
> would bypass these checks, it should probably be NOTIMPLEMENTED(). Thoughts?

Done.

[gunsch, 2015-05-29 16:41:27]

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

[gunsch, 2015-05-29 16:41:28]

@dcheng: PTAL at cdm_messages.h, thanks!

[dcheng, 2015-05-29 22:32:58]

IPC changes lgtm

https://codereview.chromium.org/1160983002/diff/100001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/1160983002/diff/100001/media/base/media_keys....
media/base/media_keys.h:76: SESSION_TYPE_MAX =
PERSISTENT_RELEASE_MESSAGE_SESSION
Hmm... we have both _MAX and _LAST. We should come up with a consistent naming
for this (especially since the clang plugin currently only checks _LAST).

[gunsch, 2015-05-29 23:03:11]

https://codereview.chromium.org/1160983002/diff/100001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/1160983002/diff/100001/media/base/media_keys....
media/base/media_keys.h:76: SESSION_TYPE_MAX =
PERSISTENT_RELEASE_MESSAGE_SESSION
On 2015/05/29 22:32:58, dcheng wrote:
> Hmm... we have both _MAX and _LAST. We should come up with a consistent naming
> for this (especially since the clang plugin currently only checks _LAST).

TIL there's a clang plugin check for this, that's helpful. Consistent within the
file but looks like broadly
(https://code.google.com/p/chromium/codesearch#search/&q=ENUM_TRAITS_MAX_VALUE)
not very consistent.

[gunsch, 2015-05-29 23:03:16]

The CQ bit was checked by gunsch@chromium.org

[gunsch, 2015-05-29 23:03:16]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1160983002/#ps100001
(title: "changes MediaDrmBridge logs to NOTIMPLEMENTED")

[commit-bot: I haz the power, 2015-05-29 23:05:26]

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

[commit-bot: I haz the power, 2015-05-29 23:10:07]

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

[commit-bot: I haz the power, 2015-05-29 23:10:09]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gunsch, 2015-05-30 00:04:32]

The CQ bit was checked by gunsch@chromium.org

[gunsch, 2015-05-30 00:04:33]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
ddorwin@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1160983002/#ps120001
(title: "rebase conflicts with https://codereview.chromium.org/1166453002")

[commit-bot: I haz the power, 2015-05-30 00:05:10]

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

[commit-bot: I haz the power, 2015-05-30 02:37:38]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-30 02:38:37]

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

[dcheng, 2015-05-30 03:19:24]

Yeah, unfortunately the check is still behind a flag, hence the rather
broad inconsistency. I'm hoping to fix this sometime next quarter.

On Fri, May 29, 2015, 16:03  <gunsch@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1160983002/diff/100001/media/base/media_keys.h
> File media/base/media_keys.h (right):
>
>
>
https://codereview.chromium.org/1160983002/diff/100001/media/base/media_keys....
> media/base/media_keys.h:76
>
<https://codereview.chromium.org/1160983002/diff/100001/media/base/media_keys....>:
> SESSION_TYPE_MAX =
> PERSISTENT_RELEASE_MESSAGE_SESSION
> On 2015/05/29 22:32:58, dcheng wrote:
> > Hmm... we have both _MAX and _LAST. We should come up with a
> consistent naming
> > for this (especially since the clang plugin currently only checks
> _LAST).
>
> TIL there's a clang plugin check for this, that's helpful. Consistent
> within the file but looks like broadly
> (
> https://code.google.com/p/chromium/codesearch#search/&q=ENUM_TRAITS_MAX_VALUE
> )
> not very consistent.
>
> https://codereview.chromium.org/1160983002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.