Make custom constructors consistent with generated bindings code. In particular, error messages used in case of argument error are different.
Created attachment 283312 [details] Patch
Comment on attachment 283312 [details] Patch This seems pretty good. Some comments below. View in context: https://bugs.webkit.org/attachment.cgi?id=283312&action=review > Source/WebCore/bindings/js/JSBlobCustom.cpp:71 > + if (!is<Document>(context) && !is<WorkerGlobalScope>(context)) I think you can keep "if(!context)" since that should be equivalent. > Source/WebCore/bindings/js/JSDOMBinding.cpp:824 > +JSC::EncodedJSValue throwConstructorExecutionContextUnavailableError(JSC::ExecState& state, const char* interfaceName) I know it is a bit long but then we should use ScriptExecutionContext and not ExecutionContext. > Source/WebCore/bindings/js/JSFileCustom.cpp:52 > + return throwConstructorExecutionContextUnavailableError(exec, "File"); I think the generator is doing something like: if(!context) throw ASSERT(context.isDocument()) File is not supposed to be exposed to workers, so if context is not a Document, we are probably in serious trouble and we should assert. > Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:48 > + return throwConstructorExecutionContextUnavailableError(exec, "HTMLElement"); Ditto here
> I know it is a bit long but then we should use ScriptExecutionContext and > not ExecutionContext. Thanks for the review. Ok, I will fix this a new patch. > > Source/WebCore/bindings/js/JSFileCustom.cpp:52 > > + return throwConstructorExecutionContextUnavailableError(exec, "File"); > > I think the generator is doing something like: > if(!context) > throw > ASSERT(context.isDocument()) > > File is not supposed to be exposed to workers, so if context is not a > Document, we are probably in serious trouble and we should assert. It seems that a ReferenceError should be thrown when File is evaluated in a global scope of a worker as per specification (c.f. example section of http://heycam.github.io/webidl/#Exposed). This behavior seems only described in this example section so maybe there is no strong requirement to throw. Wdyt?
> > File is not supposed to be exposed to workers, so if context is not a > > Document, we are probably in serious trouble and we should assert. > It seems that a ReferenceError should be thrown when File is evaluated in a > global scope of a worker as per specification (c.f. example section of > http://heycam.github.io/webidl/#Exposed). This behavior seems only described > in this example section so maybe there is no strong requirement to throw. > Wdyt? Right, but this should probably be handled above. If we are executing constructJSFile, we know that "File" is defined in this scope and therefore we must be in a window scope.
Created attachment 283498 [details] WIP
> Right, but this should probably be handled above. > If we are executing constructJSFile, we know that "File" is defined in this > scope and therefore we must be in a window scope. OK I understand. Thanks. I uploaded a new patch with new assertions when context is not a Document and renamed throwConstructorExecutionContextUnavailableError.
Comment on attachment 283498 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=283498&action=review > Source/WebCore/bindings/js/JSAudioContextCustom.cpp:53 > + if (!is<Document>(scriptExecutionContext)) Here also, you might want to keep if (!scriptExecutionContext) and add an ASSERT
Created attachment 283506 [details] Patch
Created attachment 283518 [details] Patch
Created attachment 283542 [details] Patch
Comment on attachment 283542 [details] Patch Attachment 283542 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1675344 New failing tests: storage/indexeddb/modern/blob-simple-workers.html http/tests/websocket/tests/hybi/workers/send-blob.html fast/files/workers/worker-read-blob-async.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob-worker.html fast/files/workers/worker-read-blob-sync.html
Created attachment 283554 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 284107 [details] Patch
Created attachment 286857 [details] Patch
r=me
Comment on attachment 286857 [details] Patch Clearing flags on attachment: 286857 Committed r205008: <http://trac.webkit.org/changeset/205008>
All reviewed patches have been landed. Closing bug.