EME v0.1: Report defaultURL in KeyMessage.

EME v0.1: Report defaultURL in KeyMessage.

Only valid URLs will be reported.

BUG=164656
TEST=The keymessage event for external Clear Key includes a fixed URL.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173251

[ddorwin, 2012-12-08 00:02:35]

Small CL for the Chromium side of
https://bugs.webkit.org/show_bug.cgi?id=104284. I will land this after the DEPS
roll.

scherkus, let me know if you have concerns about where we introduce GURL. I
don't do it at a higher level because we'd need to include googleurl in the
External Clear Key CDM .so.

I'll need to rebase on the cdm.h changes.

[xhwang, 2012-12-08 01:16:44]

lgtm % nit

https://codereview.chromium.org/11469040/diff/3001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_proxy.cc (right):

https://codereview.chromium.org/11469040/diff/3001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_proxy.cc:172: << "Invalid URL in default_url: " <<
default_url.c_str();
do you need c_str() here?

[ddorwin, 2012-12-08 01:20:58]

https://codereview.chromium.org/11469040/diff/3001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_proxy.cc (right):

https://codereview.chromium.org/11469040/diff/3001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_proxy.cc:172: << "Invalid URL in default_url: " <<
default_url.c_str();
On 2012/12/08 01:16:44, xhwang wrote:
> do you need c_str() here?

Yes. Without it, no newline is added and all text following default_str is
ignored.

[scherkus (not reviewing), 2012-12-11 21:07:43]

lgtm but I'm confused about your c_str()

https://codereview.chromium.org/11469040/diff/3001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_proxy.cc (right):

https://codereview.chromium.org/11469040/diff/3001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_proxy.cc:172: << "Invalid URL in default_url: " <<
default_url.c_str();
On 2012/12/08 01:20:58, ddorwin wrote:
> On 2012/12/08 01:16:44, xhwang wrote:
> > do you need c_str() here?
> 
> Yes. Without it, no newline is added and all text following default_str is
> ignored.

huh? what text following default_str?

[ddorwin, 2012-12-14 20:52:27]

Added tests and modified the implementation a bit. This includes a rebase and
new files, so please just review against base.

Added shadi@ for test review.

PTAL

https://codereview.chromium.org/11469040/diff/3001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_proxy.cc (right):

https://codereview.chromium.org/11469040/diff/3001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_proxy.cc:172: << "Invalid URL in default_url: " <<
default_url.c_str();
On 2012/12/11 21:07:43, scherkus wrote:
> On 2012/12/08 01:20:58, ddorwin wrote:
> > On 2012/12/08 01:16:44, xhwang wrote:
> > > do you need c_str() here?
> > 
> > Yes. Without it, no newline is added and all text following default_str is
> > ignored.
> 
> huh? what text following default_str?

Removed. Bug in the sample code. I updated cdm.h separately to clarify.

[scherkus (not reviewing), 2012-12-14 21:45:26]

lgtm w/ nits

https://codereview.chromium.org/11469040/diff/9001/content/test/data/media/en...
File content/test/data/media/encrypted_media_utils.js (right):

https://codereview.chromium.org/11469040/diff/9001/content/test/data/media/en...
content/test/data/media/encrypted_media_utils.js:37:
'http://test.externalclearkey.chromium.org/';
indent 2 more spaces

https://codereview.chromium.org/11469040/diff/9001/content/test/data/media/en...
content/test/data/media/encrypted_media_utils.js:150: if
(EXTERNAL_CLEAR_KEY_KEY_SYSTEM != e.keySystem) {
what's up with the yoda condition

https://codereview.chromium.org/11469040/diff/9001/webkit/media/crypto/ppapi/...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

https://codereview.chromium.org/11469040/diff/9001/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/clear_key_cdm.cc:269: std::string heartbeat_message =
using ?: is great 'n all if it fits on one line, but this is 5 lines long

at this point I'd say just use an if

[ddorwin, 2012-12-14 21:55:16]

Thanks.

https://codereview.chromium.org/11469040/diff/9001/content/test/data/media/en...
File content/test/data/media/encrypted_media_utils.js (right):

https://codereview.chromium.org/11469040/diff/9001/content/test/data/media/en...
content/test/data/media/encrypted_media_utils.js:37:
'http://test.externalclearkey.chromium.org/';
On 2012/12/14 21:45:26, scherkus wrote:
> indent 2 more spaces

Done.

https://codereview.chromium.org/11469040/diff/9001/content/test/data/media/en...
content/test/data/media/encrypted_media_utils.js:150: if
(EXTERNAL_CLEAR_KEY_KEY_SYSTEM != e.keySystem) {
On 2012/12/14 21:45:26, scherkus wrote:
> what's up with the yoda condition

Done.

https://codereview.chromium.org/11469040/diff/9001/webkit/media/crypto/ppapi/...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

https://codereview.chromium.org/11469040/diff/9001/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/clear_key_cdm.cc:269: std::string heartbeat_message =
On 2012/12/14 21:45:26, scherkus wrote:
> using ?: is great 'n all if it fits on one line, but this is 5 lines long
> 
> at this point I'd say just use an if

Done.

[shadi, 2012-12-14 22:01:20]

LGTM % nit

https://codereview.chromium.org/11469040/diff/17002/content/test/data/media/e...
File content/test/data/media/encrypted_media_utils.js (right):

https://codereview.chromium.org/11469040/diff/17002/content/test/data/media/e...
content/test/data/media/encrypted_media_utils.js:132: failTest('keymessage
without a sessionId: ' + e.message);
s/sessionId/message

[ddorwin, 2012-12-14 22:10:56]

Thanks. Added a new check. CQing.

https://codereview.chromium.org/11469040/diff/17002/content/test/data/media/e...
File content/test/data/media/encrypted_media_utils.js (right):

https://codereview.chromium.org/11469040/diff/17002/content/test/data/media/e...
content/test/data/media/encrypted_media_utils.js:132: failTest('keymessage
without a sessionId: ' + e.message);
On 2012/12/14 22:01:20, shadi wrote:
> s/sessionId/message

Done.

[commit-bot: I haz the power, 2012-12-14 22:13:08]

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