ReleaseSession() should call RemoveSession()

Changing the behavior of ReleaseSession() from Close() to Remove()

With the recent EME spec changes, ReleaseSession() got replaced by
CloseSession() and RemoveSession(). Until all the changes to support
both methods are in place, calls to ReleaseSession() should call
RemoveSession() in order for existing prefixed EME applications
to continue to work.

Unprefixed Close() calls now call RemoveSession(), and thus don't do
the correct thing. This will be fixed when both CloseSession() and
RemoveSession() are passed through Pepper.

BUG=406606
TEST=All EME browser_tests pass

Committed: https://crrev.com/ec6d38eaaacd81198e1a78dec84418b54b218e55
Cr-Commit-Position: refs/heads/master@{#291602}

[jrummell, 2014-08-22 23:47:57]

PTAL.

[ddorwin, 2014-08-22 23:59:44]

The CL description should probably mention that unprefixed EME's close() will
have the wrong behavior.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:388: cdm_->RemoveSession(
This CL makes a bunch of changes we'll want to undo (i.e. revert this CL) once
Pepper is properly piped. Can we instead limit the number of changes by adding
LegacyReleaseSession() to CdmWrapper, calling that here and having it call
Remove?

CloseSession() would never be called. It might require some more code in the
Wrapper, but it avoids unwanted changes here.

[xhwang, 2014-08-23 00:01:49]

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter.h
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.h:72: const std::string& web_session_id) OVERRIDE;
Will we also remove this in CDM.h?

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper.h
File media/cdm/ppapi/cdm_wrapper.h (right):

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:59: virtual bool CloseSession(uint32_t promise_id,
We can change the return value back to void once we stop supporting CDM4? In
that case, add a TODO here to track that.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:64: uint32_t web_session_id_size) = 0;
nit: swap the order of CloseSession and RemoveSession to be consistent with
CdmAdapter.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:473: }
ditto about order swap

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:583: void
CdmWrapperImpl<cdm::ContentDecryptionModule_5>::RemoveSession(
It might be easier if we just drop CDM5 support :)

[ddorwin, 2014-08-23 00:14:31]

LGTM once xhwang's comments are resolved.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:388: cdm_->RemoveSession(
On 2014/08/22 23:59:44, ddorwin wrote:
> This CL makes a bunch of changes we'll want to undo (i.e. revert this CL) once
> Pepper is properly piped. Can we instead limit the number of changes by adding
> LegacyReleaseSession() to CdmWrapper, calling that here and having it call
> Remove?
> 
> CloseSession() would never be called. It might require some more code in the
> Wrapper, but it avoids unwanted changes here.

As discussed, even after piping everything through Pepper, we would still need
RemoveSession() to succeed for all CDM versions, so some such changes will be
necessary.

Maybe we should update the description, etc. to be more explicit that we are
changing the behavior of ReleaseSession() from Close to Remove. (And that
unprefixed Close() is thus broken for now.)

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper.h
File media/cdm/ppapi/cdm_wrapper.h (right):

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:64: uint32_t web_session_id_size) = 0;
On 2014/08/23 00:01:48, xhwang wrote:
> nit: swap the order of CloseSession and RemoveSession to be consistent with
> CdmAdapter.

Actually, the CdmAdapter order is "wrong". I am fine with leaving it, though, to
kep this CL small. Maybe we need a TODO there along with the rename. WDYT?

[jrummell, 2014-08-23 00:27:16]

Updated.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:388: cdm_->RemoveSession(
On 2014/08/23 00:14:30, ddorwin wrote:
> On 2014/08/22 23:59:44, ddorwin wrote:
> > This CL makes a bunch of changes we'll want to undo (i.e. revert this CL)
once
> > Pepper is properly piped. Can we instead limit the number of changes by
adding
> > LegacyReleaseSession() to CdmWrapper, calling that here and having it call
> > Remove?
> > 
> > CloseSession() would never be called. It might require some more code in the
> > Wrapper, but it avoids unwanted changes here.
> 
> As discussed, even after piping everything through Pepper, we would still need
> RemoveSession() to succeed for all CDM versions, so some such changes will be
> necessary.
> 
> Maybe we should update the description, etc. to be more explicit that we are
> changing the behavior of ReleaseSession() from Close to Remove. (And that
> unprefixed Close() is thus broken for now.)

Description updated.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter.h
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.h:72: const std::string& web_session_id) OVERRIDE;
On 2014/08/23 00:01:48, xhwang wrote:
> Will we also remove this in CDM.h?

It is already called RemoveSession() in CDM6.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper.h
File media/cdm/ppapi/cdm_wrapper.h (right):

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:59: virtual bool CloseSession(uint32_t promise_id,
On 2014/08/23 00:01:48, xhwang wrote:
> We can change the return value back to void once we stop supporting CDM4? In
> that case, add a TODO here to track that.

Done.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:64: uint32_t web_session_id_size) = 0;
On 2014/08/23 00:01:48, xhwang wrote:
> nit: swap the order of CloseSession and RemoveSession to be consistent with
> CdmAdapter.

Fixed CdmAdapter instead.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:64: uint32_t web_session_id_size) = 0;
On 2014/08/23 00:14:30, ddorwin wrote:
> On 2014/08/23 00:01:48, xhwang wrote:
> > nit: swap the order of CloseSession and RemoveSession to be consistent with
> > CdmAdapter.
> 
> Actually, the CdmAdapter order is "wrong". I am fine with leaving it, though,
to
> kep this CL small. Maybe we need a TODO there along with the rename. WDYT?

Fixed CdmAdapter.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:473: }
On 2014/08/23 00:01:48, xhwang wrote:
> ditto about order swap

CDM6 (and the EME spec) have CloseSession() first, so copying that.

https://codereview.chromium.org/497153005/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:583: void
CdmWrapperImpl<cdm::ContentDecryptionModule_5>::RemoveSession(
On 2014/08/23 00:01:48, xhwang wrote:
> It might be easier if we just drop CDM5 support :)

Acknowledged. That would just make this change bigger :)

[ddorwin, 2014-08-23 00:33:57]

Please update the first line to say we are changing the behavior of
ReleaseSession from Close to Remove (to restore the correct behavior for
prefixed EME use cases).

Then, LGTM. Thanks!

[xhwang, 2014-08-23 00:44:01]

In the CL description, "Unprefixed Close()" should be "Unprefixed
cancelKeyRequest()"?

otherwise, lgtm!

[jrummell, 2014-08-23 00:47:19]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-08-23 04:56:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jrummell@chromium.org/497153005/20001

[commit-bot: I haz the power, 2014-08-23 22:09:32]

Committed patchset #2 (20001) as 26432cb887b4ce8dade42601352a0e7377a00f50

[commit-bot: I haz the power, 2014-09-10 02:31:50]

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