ExecState should be passed by reference in JS bindings generator for custom constructors.
Created attachment 282560 [details] Patch
Created attachment 282704 [details] Patch
Created attachment 282709 [details] Patch
Comment on attachment 282709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282709&action=review Seems fine to me. Can you use more uncheckedArguments and toJSNewlyCreated before landing this patch. For the other points, it might be easier to tackle them as follow-ups. > Source/WebCore/bindings/js/JSAudioContextCustom.cpp:51 > + return throwVMError(&exec, createReferenceError(&exec, "AudioContext constructor callee is unavailable")); Can this really happen? I don't think binding generator is checking that. And neither constructors. It might be good to evaluate whether an ASSERT is good enough or not and refactor the code accordingly as a later patch. > Source/WebCore/bindings/js/JSAudioContextCustom.cpp:55 > + return throwVMError(&exec, createReferenceError(&exec, "AudioContext constructor script execution context is unavailable")); This error message is not consistent with binding generated code (see for instance JSXMLHttpRequest.cpp) which uses throwConstructorDocumentUnavailableError Can we use it instead? > Source/WebCore/bindings/js/JSAudioContextCustom.cpp:72 > if (!audioContext.get()) Remove .get() ? > Source/WebCore/bindings/js/JSAudioContextCustom.cpp:85 > + float sampleRate = exec.argument(2).toFloat(&exec); Can you use uncheckedArgument here? > Source/WebCore/bindings/js/JSAudioContextCustom.cpp:109 > + return throwVMError(&exec, createReferenceError(&exec, "Error creating AudioContext")); This line is probably only relevant for the else block. Maybe want to move it into that block. > Source/WebCore/bindings/js/JSBlobCustom.cpp:65 > + DOMConstructorObject* jsConstructor = jsCast<DOMConstructorObject*>(exec.callee()); We do not check whether jsConstructor is null or not. Might want to add an ASSERT. Related to first point above. > Source/WebCore/bindings/js/JSBlobCustom.cpp:75 > + JSObject* blobParts = toJSSequence(&exec, exec.argument(0), blobPartsLength); Use uncheckedArgument? > Source/WebCore/bindings/js/JSBlobCustom.cpp:84 > + JSValue blobPropertyBagValue = exec.argument(1); Use uncheckedArgument? > Source/WebCore/bindings/js/JSBlobCustom.cpp:127 > + String string = item.toString(&exec)->value(&exec); Maybe use toWTFString? > Source/WebCore/bindings/js/JSDOMFormDataCustom.cpp:55 > + return JSValue::encode(asObject(toJS(&exec, jsConstructor->globalObject(), domFormData))); We should probably use toJSNewlyCreated. > Source/WebCore/bindings/js/JSDataCueCustom.cpp:56 > + double startTime(exec.argument(0).toNumber(&exec)); Use uncheckedArgument? > Source/WebCore/bindings/js/JSDataCueCustom.cpp:60 > + double endTime(exec.argument(1).toNumber(&exec)); Ditto. > Source/WebCore/bindings/js/JSDataCueCustom.cpp:70 > + if (exec.argumentCount() > 3) { Ditto. > Source/WebCore/bindings/js/JSDataCueCustom.cpp:77 > + JSValue valueArgument = exec.argument(2); Ditto. > Source/WebCore/bindings/js/JSDataCueCustom.cpp:97 > + return JSValue::encode(asObject(toJS(&exec, castedThis->globalObject(), object.get()))); We should probably use toJSNewlyCreated in lieu of toJS. > Source/WebCore/bindings/js/JSDataCueCustom.cpp:104 > + return JSValue::encode(asObject(toJS(&exec, castedThis->globalObject(), object.get()))); Ditto and remove object variable if possible. > Source/WebCore/bindings/js/JSFileCustom.cpp:62 > + return throwVMError(&exec, createTypeError(&exec, "Second argument to File constructor must be a valid string, was undefined")); I am not sure the error message names are consistent with binding generated code here. > Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:41 > +EncodedJSValue JSC_HOST_CALL constructJSHTMLElement(ExecState& exec) Ah, I see you are renaming state into exec :) I do not have strong feelings on this one but since there is no consensus, it might be best to not change anything here. Except maybe to make the file consistent on its own. > Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:43 > + auto* jsConstructor = jsCast<DOMConstructorObject*>(exec.callee()); Adding ASSERT maybe. > Source/WebCore/bindings/js/JSHTMLElementCustom.cpp:47 > + return throwConstructorDocumentUnavailableError(exec, "HTMLElement"); This is what the binding generated code looks like. Maybe we should use that method in all methods above. > Source/WebCore/bindings/js/JSMediaSessionCustom.cpp:-44 > - auto* castedThis = jsCast<DOMConstructorObject*>(exec->callee()); Ditto for an ASSERT > Source/WebCore/bindings/js/JSMediaSessionCustom.cpp:51 > + JSString* kindString = exec.argument(0).toString(&exec); Use uncheckedArgument? > Source/WebCore/bindings/js/JSMutationObserverCustom.cpp:50 > + JSObject* object = exec.argument(0).getObject(); uncheckedArgument > Source/WebCore/bindings/js/JSMutationObserverCustom.cpp:57 > + JSObject* jsObserver = asObject(toJS(&exec, jsConstructor->globalObject(), MutationObserver::create(WTFMove(callback)))); Probably can use toJSNewlyCreated. > Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:37 > +EncodedJSValue JSC_HOST_CALL constructJSReadableStreamController(ExecState& exec) Ditto for exec/state > Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:46 > + return throwVMError(&exec, createTypeError(&exec, ASCIILiteral("ReadableStreamReader constructor parameter is not a ReadableStream"))); We could probably make the error message consistent here. > Source/WebCore/bindings/js/JSWebKitPointCustom.cpp:42 > + x = static_cast<float>(exec.argument(0).toNumber(&exec)); uncheckedArgument > Source/WebCore/bindings/js/JSWebKitPointCustom.cpp:49 > + return JSValue::encode(asObject(toJS(&exec, jsConstructor->globalObject(), WebKitPoint::create(x, y)))); Use toJSNewlyCreated? > Source/WebCore/bindings/js/JSWorkerCustom.cpp:70 > + return JSValue::encode(asObject(toJS(&exec, jsConstructor->globalObject(), *worker))); Use toJSNewlyCreated?
> Ah, I see you are renaming state into exec :) > I do not have strong feelings on this one but since there is no consensus, it might be best to not change anything here. Except maybe to make the file consistent on its own. While there is no formal consensus, as do have strong feelings about this. State is a poor variable name in general. Either "exec" or "execState" is better. I support the rename in places touched for any other reason, even if it leaves the file inconsistent. If we decide to formalize a style on it, we can do the rest of the project in one big shot.
Created attachment 283009 [details] Patch
Thanks for the reviews. I uploaded a new patch with the modifications proposed in the reviews. The patch is bigger and modifies tests and expectations. Is it OK to land this patch as this or should it be split in two smaller patches? The new patch would address only error messages consistency in custom constructor and binding generated code. > State is a poor variable name in general. Either "exec" or "execState" is better. It seems that ExecState handle execution context. Is "context" or "ctx" or "execCtx" better ?
Comment on attachment 283009 [details] Patch Attachment 283009 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1641515 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 283012 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 283009 [details] Patch Attachment 283009 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1641519 New failing tests: storage/indexeddb/modern/blob-simple-workers.html http/tests/websocket/tests/hybi/workers/send-blob.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob-worker.html
Created attachment 283013 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 283009 [details] Patch Attachment 283009 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1641521 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 283014 [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
Comment on attachment 283009 [details] Patch Attachment 283009 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1641522 New failing tests: http/tests/websocket/tests/hybi/workers/send-blob.html imported/w3c/web-platform-tests/fetch/api/basic/scheme-blob-worker.html
Created attachment 283016 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Failed tests seem due to the fact that ScriptExecutionContext associated to JSBlob is not always a Document. Adding the possibility for ScriptExecutionContext to be a WorkerGlobalScope seems to fix test errors. I'm not very familiar with ScriptExecutionContext. Should other cases be considered?
(In reply to comment #16) > Failed tests seem due to the fact that ScriptExecutionContext associated to > JSBlob is not always a Document. Adding the possibility for > ScriptExecutionContext to be a WorkerGlobalScope seems to fix test errors. > > I'm not very familiar with ScriptExecutionContext. Should other cases be > considered? Blob is exposed both to worker and regular window. ScriptExecutionContext is an abstraction to cover both cases. You should be safe covering these two cases.
Created attachment 283123 [details] Patch
(In reply to comment #17) > Blob is exposed both to worker and regular window. > ScriptExecutionContext is an abstraction to cover both cases. > You should be safe covering these two cases. Ok, thanks. I will fix this in a follow-up bug as you proposed in your first comment. Modifications made in the uploaded patch consists in more uses of toJSNewlyCreated and uncheckedArguments and in making ExecState* parameter name more consistent in JSDOMFormDataCustom.cpp.
Comment on attachment 283123 [details] Patch R=me. Sounds good to split the issues in different bugs. A couple of nits below. View in context: https://bugs.webkit.org/attachment.cgi?id=283123&action=review > Source/WebCore/bindings/js/JSDOMFormDataCustom.cpp:55 > + return JSValue::encode(asObject(toJSNewlyCreated(&exec, jsConstructor->globalObject(), WTFMove(domFormData)))); No need for asObject here. > Source/WebCore/bindings/js/JSDataCueCustom.cpp:93 > + return JSValue::encode(asObject(toJSNewlyCreated(&exec, castedThis->globalObject(), Ref<TextTrackCue>(DataCue::create(*context, MediaTime::createWithDouble(startTime), MediaTime::createWithDouble(endTime), *data, type))))); Ditto here. You might want to use CREATE_DOM_WRAPPER to remove the cast. > Source/WebCore/bindings/js/JSDataCueCustom.cpp:99 > + return JSValue::encode(asObject(toJSNewlyCreated(&exec, castedThis->globalObject(), Ref<TextTrackCue>(DataCue::create(*context, MediaTime::createWithDouble(startTime), MediaTime::createWithDouble(endTime), valueArgument, type))))); Ditto > Source/WebCore/bindings/js/JSMediaSessionCustom.cpp:61 > + return JSValue::encode(asObject(toJSNewlyCreated(&exec, castedThis->globalObject(), MediaSession::create(*context, kind)))); Remove asObject is probably not needed. > Source/WebCore/bindings/js/JSWorkerCustom.cpp:55 > + String scriptURL = exec.argument(0).toWTFString(&exec); uncheckedArgument here probably.
Created attachment 283163 [details] Patch for landing
Comment on attachment 283163 [details] Patch for landing Clearing flags on attachment: 283163 Committed r202981: <http://trac.webkit.org/changeset/202981>
All reviewed patches have been landed. Closing bug.