Bug 159550 – Make custom constructors consistent with generated bindings code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nael Ouedraogo]

Make custom constructors consistent with generated bindings code. In particular, error messages used in case of argument error are different.

[Nael Ouedraogo]

Created attachment 283312 [details]
Patch

[youenn fablet]

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

[Nael Ouedraogo]

> 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?

[youenn fablet]

> > 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.

[Nael Ouedraogo]

Created attachment 283498 [details]
WIP

[Nael Ouedraogo]

> 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.

[youenn fablet]

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

[Nael Ouedraogo]

Created attachment 283506 [details]
Patch

[Nael Ouedraogo]

Created attachment 283518 [details]
Patch

[Nael Ouedraogo]

Created attachment 283542 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Nael Ouedraogo]

Created attachment 284107 [details]
Patch

[Nael Ouedraogo]

Created attachment 286857 [details]
Patch

[youenn fablet]

r=me

[WebKit Commit Bot]

Comment on attachment 286857 [details]
Patch

Clearing flags on attachment: 286857

Committed r205008: <http://trac.webkit.org/changeset/205008>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.