Integrate browser tests with new EME player.

Integrate browser tests with new EME player.

This CL makes several changes to the EME player to enable test
specific expectations possible.

The EME player can be still used as a stand alone player.

BUG=356892
TESTS=out/Release/browser_tests --gtest_filter=*EncryptedMedia*

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

[shadi, 2014-05-29 00:47:45]

PTAL. 

The change updates eme browser tests that use the encrypted_media_player.html to
use eme_player.html (the new player based). 

There is still some changes needed to support FILE_IO* cases. However, I want to
start the review process while doing that. 

All the changes to the player should not affect the ability to run the player as
a stand alone test app as well.

[jrummell, 2014-05-29 21:45:05]

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player.html (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player.html:65: var video = null;
Why have a global var 'video'? onTimeUpdate() creates it's own, so it is only
used in Play().

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/globals.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/globals.js:52:
EME_VERSIONS_OPTIONS['Unprefixed EME (Working draft)'] = 'false';
If there is only unprefixed EME available, wouldn't this need to be 'true'?

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/globals.js:53: video = null;
It appears that the use of video here is just a local variable, not global. Can
this be scoped inside a function instead?

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/player.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/player.js:23: var failingEvents = ['error',
'onAbort'];
I'm confused. The first entry is event name, the second is a listener function
name. Shouldn't these be the same type? From below it appears they need to be
event names.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/player.js:57: return;
The return is not really necessary.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/player.js:94:
video.addEventListener('webkitkeyerror', function(error) {
Curious why some events are done as play.onWebKitxxx (2 above) while the 2 new
ones are done by registering the event handler directly. Shouldn't they be done
the same way?

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/test_app.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/test_app.js:47: // Update keySystem if
using prefixed Clear Key since it is not available in
s/available in/available/

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/test_app.js:69: return new
PrefixedClearKeyPlayer();
Should you be setting TestConfig.keySystem here, or are you expecting it to fail
when MediaKeys is created?

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/test_config.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/test_config.js:26: var params = {};
Is params needed anymore?

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/utils.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/utils.js:8: this.titleChanged = false;
Should you call resetTitleChange() instead?

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/utils.js:235: logString += ' ' +
arguments[i];
Could you use something like format_value() from
third_party/WebKit/LayoutTests/resources/testharness.js to pretty-print the
value of arguments[i]?

[ddorwin, 2014-05-30 17:42:55]

I just skimmed the data/ files.

https://codereview.chromium.org/308553002/diff/1/chrome/browser/media/encrypt...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/308553002/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_browsertest.cc:71: const char
kDefaultEMEPlayer[] = "eme_player.html";
"Eme"

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player.html (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player.html:63: // Global video element used to
access default video element from browser
Where/how does the browser do this?
If this is for browser tests only, maybe there should be a comment noting that.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player.html:68: var video = e.target;
Better to avoid shadowing the global. Is there a style for global names that
avoids this?

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/player.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/player.js:63: } catch (error) {
This is really an "exception". I think "e" is probably used like this commonly
enough for this to be okay, but I'm not familiar with the JS style.

This is also a little confusing with error at 60-61.

[shadi, 2014-05-31 00:31:37]

Resolved comments. FILE_IO cases still not converted yet though.

I wanted to upload the patch before my days off.

https://codereview.chromium.org/308553002/diff/1/chrome/browser/media/encrypt...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/308553002/diff/1/chrome/browser/media/encrypt...
chrome/browser/media/encrypted_media_browsertest.cc:71: const char
kDefaultEMEPlayer[] = "eme_player.html";
On 2014/05/30 17:42:56, ddorwin wrote:
> "Eme"

Done.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player.html (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player.html:63: // Global video element used to
access default video element from browser
On 2014/05/30 17:42:56, ddorwin wrote:
> Where/how does the browser do this?
> If this is for browser tests only, maybe there should be a comment noting
that.

The browser test checks for video.receivedKeyMessage in
RunSimpleEncryptedMediaTest(). I don't think it is easy for the test to know
whether this is expected or not at runtime...

I removed the need for global video variable by editing the browser test to
query "document.querySelector('video').receivedKeyMessage" instead.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player.html:65: var video = null;
On 2014/05/29 21:45:05, jrummell wrote:
> Why have a global var 'video'? onTimeUpdate() creates it's own, so it is only
> used in Play().
It was for the browser test code. As commented above, it is removed now.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player.html:68: var video = e.target;
On 2014/05/30 17:42:56, ddorwin wrote:
> Better to avoid shadowing the global. Is there a style for global names that
> avoids this?

No need for global anymore. The style guide suggests using namespace for
globals. Since I am not using namespace, probably we can have
TestApp.getVideoElement[s]() function if needed.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/globals.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/globals.js:52:
EME_VERSIONS_OPTIONS['Unprefixed EME (Working draft)'] = 'false';
On 2014/05/29 21:45:05, jrummell wrote:
> If there is only unprefixed EME available, wouldn't this need to be 'true'?

The true|false is for usePrefixedEME value. If there is only unprefixed EME
available then usePrefixedEME=false.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/globals.js:53: video = null;
On 2014/05/29 21:45:05, jrummell wrote:
> It appears that the use of video here is just a local variable, not global.
Can
> this be scoped inside a function instead? 

Done.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/player.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/player.js:23: var failingEvents = ['error',
'onAbort'];
On 2014/05/29 21:45:05, jrummell wrote:
> I'm confused. The first entry is event name, the second is a listener function
> name. Shouldn't these be the same type? From below it appears they need to be
> event names.

Done.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/player.js:57: return;
On 2014/05/29 21:45:05, jrummell wrote:
> The return is not really necessary.

Done.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/player.js:63: } catch (error) {
On 2014/05/30 17:42:56, ddorwin wrote:
> This is really an "exception". I think "e" is probably used like this commonly
> enough for this to be okay, but I'm not familiar with the JS style.
> 
> This is also a little confusing with error at 60-61.

Done.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/player.js:94:
video.addEventListener('webkitkeyerror', function(error) {
On 2014/05/29 21:45:05, jrummell wrote:
> Curious why some events are done as play.onWebKitxxx (2 above) while the 2 new
> ones are done by registering the event handler directly. Shouldn't they be
done
> the same way?

The disadvantage of using player.[event_name] here is that it would replace
whatever is defined in the player class (in prefixed_clearkey_player.js for
example). However, adding a video.event listener would still call the
player.event listener as well. 

For example, here 'webkitkeymessage' event listener sets receivedKeyMessage and
receivedHeartbeat flags for video in any prefixed clear player, even though
webkitAddKey gets handled in the player.onWebkitKeyMessage() function for each
of prefixed_clearkey_player and prefixed_widevine_player. 

Having said that, I agree it is confusing. I am going to stick to video event
listeners here unless there is a good need not to.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/test_app.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/test_app.js:47: // Update keySystem if
using prefixed Clear Key since it is not available in
On 2014/05/29 21:45:05, jrummell wrote:
> s/available in/available/

Done.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/test_app.js:69: return new
PrefixedClearKeyPlayer();
On 2014/05/29 21:45:05, jrummell wrote:
> Should you be setting TestConfig.keySystem here, or are you expecting it to
fail
> when MediaKeys is created?
Yes, I added these to support key systems as
"org.chromium.externalclearkey.decryptonly" or other test specific key systems.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/test_config.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/test_config.js:26: var params = {};
On 2014/05/29 21:45:05, jrummell wrote:
> Is params needed anymore?

Done.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
File chrome/test/data/media/eme_player_js/utils.js (right):

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/utils.js:8: this.titleChanged = false;
On 2014/05/29 21:45:05, jrummell wrote:
> Should you call resetTitleChange() instead?
This is more like a constructor, so setting this.titleChanged here is fine.

Since I have new function(){}, it will get executed before
Utils.resetTitleChange() is defined.

https://codereview.chromium.org/308553002/diff/1/chrome/test/data/media/eme_p...
chrome/test/data/media/eme_player_js/utils.js:235: logString += ' ' +
arguments[i];
On 2014/05/29 21:45:05, jrummell wrote:
> Could you use something like format_value() from
> third_party/WebKit/LayoutTests/resources/testharness.js to pretty-print the
> value of arguments[i]?

That would be nice, except that I would have to duplicate their code in here and
I do not see a good reason for that; our logs are too simple for now :). We
should keep an eye on the logs though in case pretty printing is needed.

And, by the way, the reason I switched to string += style is because
browsertests console logs do not log extra console.log args by default.

[shadi, 2014-06-12 22:12:27]

Added support for FileIO and LoadSession cases. 

Also passed all tests out/Release/browser_tests --gtest_filter=*EncryptedMedia*

PTAL

[ddorwin, 2014-06-13 21:03:56]

My comments have been addressed. Thanks. I'll leave this to John to complete the
full review.

[jrummell, 2014-06-13 21:53:59]

lgtm with a few nits.

https://codereview.chromium.org/308553002/diff/80001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_browsertest.cc (left):

https://codereview.chromium.org/308553002/diff/80001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_browsertest.cc:625: // TODO(jrummell): Fix
these tests to handle rejected promises when the CDM
Thanks for re-enabling these tests.

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
File chrome/test/data/media/eme_player_js/globals.js (right):

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
chrome/test/data/media/eme_player_js/globals.js:55:
EME_VERSIONS_OPTIONS['Unprefixed EME (Working draft)'] = 'false';
What happens if only unprefixed EME is available?

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
File chrome/test/data/media/eme_player_js/media_source_utils.js (right):

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
chrome/test/data/media/eme_player_js/media_source_utils.js:1: // Copyright 2013
The Chromium Authors. All rights reserved.
Date goes backwards? Looks like there are no other changes in this file, so it
can be reverted.

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
File chrome/test/data/media/eme_player_js/utils.js (right):

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
chrome/test/data/media/eme_player_js/utils.js:104: title = msg.name;
Do you need || 'Error' here as well? Or +=?

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
chrome/test/data/media/eme_player_js/utils.js:159: Utils.hasPrefix =
function(msg, prefix) {
I think it would be simpler to use the code from isHeartBeatMessage here (and
then have isHeartBeatMessage simply call hasPrefix(msg, HEART_BEAT_HEADER);).

[shadi, 2014-06-19 01:07:04]

Thanks for reviews.

As a summary, this CL changes all "simple playback" tests to use the new player.
I am still working on getting the few other tests (frame_size_change, and
config_change) to use updated player.

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
File chrome/test/data/media/eme_player_js/globals.js (right):

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
chrome/test/data/media/eme_player_js/globals.js:55:
EME_VERSIONS_OPTIONS['Unprefixed EME (Working draft)'] = 'false';
On 2014/06/13 21:53:59, jrummell wrote:
> What happens if only unprefixed EME is available?
It will only show the option of unprefixed EME. This still implies that
usePrefixedEME = false. 

Since this is confusing to read, I now disable the version (instead of not
showing it). The value of the option is a string used in test_config.js to
determine is usePrefixedEME is true.

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
File chrome/test/data/media/eme_player_js/media_source_utils.js (right):

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
chrome/test/data/media/eme_player_js/media_source_utils.js:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2014/06/13 21:53:59, jrummell wrote:
> Date goes backwards? Looks like there are no other changes in this file, so it
> can be reverted.
Actually 2014 to 2013 is the revert of my previous change. On xhwang's comment,
we should not increment the year.

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
File chrome/test/data/media/eme_player_js/utils.js (right):

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
chrome/test/data/media/eme_player_js/utils.js:104: title = msg.name;
On 2014/06/13 21:53:59, jrummell wrote:
> Do you need || 'Error' here as well? Or +=?

Done.

https://codereview.chromium.org/308553002/diff/80001/chrome/test/data/media/e...
chrome/test/data/media/eme_player_js/utils.js:159: Utils.hasPrefix =
function(msg, prefix) {
On 2014/06/13 21:53:59, jrummell wrote:
> I think it would be simpler to use the code from isHeartBeatMessage here (and
> then have isHeartBeatMessage simply call hasPrefix(msg, HEART_BEAT_HEADER);).

Done.

[shadi, 2014-06-19 01:07:12]

The CQ bit was checked by shadi@chromium.org

[commit-bot: I haz the power, 2014-06-19 01:11:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shadi@chromium.org/308553002/100001

[commit-bot: I haz the power, 2014-06-19 11:36:35]

Change committed as 278330

[chrishtr, 2014-06-19 17:11:02]

A revert of this CL has been created in
https://codereview.chromium.org/332323003/ by chrishtr@chromium.org.

The reason for reverting is: Causes browser tests to fail on all waterfalls..