Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
182113005:
Update EME browser tests to include EME WD (Closed)
|
Created:
6 years, 9 months ago by jrummell Modified:
6 years, 9 months ago CC:
chromium-reviews, fischman+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUpdate EME browser tests to include EME WD
Modify the existing EME browser tests to include testing EME WD.
BUG=224791
TEST=new browser tests pass
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255676
Patch Set 1 #
Total comments: 35
Patch Set 2 : WD -> unprefixed #
Total comments: 8
Patch Set 3 : Changes + shadi's fix #
Total comments: 18
Patch Set 4 : Rename tests #
Total comments: 25
Patch Set 5 : Cleanup #
Messages
Total messages: 28 (0 generated)
PTAL.
LG overall. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:75: // Whether to use prefixed EME or EME WD. Let's keep it simple: prefixed vs. unprefixed. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:76: enum EMEType { EmeVersion? https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:148: query_params.push_back(std::make_pair("useMSE", "1")); Not sure about JS style, but these might be incorrectly capitalized. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:343: // - bool: True to use EME WD, otherwise use prefixed EME. ditto https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:423: This is a lot of tests, each of which takes several seconds to launch. I wonder if we there is a subset of unprefixed we can run until we get closer to the transition. For example, maybe we focus on MSE for unprefixed and don't test SRC for now. I don't think we'd lose much coverage. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:430: INSTANTIATE_TEST_CASE_P(SRC_ClearKey_WD, ditto here and below. Also, I would instead name the tests _Prefixed and leave off _WD or _Unprefixed. We want the least amount of work to do to remove prefixed and keep the permanent spec. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:556: Prefixed, We should be able to test this with unprefixed too. createSession() should throw. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:53: Can we say that these are copied from the layout test file? (Are they?) https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:54: // Encodes data into base64 string without trailing '='. What you really want is a base64url encoding (though that may not be what Chrome currently expects since the spec is not yet updated [1]). Maybe we just need a TODO to update this when the implementation is updated. The method would then be base64urlEncode. [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=17682 https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:55: function base64Encode(data) |dataString|? https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:57: var result = btoa(String.fromCharCode.apply(null, data)); What is the apply() call doing/solving? https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:72: // Creates a JWK Set from multiple JWKs. ... from an array of JWK(s). https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:115: // Shared by prefixed EME and EME WD ditto on "WD" in this file. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:216: video.receivedKeyAdded = true; Note: This will break when we remove ready. Maybe we'll just skip this for unprefixed. We'll also want to add receivedOpen at that time. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:219: function onMessage(message) { This function has a lot of duplicate code. How much is actually different other than default/destinationURL? Can we try to reuse more of it? https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:281: I skipped to here. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:339: mediaKeys = new MediaKeys(keySystem); nit: Any reason not to do this at 328?
Updated. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:75: // Whether to use prefixed EME or EME WD. On 2014/03/04 21:18:48, ddorwin wrote: > Let's keep it simple: prefixed vs. unprefixed. Done. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:76: enum EMEType { On 2014/03/04 21:18:48, ddorwin wrote: > EmeVersion? Done. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:148: query_params.push_back(std::make_pair("useMSE", "1")); On 2014/03/04 21:18:48, ddorwin wrote: > Not sure about JS style, but these might be incorrectly capitalized. I just followed what the existing code did. I can't find anything the the Google JavaScript Style Guide that mentions what to do with acronyms. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:343: // - bool: True to use EME WD, otherwise use prefixed EME. On 2014/03/04 21:18:48, ddorwin wrote: > ditto Done. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:423: On 2014/03/04 21:18:48, ddorwin wrote: > This is a lot of tests, each of which takes several seconds to launch. I wonder > if we there is a subset of unprefixed we can run until we get closer to the > transition. For example, maybe we focus on MSE for unprefixed and don't test SRC > for now. I don't think we'd lose much coverage. Done. https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:430: INSTANTIATE_TEST_CASE_P(SRC_ClearKey_WD, On 2014/03/04 21:18:48, ddorwin wrote: > ditto here and below. Also, I would instead name the tests _Prefixed and leave > off _WD or _Unprefixed. We want the least amount of work to do to remove > prefixed and keep the permanent spec. Done. I didn't want to change the names of the existing tests in case there are downstream systems that do things with the names (like test flakiness). https://codereview.chromium.org/182113005/diff/1/chrome/browser/media/encrypt... chrome/browser/media/encrypted_media_browsertest.cc:556: Prefixed, On 2014/03/04 21:18:48, ddorwin wrote: > We should be able to test this with unprefixed too. createSession() should > throw. Added, but fails due to FATAL error. Opened http://crbug.com/349181 to track enabling this new test. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:53: On 2014/03/04 21:18:48, ddorwin wrote: > Can we say that these are copied from the layout test file? (Are they?) They are. Updated. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:54: // Encodes data into base64 string without trailing '='. On 2014/03/04 21:18:48, ddorwin wrote: > What you really want is a base64url encoding (though that may not be what Chrome > currently expects since the spec is not yet updated [1]). > > Maybe we just need a TODO to update this when the implementation is updated. > > The method would then be base64urlEncode. > > [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=17682 Done. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:55: function base64Encode(data) On 2014/03/04 21:18:48, ddorwin wrote: > |dataString|? This is a Uint8Array, not a string. Updated comment. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:57: var result = btoa(String.fromCharCode.apply(null, data)); On 2014/03/04 21:18:48, ddorwin wrote: > What is the apply() call doing/solving? It appears to be converting the Uint8Array to a string. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:72: // Creates a JWK Set from multiple JWKs. On 2014/03/04 21:18:48, ddorwin wrote: > ... from an array of JWK(s). Done. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:115: // Shared by prefixed EME and EME WD On 2014/03/04 21:18:48, ddorwin wrote: > ditto on "WD" in this file. Done. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:216: video.receivedKeyAdded = true; On 2014/03/04 21:18:48, ddorwin wrote: > Note: This will break when we remove ready. Maybe we'll just skip this for > unprefixed. > We'll also want to add receivedOpen at that time. encrypted_media_player.html checks for the flag being set. Fixed. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:219: function onMessage(message) { On 2014/03/04 21:18:48, ddorwin wrote: > This function has a lot of duplicate code. How much is actually different other > than default/destinationURL? Can we try to reuse more of it? Also keySystem, update()/webKitAddkey(), using video/message.target. Not sure if we care about the naming (onKeyMessage/onMessage). However, I combined them. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:339: mediaKeys = new MediaKeys(keySystem); On 2014/03/04 21:18:48, ddorwin wrote: > nit: Any reason not to do this at 328? Unprefixed tests timeout if done at 328.
There is also one comment in PS1. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:339: mediaKeys = new MediaKeys(keySystem); On 2014/03/04 23:55:49, jrummell wrote: > On 2014/03/04 21:18:48, ddorwin wrote: > > nit: Any reason not to do this at 328? > > Unprefixed tests timeout if done at 328. Bug! Please file. I assume it is the setMediaKeys call that causes this. Do all key systems time out? I assume CK passes because we have tests for that, right? We need to add some setMediaKeys tests here once we fix that bug. https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:230: command_line->AppendSwitch(switches::kEnableEncryptedMedia); Why does this no longer check the EME version? We should try to run tests in production configuration wherever possible. https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:427: Values(Prefixed))); Maybe we should have a TODO to add Unprefixed later so we don't forget. If there's a way to make DISABLED_ work, that would be even better. https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:543: IN_PROC_BROWSER_TEST_F(WVEncryptedMediaTest, ParentThrowsExceptionPrefixed) { _Prefixed https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:557: // FIXME(jrummell): http://crbug.com/349181 Move this to above line 555 and add DISABLED_ to the name. We still want to compile, and we don't leave code commented out.
Updated, and includes https://codereview.chromium.org/180733004/. Changes in encrypted_media_utils.js due to rebase only. https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/1/chrome/test/data/media/encry... chrome/test/data/media/encrypted_media_utils.js:339: mediaKeys = new MediaKeys(keySystem); On 2014/03/05 19:04:36, ddorwin wrote: > On 2014/03/04 23:55:49, jrummell wrote: > > On 2014/03/04 21:18:48, ddorwin wrote: > > > nit: Any reason not to do this at 328? > > > > Unprefixed tests timeout if done at 328. > > Bug! Please file. I assume it is the setMediaKeys call that causes this. Do all > key systems time out? I assume CK passes because we have tests for that, right? > We need to add some setMediaKeys tests here once we fix that bug. Opened http://crbug.com/349546 https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:230: command_line->AppendSwitch(switches::kEnableEncryptedMedia); On 2014/03/05 19:04:36, ddorwin wrote: > Why does this no longer check the EME version? We should try to run tests in > production configuration wherever possible. WVEncryptedMediaTest isn't parameterized, so can't do it during initialization. However, moved to always set the switch for WV, and put other tests back to checking the setting. https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:427: Values(Prefixed))); On 2014/03/05 19:04:36, ddorwin wrote: > Maybe we should have a TODO to add Unprefixed later so we don't forget. > If there's a way to make DISABLED_ work, that would be even better. Done as TODO. https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:543: IN_PROC_BROWSER_TEST_F(WVEncryptedMediaTest, ParentThrowsExceptionPrefixed) { On 2014/03/05 19:04:36, ddorwin wrote: > _Prefixed Done. https://codereview.chromium.org/182113005/diff/20001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:557: // FIXME(jrummell): http://crbug.com/349181 On 2014/03/05 19:04:36, ddorwin wrote: > Move this to above line 555 and add DISABLED_ to the name. We still want to > compile, and we don't leave code commented out. Done.
LGTM % comments. https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:432: // TODO: Enable unprefixed tests when prefixed EME goes away. Disabled now ...before shipping unprefixed EME. https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:435: // INSTANTIATE_TEST_CASE_P(SRC_ClearKey, use #if 0 rather than comments.
https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:333: command_line->AppendSwitch(switches::kEnableEncryptedMedia); Is it safe to add this for both versions (prefixed and unprefixed)? https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:435: // INSTANTIATE_TEST_CASE_P(SRC_ClearKey, On 2014/03/05 23:57:48, ddorwin wrote: > use #if 0 rather than comments. Another way is to name it DISABLED_SRC_Clearkey. https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:446: Values(Prefixed))); For the other prefixed tests there is no _Prefixed in the name. Should we assume default is prefixed and just do a MSE_ClearKey_Unprefixed? https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:481: Values(Prefixed))); I might be reading the parametrized tests wrong, does this mean we don't want to test WV unprefixed version? https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_player.html (right): https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_player.html:13: var usePrefixedEME = QueryString.usePrefixedEME == 1; I think you don't need this, usePrefixedEME is global variable in encrypted_media_utils.js. https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:304: video.setMediaKeys(mediaKeys); Out of curiosity, should this always be after video.src = ...? https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/m... File chrome/test/data/media/mse_config_change.html (right): https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/m... chrome/test/data/media/mse_config_change.html:12: var usePrefixedEME = QueryString.usePrefixedEME == 1; You don't need this, usePrefixedEME is global variable in encrypted_media_utils.js (the runEncrypted flag is only used in this test though).
Updated. https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:333: command_line->AppendSwitch(switches::kEnableEncryptedMedia); On 2014/03/06 01:16:21, shadi1 wrote: > Is it safe to add this for both versions (prefixed and unprefixed)? Should be fine. Events generated have different names (e.g. webkitneedkey and needkey), and the JS only adds a listener for the one it is expecting. Chrome may need to have both prefixed and unprefixed available during a transition period. https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:432: // TODO: Enable unprefixed tests when prefixed EME goes away. Disabled now On 2014/03/05 23:57:48, ddorwin wrote: > ...before shipping unprefixed EME. Done. https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:435: // INSTANTIATE_TEST_CASE_P(SRC_ClearKey, On 2014/03/05 23:57:48, ddorwin wrote: > use #if 0 rather than comments. Done. https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:435: // INSTANTIATE_TEST_CASE_P(SRC_ClearKey, On 2014/03/06 01:16:21, shadi1 wrote: > On 2014/03/05 23:57:48, ddorwin wrote: > > use #if 0 rather than comments. > > Another way is to name it DISABLED_SRC_Clearkey. Done. https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:446: Values(Prefixed))); On 2014/03/06 01:16:21, shadi1 wrote: > For the other prefixed tests there is no _Prefixed in the name. Should we assume > default is prefixed and just do a MSE_ClearKey_Unprefixed? In the long run the prefixed ones will go away, so we want the unadorned names to remain around. Once we get the other unprefixed CDMs to load, we'll enable those tests to do both. https://codereview.chromium.org/182113005/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:481: Values(Prefixed))); On 2014/03/06 01:16:21, shadi1 wrote: > I might be reading the parametrized tests wrong, does this mean we don't want to > test WV unprefixed version? Correct (for now, until unprefixed WV loads). https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_player.html (right): https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_player.html:13: var usePrefixedEME = QueryString.usePrefixedEME == 1; On 2014/03/06 01:16:21, shadi1 wrote: > I think you don't need this, usePrefixedEME is global variable in > encrypted_media_utils.js. Done. https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:304: video.setMediaKeys(mediaKeys); On 2014/03/06 01:16:21, shadi1 wrote: > Out of curiosity, should this always be after video.src = ...? Nope. But the tests timeout if it is before (http://crbug.com/349546). https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/m... File chrome/test/data/media/mse_config_change.html (right): https://codereview.chromium.org/182113005/diff/40001/chrome/test/data/media/m... chrome/test/data/media/mse_config_change.html:12: var usePrefixedEME = QueryString.usePrefixedEME == 1; On 2014/03/06 01:16:21, shadi1 wrote: > You don't need this, usePrefixedEME is global variable in > encrypted_media_utils.js (the runEncrypted flag is only used in this test > though). Done.
LGTM Thanks!
Thanks a lot! lgtm % nits ddorwin/shadi: Please see questions for discussion. https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:76: enum EMEVersion { ddorwin@: What did we decide on EME or Eme? https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:78: Unprefixed nit: We use UPPER_CASE for enums. https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:329: class WVEncryptedMediaTest : public EncryptedMediaTestBase { Suggestion for a follow up cleanup CL. Now this is only used to test ParentThrowsException, which can easily be covered by ECKEncryptedMediaTest. Then we can remove this test. ddorwin/shadi: WDYT? https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:432: // TODO: Enable unprefixed tests before shipping unprefixed EME. Disabled now Here and below, TODO(jrummell) https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:603: // FIXME(jrummell): http://crbug.com/349181 s/FIXME/TODO https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:59: // TODO: Update once the EME spec is updated to say base64url encoding. TODO(jrummell) https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:61: { Nit: Layout test uses function foo { } style. But this test uses function foo { } I don't know what's the style rule for JS code in Chromium. But better be consistent. https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:206: // No tested key system returns defaultURL in for key request messages. shadi: Is this also true for Widevine? https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:225: mediaKeySession.update(jwkSet); Does the invalid response have to be a valid JWK set? e.g. Can we just update(invalidData)?
https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:76: enum EMEVersion { On 2014/03/06 16:36:53, xhwang wrote: > ddorwin@: What did we decide on EME or Eme? Eme (where we are addding new values). See line 65 for example. But leave MSE alone for now to keep the changes to a minimum. https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:329: class WVEncryptedMediaTest : public EncryptedMediaTestBase { On 2014/03/06 16:36:53, xhwang wrote: > Suggestion for a follow up cleanup CL. > > Now this is only used to test ParentThrowsException, which can easily be covered > by ECKEncryptedMediaTest. Then we can remove this test. > > ddorwin/shadi: WDYT? If Widevine is not registered (by line 334), this won't test the same path on Win and Mac. https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:61: { On 2014/03/06 16:36:53, xhwang wrote: > Nit: Layout test uses > > function foo > { > } > > style. But this test uses > > function foo { > } > > I don't know what's the style rule for JS code in Chromium. But better be > consistent. Hmm. This is copied from Blink as noted on line 55, so I think it's better left this way. WDYT?
https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:61: { On 2014/03/06 18:11:10, ddorwin wrote: > On 2014/03/06 16:36:53, xhwang wrote: > > Nit: Layout test uses > > > > function foo > > { > > } > > > > style. But this test uses > > > > function foo { > > } > > > > I don't know what's the style rule for JS code in Chromium. But better be > > consistent. > > Hmm. This is copied from Blink as noted on line 55, so I think it's better left > this way. WDYT? I think that unless we are always keeping layout test version and this version in sync we should reformat. https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:206: // No tested key system returns defaultURL in for key request messages. On 2014/03/06 16:36:53, xhwang wrote: > shadi: Is this also true for Widevine? Yes. The policies used by the test license server do not have a default URL value. If that changes, then this would fail. Since we just landed the test license server we can start to test this. However, I think we should create specific policy test cases for that.
https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:329: class WVEncryptedMediaTest : public EncryptedMediaTestBase { On 2014/03/06 18:11:10, ddorwin wrote: > On 2014/03/06 16:36:53, xhwang wrote: > > Suggestion for a follow up cleanup CL. > > > > Now this is only used to test ParentThrowsException, which can easily be > covered > > by ECKEncryptedMediaTest. Then we can remove this test. > > > > ddorwin/shadi: WDYT? > > If Widevine is not registered (by line 334), this won't test the same path on > Win and Mac. I see. Thanks! https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:61: { On 2014/03/06 18:18:41, shadi1 wrote: > On 2014/03/06 18:11:10, ddorwin wrote: > > On 2014/03/06 16:36:53, xhwang wrote: > > > Nit: Layout test uses > > > > > > function foo > > > { > > > } > > > > > > style. But this test uses > > > > > > function foo { > > > } > > > > > > I don't know what's the style rule for JS code in Chromium. But better be > > > consistent. > > > > Hmm. This is copied from Blink as noted on line 55, so I think it's better > left > > this way. WDYT? > I think that unless we are always keeping layout test version and this version > in sync we should reformat. We need to keep the functionality of these two versions in sync. About the format, I don't know. Either way is fine to me :)
Thanks for the reviews. https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:76: enum EMEVersion { On 2014/03/06 18:11:10, ddorwin wrote: > On 2014/03/06 16:36:53, xhwang wrote: > > ddorwin@: What did we decide on EME or Eme? > > Eme (where we are addding new values). See line 65 for example. > > But leave MSE alone for now to keep the changes to a minimum. Done. https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:78: Unprefixed On 2014/03/06 16:36:53, xhwang wrote: > nit: We use UPPER_CASE for enums. Done. https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:432: // TODO: Enable unprefixed tests before shipping unprefixed EME. Disabled now On 2014/03/06 16:36:53, xhwang wrote: > Here and below, TODO(jrummell) Done. https://codereview.chromium.org/182113005/diff/60001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_browsertest.cc:603: // FIXME(jrummell): http://crbug.com/349181 On 2014/03/06 16:36:53, xhwang wrote: > s/FIXME/TODO Done. https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:59: // TODO: Update once the EME spec is updated to say base64url encoding. On 2014/03/06 16:36:53, xhwang wrote: > TODO(jrummell) Done. https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:61: { On 2014/03/06 18:11:10, ddorwin wrote: > On 2014/03/06 16:36:53, xhwang wrote: > > Nit: Layout test uses > > > > function foo > > { > > } > > > > style. But this test uses > > > > function foo { > > } > > > > I don't know what's the style rule for JS code in Chromium. But better be > > consistent. > > Hmm. This is copied from Blink as noted on line 55, so I think it's better left > this way. WDYT? Made consistent with rest of file. Also updated leading spaces. https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:225: mediaKeySession.update(jwkSet); On 2014/03/06 16:36:53, xhwang wrote: > Does the invalid response have to be a valid JWK set? e.g. Can we just > update(invalidData)? Done.
The CQ bit was checked by jrummell@chromium.org
https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:55: // JWK routines copied from third_party/WebKit/LayoutTests/media/ s/JWK/JSON Web Key (JWK) https://codereview.chromium.org/182113005/diff/60001/chrome/test/data/media/e... chrome/test/data/media/encrypted_media_utils.js:61: { On 2014/03/06 18:20:57, xhwang wrote: > On 2014/03/06 18:18:41, shadi1 wrote: > > On 2014/03/06 18:11:10, ddorwin wrote: > > > On 2014/03/06 16:36:53, xhwang wrote: > > > > Nit: Layout test uses > > > > > > > > function foo > > > > { > > > > } > > > > > > > > style. But this test uses > > > > > > > > function foo { > > > > } > > > > > > > > I don't know what's the style rule for JS code in Chromium. But better be > > > > consistent. > > > > > > Hmm. This is copied from Blink as noted on line 55, so I think it's better > > left > > > this way. WDYT? > > I think that unless we are always keeping layout test version and this version > > in sync we should reformat. > > We need to keep the functionality of these two versions in sync. About the > format, I don't know. Either way is fine to me :) What about adding comments // Start of copy of JWK helper functions .... // End of copy of JWK helper functions
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/182113005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/182113005/80001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/182113005/80001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/182113005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) ash_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/182113005/80001
Message was sent while issue was closed.
Change committed as 255676 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
