These tests were added in http://trac.webkit.org/changeset/113736 and check that the error's code attribute is set to DOMException.TYPE_ERR. However, I don't think they're doing what was intended. In both JSC and V8, DOMException.TYPE_ERR is undefined. I think the correct name of the error is DOMException.TYPE_MISMATCH_ERR. The tests pass because neither JSC nor V8 set the error attribute when creating errors of this type. They do however set the attribute for other types of errors. I'm not sure whether or not this is a bug in the bindings, though I suspect not. We should fix the test and perhaps the bindings too.
Created attachment 194953 [details] Patch
Comment on attachment 194953 [details] Patch David, as author of the tests, do you know whether it's expected that the exception error code is not set in this case?
(In reply to comment #2) > (From update of attachment 194953 [details]) > David, as author of the tests, do you know whether it's expected that the exception error code is not set in this case? I'm not sure what you mean. I expected two few parameters to throw an exception. That's really all that matters. View in context: https://bugs.webkit.org/attachment.cgi?id=194953&action=review > LayoutTests/media/video-test.js:211 > + logResult(code === DOMException.TYPE_MISMATCH_ERR || code === ex.code, What does the first comparison do to prevent the problem?
> I'm not sure what you mean. I expected two few parameters to throw an exception. That's really all that matters. In addition to checking that an exception is thrown, the tests check the value of the exception's code attribute. In some of these tests, the expected value is DOMException.TYPE_ERR, but this constant is undefined. These tests only pass because the exception's code attribute is also undefined. Presumably, this isn't intentional? I think the expected value should be DOMException.TYPE_MISMATCH_ERR. My question is whether the exception's code attribute should be undefined in this case or whether it should be set to this value.
(In reply to comment #4) > > I'm not sure what you mean. I expected two few parameters to throw an exception. That's really all that matters. > In addition to checking that an exception is thrown, the tests check the value of the exception's code attribute. In some of these tests, the expected value is DOMException.TYPE_ERR, but this constant is undefined. These tests only pass because the exception's code attribute is also undefined. Presumably, this isn't intentional? > > I think the expected value should be DOMException.TYPE_MISMATCH_ERR. My question is whether the exception's code attribute should be undefined in this case or whether it should be set to this value. I'm not sure what the correct behavior is or if the current behavior (below) is expected or a bug in the implementation. I'd defer to someone with more experience in this area. It appears that this is not actually a DOMException that gets thrown. Chrome (and maybe other ports) does not throw TYPE_MISMATCH_ERR for these values. It reports "TypeError: Not enough arguments", which does not have a value like the other tests. This is one of several other test that expects the same behavior: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/storage/storageinfo-missing-arguments.html&l=9 Below are a few commands and results run in Chrome's Developer Tools Console. video.webkitGenerateKeyRequest() TypeError: Not enough arguments video.webkitAddKey('webkit-org.w3.clearkey', '') Error: SyntaxError: DOM Exception 12 video.webkitAddKey('webkit-org.w3.clearkey', new Uint8Array()) Error: TypeMismatchError: DOM Exception 17
> It appears that this is not actually a DOMException that gets thrown. I think you're right. In the case where insufficient arguments are supplied, a v8::Exception::TypeError is thrown, which doesn't have a code attribute. The code attribute is present only on DOMExceptions. I think the right fix is to rename the existing testException() function to testDomException(), and add a new testException() function (similar to shouldThrow() from js-test-pre.js) for the tests with insufficient arguments.
Created attachment 195689 [details] Patch
Created attachment 195690 [details] Patch
Comment on attachment 195690 [details] Patch Seems ok. You mentioned V8's behavior. Does JSC also throw a TypeError with the same string? These tests also run on Mac. View in context: https://bugs.webkit.org/attachment.cgi?id=195690&action=review > LayoutTests/media/video-test.js:208 > eval(testString); To help avoid and detect accidental bad expected files, should we logResult("Failed to receive exception for....")? Same at 217.
Created attachment 196270 [details] Patch
> Seems ok. You mentioned V8's behavior. Does JSC also throw a TypeError with the > same string? These tests also run on Mac. Yes, it does. > To help avoid and detect accidental bad expected files, should we logResult("Failed to receive exception for....")? Done
+abarth for review, who originally reviewed these tests
Comment on attachment 196270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196270&action=review > LayoutTests/media/video-test.js:212 > + logResult(exception instanceof DOMException && exception.code === eval(exceptionString), |exception| could be undefined here if the |testString| did not throw. It seems that would lead to a confusing failure message. Would it be simpler to just logResult("FAILED: " + testString + " did not throw " + exceptionString) after 208 and move this back into the catch?
Comment on attachment 196270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196270&action=review >> LayoutTests/media/video-test.js:212 >> + logResult(exception instanceof DOMException && exception.code === eval(exceptionString), > > |exception| could be undefined here if the |testString| did not throw. It seems that would lead to a confusing failure message. > Would it be simpler to just logResult("FAILED: " + testString + " did not throw " + exceptionString) after 208 and move this back into the catch? I don't think the failure message would be confusing: logResult will append 'FAIL' to the text below. I prefer this approach as it means we only need a single call to logResult() and hence a single description of the test.
Comment on attachment 196270 [details] Patch Ok. Thanks.
Comment on attachment 196270 [details] Patch Clearing flags on attachment: 196270 Committed r147969: <http://trac.webkit.org/changeset/147969>
All reviewed patches have been landed. Closing bug.