Extra arguments for JavaScript Constructors should come first

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Anna Cavender]

Especially for JavaScript Constructors that have optional arguments, having the extra parameters (ScriptExecutionContext and ExceptionCode) come at the end of the argument list is inconvenient and forces authors to make these important arguments optional when likely they are not.

[Anna Cavender]

Created attachment 110442 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 110442 [details]
Patch

Attachment 110442 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10025131

[Adam Barth]

Comment on attachment 110442 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110442&action=review

> Source/WebCore/css/WebKitCSSMatrix.h:42
> -    static PassRefPtr<WebKitCSSMatrix> create(const String& s, ExceptionCode& ec)
> +    static PassRefPtr<WebKitCSSMatrix> create(ExceptionCode& ec, const String& s)

Usually the ExceptionCode is the last argument.

[Early Warning System Bot]

Comment on attachment 110442 [details]
Patch

Attachment 110442 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10028061

[Anna Cavender]

haraken, I'm adding you as you seem to be working in this area too.  I'm open to suggestions.

[Darin Adler]

Who are these authors you are talking about, who have to make these arguments optional?

This change looks like a V8-only change. What about the original WebKit JavaScript engine?

[Kentaro Hara]

(In reply to comment #5)
> haraken, I'm adding you as you seem to be working in this area too.  I'm open to suggestions.

I agree to this change. I am waiting for this patch landed!

- I am afraid that this change will break JSC build. You need to change the corresponding custom constructors in JSC too.

- I think it is OK to make changes only for WebKitCSSMatrix, EventSource and Worker in this patch, but please note that there are other custom constructors that are putting ScriptExecutionContext as the last argument. (As far as I see, all the custom constructors that are using ScriptExecutionContext are putting it as the last argument.) Please make a follow-up patch for fixing it. (If you do not want to do it, I can do it instead.)

[Darin Adler]

I’m sorry, this does not look right to me. Why does it matter whether these arguments are first or last? The arguments are passed from generated code; the arguments should not be optional.

[Anna Cavender]

Thanks everyone for the feedback!

As for why authors might need optional arguments, I suppose I am suggesting this for selfish reasons as I will need to do something like this:
cue = new TextTrackCue( id, startTime, endTime, text [, settings [, pauseOnExit ] ] )
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#texttrackcue

As for other JavaScript engines, yes I see now that I need to do that.  haraken, I'd love some help with that as I won't know what other classes will be broken by this patch beyond the ones that use V8.

[Adam Barth]

> Why does it matter whether these arguments are first or last? The arguments are passed from
> generated code; the arguments should not be optional.

These context objects can be added by the code generator both for constructors and for normal function members.  For normal function members, we pass them in at the beginning.  For constructor functions, we pass them in at the end.

This patch is mostly about consistency between these two approaches.  We've been moving WebKit's IDL to be closer to WebIDL, which treats the constructor more like a normal function (e.g., you can specify the types of the arguments like you would for a normal function).

[Kentaro Hara]

(In reply to comment #10)
> > Why does it matter whether these arguments are first or last? The arguments are passed from
> > generated code; the arguments should not be optional.
> 
> These context objects can be added by the code generator both for constructors and for normal function members.  For normal function members, we pass them in at the beginning.  For constructor functions, we pass them in at the end.
> 
> This patch is mostly about consistency between these two approaches.  We've been moving WebKit's IDL to be closer to WebIDL, which treats the constructor more like a normal function (e.g., you can specify the types of the arguments like you would for a normal function).

I agree to the idea. I think that it is possible to solve the current problem by setting [CallWithDefaultValue] or [CallWithNullValue] for |settings| and |pauseOnExit|, but moving ScriptExecutionContext to the beginning makes more sense for consistency.

[Kentaro Hara]

(In reply to comment #9)
> As for other JavaScript engines, yes I see now that I need to do that.  haraken, I'd love some help with that as I won't know what other classes will be broken by this patch beyond the ones that use V8.

OK, please make changes for WebKitCSSMatrix, EventSource and Worker in this patch. I think that this change won't break anything, since the constructors that are putting ScriptExecutionContext at the end and are using CodeGeneratorV8.pm are only the above three. I will make a follow-up patch for making the similar changes for other custom constructors that are not using CodeGeneratorV8.pm.

[Anna Cavender]

Created attachment 110543 [details]
Patch

[Anna Cavender]

Created attachment 110558 [details]
Patch

including changes to test files

[Adam Barth]

Comment on attachment 110558 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110558&action=review

Assuming this builds, this looks great.

> Source/WebCore/ChangeLog:4
> +        IDL constructors that use ConstrcutorWith=ScriptExecutionContext.

ConstrcutorWith <-- typo

[Anna Cavender]

Created attachment 110701 [details]
Patch for landing

[Adam Barth]

Comment on attachment 110701 [details]
Patch for landing

Clearing flags on attachment: 110701

Committed r97287: <http://trac.webkit.org/changeset/97287>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.