Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
182453005:
Encrypted Media: Convert unprefixed syntax layout test to use testharness.js. (Closed)
DescriptionEncrypted Media: Convert unprefixed syntax layout test to use testharness.js.
BUG=224791
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168884
Patch Set 1 #
Total comments: 42
Patch Set 2 : comments addressed #
Total comments: 16
Patch Set 3 : comments addressed #
Total comments: 4
Patch Set 4 : comments addressed #
Messages
Total messages: 17 (0 generated)
PTAL
https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right): https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:1: <!DOCTYPE html> We'll need to rename this file in a future CL. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:4: <title>Test unprefixed EME syntax</title> "unprefixed " should be removed. This would be irrelevant if upstreamed. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:8: empty line https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:16: var initData = stringToUint8Array('mock'); FWIW, 'mock' is left over from when there was a mock CDM. We can pick something else. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:19: function test_syntax(func, params, exception, description) test_invalid_syntax? It's not possible to test valid syntax. Why hacker style instead of testInvalidSyntax? Now that it won't fit, it's probably worth renaming exception to expectedException. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:25: }, description + ": '" + params[i].join("', '") + "'."); missing the function name? https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:37: assert_throws("NOT_SUPPORTED_ERR", function() { new MediaKeys("unsupported"); }); This isn't really a syntax test (nor I guess are null and undefined at the moment). Maybe we should just have a mediakeys test that has all these syntax tests. WDYT? https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:44: mediaKeys = new MediaKeys("org.w3.clearkey", extraParam); s/extraParam/"extra"? Do we need a variable? https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:51: [], Is this much more readable than l33-37? Especially since these are syntax tests, it seems better to write out the full function. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:58: ], typeError, 'Test MediaKeys createSession() type errors'); "too few parameters"? https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:64: ['video/webm', new Uint8Array(0)], We should test normal arrays, objects, numbers, etc. I imagine that's a type error. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:66: ], 'InvalidAccessError', 'Test MediaKeys createSession() invalid access errors'); Except for the last one, these are really empty array tests I think. (If not using test_syntax, we could group tests for each parameter together, which would be easier to reason about IMO.) https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:74: Should we have a basic isTypeSupported() syntax test as well? We have encrypted-media-istypesupported.html, but it doesn't appear to test syntax. (Be sure to test it against MediaKeys and mediaKeys.) https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:80: assert_equals(typeof mediaKeySession.addEventListener, 'function'); Should check MK does not have this. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:84: assert_not_equals(mediaKeySession.sessionId, null); Can we actually verify it's a string and not empty? https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:85: assert_equals(mediaKeySession.onwebkitkeyadded, undefined); Is this useful? Maybe we should test that none of the new events have "on" values. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:86: assert_equals(mediaKeySession.onwebkitkeyerror, undefined); speaking of which, we should have HTMLMediaElement syntax in here as well - setMediaKeys, mediakeys, onneedkey. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:92: ], typeError, 'Test MediaKeySession update() type errors'); too few parameters https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:93: Looking at base, I see we lose some verbosity. Good for writing tests, but bad for catching regressions in our messages. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:111: // TODO(xhwang): This causes Chromium to crash. Uncomment this once Chromium is fixed. Yay for tests! :) Change to: FIXME: ^ no name
comments addressed
Thanks! PTAL again. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right): https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:1: <!DOCTYPE html> On 2014/03/07 01:45:51, ddorwin wrote: > We'll need to rename this file in a future CL. okay. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:4: <title>Test unprefixed EME syntax</title> On 2014/03/07 01:45:51, ddorwin wrote: > "unprefixed " should be removed. This would be irrelevant if upstreamed. Done. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:8: On 2014/03/07 01:45:51, ddorwin wrote: > empty line Done. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:16: var initData = stringToUint8Array('mock'); On 2014/03/07 01:45:51, ddorwin wrote: > FWIW, 'mock' is left over from when there was a mock CDM. We can pick something > else. Done. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:19: function test_syntax(func, params, exception, description) On 2014/03/07 01:45:51, ddorwin wrote: > test_invalid_syntax? It's not possible to test valid syntax. > Why hacker style instead of testInvalidSyntax? > Now that it won't fit, it's probably worth renaming exception to > expectedException. I am not sure. I was following media source tests. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... +acolwell: I saw hacker_style in .html files, but kCamelCase in .js files for functions names. What's the rule about these? https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:25: }, description + ": '" + params[i].join("', '") + "'."); On 2014/03/07 01:45:51, ddorwin wrote: > missing the function name? The name is optional. I skip it because I don't see much value of it. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:37: assert_throws("NOT_SUPPORTED_ERR", function() { new MediaKeys("unsupported"); }); On 2014/03/07 01:45:51, ddorwin wrote: > This isn't really a syntax test (nor I guess are null and undefined at the > moment). > > Maybe we should just have a mediakeys test that has all these syntax tests. > WDYT? "null" may also not be testing syntax :) Also, it's interesting to see that createSession('video/webm', null) throws InvalidAccessError, but createSession(null, initData) throws NotSupportedError. Let's keep them here for now then move them later if necessary? https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:44: mediaKeys = new MediaKeys("org.w3.clearkey", extraParam); On 2014/03/07 01:45:51, ddorwin wrote: > s/extraParam/"extra"? Do we need a variable? Done. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:51: [], On 2014/03/07 01:45:51, ddorwin wrote: > Is this much more readable than l33-37? Especially since these are syntax tests, > it seems better to write out the full function. Done. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:58: ], typeError, 'Test MediaKeys createSession() type errors'); On 2014/03/07 01:45:51, ddorwin wrote: > "too few parameters"? Added comments. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:64: ['video/webm', new Uint8Array(0)], On 2014/03/07 01:45:51, ddorwin wrote: > We should test normal arrays, objects, numbers, etc. I imagine that's a type > error. Done. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:66: ], 'InvalidAccessError', 'Test MediaKeys createSession() invalid access errors'); On 2014/03/07 01:45:51, ddorwin wrote: > Except for the last one, these are really empty array tests I think. > > (If not using test_syntax, we could group tests for each parameter together, > which would be easier to reason about IMO.) Done. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:74: On 2014/03/07 01:45:51, ddorwin wrote: > Should we have a basic isTypeSupported() syntax test as well? > We have encrypted-media-istypesupported.html, but it doesn't appear to test > syntax. > > (Be sure to test it against MediaKeys and mediaKeys.) Yeah, will do. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:80: assert_equals(typeof mediaKeySession.addEventListener, 'function'); On 2014/03/07 01:45:51, ddorwin wrote: > Should check MK does not have this. Done. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:84: assert_not_equals(mediaKeySession.sessionId, null); On 2014/03/07 01:45:51, ddorwin wrote: > Can we actually verify it's a string and not empty? We need to wait for the "ready" if we want to check that the string is not empty. Otherwise it's an empty string. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:85: assert_equals(mediaKeySession.onwebkitkeyadded, undefined); On 2014/03/07 01:45:51, ddorwin wrote: > Is this useful? Maybe we should test that none of the new events have "on" > values. Removed. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:86: assert_equals(mediaKeySession.onwebkitkeyerror, undefined); On 2014/03/07 01:45:51, ddorwin wrote: > speaking of which, we should have HTMLMediaElement syntax in here as well - > setMediaKeys, mediakeys, onneedkey. Added FIXME. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:92: ], typeError, 'Test MediaKeySession update() type errors'); On 2014/03/07 01:45:51, ddorwin wrote: > too few parameters Done. https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:93: On 2014/03/07 01:45:51, ddorwin wrote: > Looking at base, I see we lose some verbosity. Good for writing tests, but bad > for catching regressions in our messages. Added some comments. Do we need to have description for each test (assert_*) ? https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:111: // TODO(xhwang): This causes Chromium to crash. Uncomment this once Chromium is fixed. On 2014/03/07 01:45:51, ddorwin wrote: > Yay for tests! :) > > Change to: > FIXME: > ^ no name Done.
I like this better. Thanks! https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right): https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:37: assert_throws("NOT_SUPPORTED_ERR", function() { new MediaKeys("unsupported"); }); On 2014/03/08 00:37:45, xhwang wrote: > On 2014/03/07 01:45:51, ddorwin wrote: > > This isn't really a syntax test (nor I guess are null and undefined at the > > moment). > > > > Maybe we should just have a mediakeys test that has all these syntax tests. > > WDYT? > > "null" may also not be testing syntax :) Also, it's interesting to see that > createSession('video/webm', null) throws InvalidAccessError, but > createSession(null, initData) throws NotSupportedError. Let's look at what they _should_ throw, but note that the first is a null array - whatever that becomes - and the second is the string "null". > > Let's keep them here for now then move them later if necessary? https://codereview.chromium.org/182453005/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:64: ['video/webm', new Uint8Array(0)], On 2014/03/08 00:37:45, xhwang wrote: > On 2014/03/07 01:45:51, ddorwin wrote: > > We should test normal arrays, objects, numbers, etc. I imagine that's a type > > error. > > Done. As mentioned at new line 22, I don't think we are testing an incompatible type. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right): https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:22: assert_throws('INVALID_ACCESS_ERR', function() { new MediaKeys(''); }); This is "just" testing empty string. Add some other types to: array/Uint8Array that can't be stringized; integer (though that's probably stringized). https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:26: assert_throws('NOT_SUPPORTED_ERR', function() { new MediaKeys('unsupported'); }); integer would go here per order below https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:32: mediaKeys = new MediaKeys('org.w3.clearkey'); The result of this line is not checked (other than not an exception, but it could be null). https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:37: assert_equals(typeof mediaKeys.addEventListener, 'undefined'); It would be nice to test that on{open|message|close|error} are undefined. This would catch any implementation that mistakenly added these. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:62: assert_throws('NotSupportedError', function() { mediaKeys.createSession('unsupported', initData); }); nit: Add 'video/foo' (something that looks like a valid format) https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:67: mediaKeySession = mediaKeys.createSession('video/webm', initData); ditto - result not checked https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:85: assert_throws('InvalidAccessError', function() { mediaKeySession.update(new Uint8Array(0)); }); test integer https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:100: When/where will isTypeSupported test be added?
comments addressed
PTAL again. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right): https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:22: assert_throws('INVALID_ACCESS_ERR', function() { new MediaKeys(''); }); On 2014/03/10 20:11:44, ddorwin wrote: > This is "just" testing empty string. > Add some other types to: array/Uint8Array that can't be stringized; integer > (though that's probably stringized). Done. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:26: assert_throws('NOT_SUPPORTED_ERR', function() { new MediaKeys('unsupported'); }); On 2014/03/10 20:11:44, ddorwin wrote: > integer would go here per order below Done. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:32: mediaKeys = new MediaKeys('org.w3.clearkey'); On 2014/03/10 20:11:44, ddorwin wrote: > The result of this line is not checked (other than not an exception, but it > could be null). Done. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:37: assert_equals(typeof mediaKeys.addEventListener, 'undefined'); On 2014/03/10 20:11:44, ddorwin wrote: > It would be nice to test that on{open|message|close|error} are undefined. This > would catch any implementation that mistakenly added these. Done. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:62: assert_throws('NotSupportedError', function() { mediaKeys.createSession('unsupported', initData); }); On 2014/03/10 20:11:44, ddorwin wrote: > nit: Add 'video/foo' (something that looks like a valid format) Added. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:67: mediaKeySession = mediaKeys.createSession('video/webm', initData); On 2014/03/10 20:11:44, ddorwin wrote: > ditto - result not checked Done. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:85: assert_throws('InvalidAccessError', function() { mediaKeySession.update(new Uint8Array(0)); }); On 2014/03/10 20:11:44, ddorwin wrote: > test integer Done. https://codereview.chromium.org/182453005/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:100: On 2014/03/10 20:11:44, ddorwin wrote: > When/where will isTypeSupported test be added? Added FIXME.
Thanks. LGTM with comments. https://codereview.chromium.org/182453005/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right): https://codereview.chromium.org/182453005/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:28: assert_throws('NOT_SUPPORTED_ERR', function() { new MediaKeys(new Uint8Array(0)); }); Interesting. Does a 1-element Uint8Array do the same thing? https://codereview.chromium.org/182453005/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:112: // FIXME: Add syntax checks for MediaKeys.IsTypeSupported(). Should we add FIXME: for MediaKeyError (crbug.com/351136) and Events? Or maybe just file a bug to update the latter and note we should add syntax test to both bugs.
comments addressed
https://codereview.chromium.org/182453005/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right): https://codereview.chromium.org/182453005/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:28: assert_throws('NOT_SUPPORTED_ERR', function() { new MediaKeys(new Uint8Array(0)); }); On 2014/03/11 00:30:06, ddorwin wrote: > Interesting. Does a 1-element Uint8Array do the same thing? Yes, same thing. https://codereview.chromium.org/182453005/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:112: // FIXME: Add syntax checks for MediaKeys.IsTypeSupported(). On 2014/03/11 00:30:06, ddorwin wrote: > Should we add FIXME: for MediaKeyError (crbug.com/351136) and Events? Or maybe > just file a bug to update the latter and note we should add syntax test to both > bugs. Updated FIXME. I'll fix the FIXMEs after this CL.
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/182453005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/182453005/60001
Message was sent while issue was closed.
Change committed as 168884 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
