1151472 - Remove use of expression closures in calendar/

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Tooru Fujisawa [:arai]]

Similar to bug 1123124.

Need to replace non-standard expression closure `function() expr` with one of function declaration, function expression, and arrow function, before fixing bug 1083458.

I have draft patch for this.

Comment 1

[Tooru Fujisawa [:arai]]

try is running
  https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=92a106d31b78

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Remove use of expression closure in calendar/.

converting rules are following (same as bug 1123124):
  * function declaration
    add `return` and braces
  * standalone function expression contains and receives `this` (Array.map, bind, etc)
    convert to arrow function
  * standalone function expression contains no `this`
    convert to arrow function
  * method-like property contains `this`
    add `return` and braces
  * method-like property contains no `this` with short body
    convert to arrow function
  * method-like property contains no `this` with long body
    add `return` and braces
  * method-like property with named function expression
    add `return` and braces
  * getter property
    add `return` and braces
  * setter property
    add braces

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8647921 [details] [diff] [review]
Remove use of expression closure in calendar/.

Review of attachment 8647921 [details] [diff] [review]:
-----------------------------------------------------------------

r=philipp with these comments fixed/considered:

::: calendar/base/backend/icaljs/calDateTime.js
@@ +55,3 @@
>  
> +    get timezone() { return new calICALJSTimezone(this.innerObject.zone); },
> +    set timezone(val) { unwrapSetter(ICAL.Timezone, val, function(val) {

I always thought it was mandatory to have the setter return a value so that chaining works. I tried to create a testcase in scratchpad but I didn't succeed. Do you know if this changed and is no longer needed? If it is, please also return in the setters.

::: calendar/providers/gdata/components/calGoogleCalendar.js
@@ +127,5 @@
>          if (aUri && aUri.schemeIs("googleapi")) {
>              // new format:  googleapi://session-id/?calendar=[e-mail]&tasks=taskhash
>              let [fullUser, path] = aUri.path.substr(2).split("/", 2);
>              let parameters = new Map(path.substr(1).split("&").filter(Boolean)
> +                             .map(x => x.split("=", 2).map(decodeURIComponent)));

For the gdata provider, I'd appreciate if you could not use arrow functions. It is currently compatible with gecko 8 (!) so it can be used in Postbox.

Comment 4

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

Pushed to c-c.
https://hg.mozilla.org/comm-central/rev/ab6c50b76ed3

(In reply to Philipp Kewisch [:Fallen] from comment #3)
> ::: calendar/base/backend/icaljs/calDateTime.js
> @@ +55,3 @@
> >  
> > +    get timezone() { return new calICALJSTimezone(this.innerObject.zone); },
> > +    set timezone(val) { unwrapSetter(ICAL.Timezone, val, function(val) {
> 
> I always thought it was mandatory to have the setter return a value so that
> chaining works. I tried to create a testcase in scratchpad but I didn't
> succeed. Do you know if this changed and is no longer needed? If it is,
> please also return in the setters.

I don't know about the history tho, now ES6 spec says that the setter's return value is not used (unless the function is directly called).

http://www.ecma-international.org/ecma-262/6.0/#sec-ordinary-object-internal-methods-and-internal-slots-set-p-v-receiver
>  9. Let setterResult be Call(setter, Receiver, «V»).
> 10. ReturnIfAbrupt(setterResult).
> 11. Return true.

[[Set]] internal method returns the boolean value, not setter function's result.

http://www.ecma-international.org/ecma-262/6.0/#sec-assignment-operators-runtime-semantics-evaluation
> 1.f. Let status be PutValue(lref, rval).
> 1.g. ReturnIfAbrupt(status).
> 1.h. Return rval.
and
> 6. Let status be the result of performing DestructuringAssignmentEvaluation of assignmentPattern using rval as the argument.
> 7. ReturnIfAbrupt(status).
> 8. Return rval.

Also, here, assignment expression's value is the value of right-hand-side expression, not left-hand-side.

> ::: calendar/providers/gdata/components/calGoogleCalendar.js
> @@ +127,5 @@
> >          if (aUri && aUri.schemeIs("googleapi")) {
> >              // new format:  googleapi://session-id/?calendar=[e-mail]&tasks=taskhash
> >              let [fullUser, path] = aUri.path.substr(2).split("/", 2);
> >              let parameters = new Map(path.substr(1).split("&").filter(Boolean)
> > +                             .map(x => x.split("=", 2).map(decodeURIComponent)));
> 
> For the gdata provider, I'd appreciate if you could not use arrow functions.
> It is currently compatible with gecko 8 (!) so it can be used in Postbox.

wow, gecko 8!!!
Okay, I've changed them to function expression.