Add |legacy_destination_url| back to SessionMessage for EME

Add |legacy_destination_url| back to SessionMessage for EME

Recent changes to CDM_7 removed |destination_url| as it is no longer
used in the EME spec. However legacy applications using the prefixed
EME API depend on it, so adding it back in so that it is available
to those applications.

BUG=448242
TEST=existing EME tests pass + Play movies play

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

[jrummell, 2015-01-13 18:00:52]

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

[jrummell, 2015-01-13 18:00:53]

PTAL. Changes are basically what was removed, except had to add a struct in
cdm_adapter as callback_factory_.NewCallback() only handles up to 4 parameters.

[xhwang, 2015-01-13 18:45:13]

lgtm % nits

https://codereview.chromium.org/813683005/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_media_keys.cc (right):

https://codereview.chromium.org/813683005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:164: const GURL&
destination_url) {
legacy?

https://codereview.chromium.org/813683005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:171: destination_url);
legacy?

https://codereview.chromium.org/813683005/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/813683005/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.cc:791: DLOG(WARNING) <<
"SessionMessage default_url is invalid : "
Just note: It's sad to have both destination url and default url in our code.
But this is not important now.

https://codereview.chromium.org/813683005/diff/1/media/blink/cdm_session_adap...
File media/blink/cdm_session_adapter.cc (right):

https://codereview.chromium.org/813683005/diff/1/media/blink/cdm_session_adap...
media/blink/cdm_session_adapter.cc:123: const GURL& legacy_destination_url) {
/* legacy_destination_url */ to be clear that the URL is dropped here.

https://codereview.chromium.org/813683005/diff/1/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/813683005/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:268: session_message_cb_.Run(web_session_id,
LICENSE_REQUEST, message, GURL());
GURL::EmptyGURL()?

[jrummell, 2015-01-13 19:46:30]

jrummell@chromium.org changed reviewers:
+ dcheng@chromium.org, dmichael@chromium.org

[jrummell, 2015-01-13 19:46:31]

+dmichael@ for pepper changes
+dcheng@ for ppapi/proxy/ppapi_messages.h

Background: When we removed |destination_url| from the CDM last week we broke
Google Play, so this is adding it back in. Done as a single change so that it
can be merged back into M41 now that it has branched.

https://codereview.chromium.org/813683005/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_media_keys.cc (right):

https://codereview.chromium.org/813683005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:164: const GURL&
destination_url) {
On 2015/01/13 18:45:13, xhwang wrote:
> legacy?

Done.

https://codereview.chromium.org/813683005/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:171: destination_url);
On 2015/01/13 18:45:13, xhwang wrote:
> legacy?

Done.

https://codereview.chromium.org/813683005/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/813683005/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.cc:791: DLOG(WARNING) <<
"SessionMessage default_url is invalid : "
On 2015/01/13 18:45:13, xhwang wrote:
> Just note: It's sad to have both destination url and default url in our code.
> But this is not important now.

Changed message since default_url not used here.

https://codereview.chromium.org/813683005/diff/1/media/blink/cdm_session_adap...
File media/blink/cdm_session_adapter.cc (right):

https://codereview.chromium.org/813683005/diff/1/media/blink/cdm_session_adap...
media/blink/cdm_session_adapter.cc:123: const GURL& legacy_destination_url) {
On 2015/01/13 18:45:13, xhwang wrote:
> /* legacy_destination_url */ to be clear that the URL is dropped here.

Done.

https://codereview.chromium.org/813683005/diff/1/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/813683005/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:268: session_message_cb_.Run(web_session_id,
LICENSE_REQUEST, message, GURL());
On 2015/01/13 18:45:13, xhwang wrote:
> GURL::EmptyGURL()?

Done.

[dcheng, 2015-01-13 19:57:57]

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

https://codereview.chromium.org/813683005/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:790: if
(!verified_gurl.is_valid() && !verified_gurl.is_empty()) {
Why check both? empty implies invalid. What is the behavior you want here?

https://codereview.chromium.org/813683005/diff/20001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/813683005/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.h:176: SessionMessage(std::string session_id,
const ref to avoid double copies.

[jrummell, 2015-01-13 20:31:16]

Updated.

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

https://codereview.chromium.org/813683005/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:790: if
(!verified_gurl.is_valid() && !verified_gurl.is_empty()) {
On 2015/01/13 19:57:57, dcheng wrote:
> Why check both? empty implies invalid. What is the behavior you want here?

Done. I just copied back what got removed last week.

https://codereview.chromium.org/813683005/diff/20001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/813683005/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.h:176: SessionMessage(std::string session_id,
On 2015/01/13 19:57:57, dcheng wrote:
> const ref to avoid double copies.

Done.

[dcheng, 2015-01-13 20:48:27]

lgtm for IPC changes

[dmichael (off chromium), 2015-01-13 21:15:15]

pepper stuff lgtm

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

https://codereview.chromium.org/813683005/diff/40001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:787:
DCHECK(destination_url_string);
How much do we trust the CDM code? Is it OK that it could cause a NULL pointer
dereference in the renderer in a release build? Maybe this should just be a
CHECK or:
if (!destination_url_string)
  return;

https://codereview.chromium.org/813683005/diff/40001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/813683005/diff/40001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:269: GURL::EmptyGURL());
This is weird-looking enough to warrant a comment.

https://codereview.chromium.org/813683005/diff/40001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/813683005/diff/40001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:731: std::vector<uint8_t>(message, message +
message_size),
You have to allocate and copy here, but you allocate+copy again because the
constructor copied the parameter in to the struct's vector member. Is "message"
always fairly small?

[jrummell, 2015-01-13 21:41:45]

Thanks for the reviews.

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

https://codereview.chromium.org/813683005/diff/40001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:787:
DCHECK(destination_url_string);
On 2015/01/13 21:15:15, dmichael wrote:
> How much do we trust the CDM code? Is it OK that it could cause a NULL pointer
> dereference in the renderer in a release build? Maybe this should just be a
> CHECK or:
> if (!destination_url_string)
>   return;

Done.

https://codereview.chromium.org/813683005/diff/40001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/813683005/diff/40001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:269: GURL::EmptyGURL());
On 2015/01/13 21:15:15, dmichael wrote:
> This is weird-looking enough to warrant a comment.

Done.

https://codereview.chromium.org/813683005/diff/40001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/813683005/diff/40001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:731: std::vector<uint8_t>(message, message +
message_size),
On 2015/01/13 21:15:15, dmichael wrote:
> You have to allocate and copy here, but you allocate+copy again because the
> constructor copied the parameter in to the struct's vector member. Is
"message"
> always fairly small?

Changed constructor to avoid double copy.

[jrummell, 2015-01-13 22:23:11]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-01-13 22:24:12]

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

[commit-bot: I haz the power, 2015-01-14 00:35:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-14 00:36:05]

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