Last Comment Bug 1151472 - Remove use of expression closures in calendar/
: Remove use of expression closures in calendar/
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
-- normal (vote)
: 4.5
Assigned To: Tooru Fujisawa [:arai]
:
:
Mentors:
Depends on:
Blocks: 1083458
  Show dependency treegraph
 
Reported: 2015-04-06 08:21 PDT by Tooru Fujisawa [:arai]
Modified: 2015-08-16 02:21 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Remove use of expression closure in calendar/. (132.84 KB, patch)
2015-08-14 01:00 PDT, Tooru Fujisawa [:arai]
philipp: review+
Details | Diff | Splinter Review

Description User image Tooru Fujisawa [:arai] 2015-04-06 08:21:46 PDT
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 User image Tooru Fujisawa [:arai] 2015-08-13 13:59:44 PDT
try is running
  https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=92a106d31b78
Comment 2 User image Tooru Fujisawa [:arai] 2015-08-14 01:00:41 PDT
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
Comment 3 User image Philipp Kewisch [:Fallen] 2015-08-15 08:47:54 PDT
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.
Comment 4 User image Tooru Fujisawa [:arai] 2015-08-15 14:53:17 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.