The default bug view has changed. See this FAQ.

Remove use of expression closures in calendar/

RESOLVED FIXED in 4.5

Status

Calendar
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks: 1 bug)

unspecified

Details

Attachments

(1 attachment)

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.
try is running
  https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=92a106d31b78
Created attachment 8647921 [details] [diff] [review]
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
Assignee: nobody → arai.unmht
Attachment #8647921 - Flags: review?(philipp)
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=calhash@group.calendar.google.com&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.
Attachment #8647921 - Flags: review?(philipp) → review+
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=calhash@group.calendar.google.com&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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Target Milestone: --- → 4.5
You need to log in before you can comment on or make changes to this bug.