Remove use of expression closure from toolkit/components/.

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: arai, Unassigned)

Tracking

unspecified
Firefox 44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments)

Need to replace non-standard expression closure with one of:
  * function declaration
  * function expression
  * arrow function
before fixing bug 1083458.

converting rules are following:
  * function declaration
    add `return` and braces
  * standalone named function expression
    add `return` and braces
  * standalone anonymous function expression contans and receives `this` (Array.filter, bind, etc)
    convert to arrow function, and remove code passing |this|
  * standalone anonymous function expression contans no `this`
    convert to arrow function
  * property with anonymous function expression, contains `this`
    add `return` and braces
  * property with anonymous function expression, contains no `this`, short body
    convert to arrow function
  * property with anonymous function expression, contains no `this`, long body
    add `return` and braces
  * property with named function expression
    add `return` and braces
  * getter property
    add `return` and braces
  * setter property
    add braces

Since there are a lot of patches, separated into 8 bugs, each bug corresponds to one of following directories:
  * browser/, except browser/components/.
  * browser/components/.
  * dom/.
  * layout/.
  * services/.
  * toolkit/, except toolkit/components/.
  * toolkit/components/.
  * b2g/, chrome/, docshell/, mobiles/, modules/, netwerk/, parser/, security/, storage/, testing/, webapprt/, widget/, xpcom/
(not yet touched addon-sdk)

I have draft patches, will post them (may take some time to prepare and post).
Bug 1207498 - Part 1: Remove use of expression closure from toolkit/components/, except tests. r?Gijs
Attachment #8665774 - Flags: review?(gijskruitbosch+bugs)
Bug 1207498 - Part 2: Remove use of expression closure from tests in toolkit/components/. r?Gijs
Attachment #8665775 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8665774 [details]
MozReview Request: Bug 1207498 - Part 1: Remove use of expression closure from toolkit/components/, except tests. r?Gijs

https://reviewboard.mozilla.org/r/20331/#review18293
Attachment #8665774 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8665775 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8665775 [details]
MozReview Request: Bug 1207498 - Part 2: Remove use of expression closure from tests in toolkit/components/. r?Gijs

https://reviewboard.mozilla.org/r/20333/#review18295

::: toolkit/components/places/tests/autocomplete/test_special_search.js:81
(Diff revision 1)
> -   "foo |", [1,2,3,5,10], function() { changeRestrict("history", "|"); }],
> +   "foo |", [1,2,3,5,10], () => changeRestrict("history", "|")],

Hm, this doesn't need changing, does it? And you're changing the meaning in that the current code doesn't return the result of the changeRestrict call, and your new arrow function would.

Of course, all the other tests here *do* return the rv, so it probably makes sense to do assuming the test is green on try with this change...
Thank you for reviewing :D

forgot to note that.  I changed that line to make it consistent with others.
That function is called here:
https://dxr.mozilla.org/mozilla-central/rev/001942e4617b2324bfa6cdfb1155581cbc3f0cc4/toolkit/components/places/tests/autocomplete/head_autocomplete.js?offset=0#267
Returned value is not used.
(In reply to Tooru Fujisawa [:arai] from comment #5)
> Thank you for reviewing :D
> 
> forgot to note that.  I changed that line to make it consistent with others.
> That function is called here:
> https://dxr.mozilla.org/mozilla-central/rev/
> 001942e4617b2324bfa6cdfb1155581cbc3f0cc4/toolkit/components/places/tests/
> autocomplete/head_autocomplete.js?offset=0#267
> Returned value is not used.

Perfect. all good assuming this is green on try (or you are feeling confident today...). :-)
Backed out for Linux pgo M(oth) failure
https://hg.mozilla.org/integration/mozilla-inbound/rev/193ef96bd824
https://hg.mozilla.org/integration/mozilla-inbound/rev/f76692f0fcf8

Not sure how pgo can interact with JS change...
It fails even after the backout:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8796f04aea96
There is also same failure before my patch:
https://treeherder.mozilla.org/logviewer.html#?job_id=14609307&repo=mozilla-inbound

I'll re-land after tree open :)
https://hg.mozilla.org/mozilla-central/rev/b5188b4db877
https://hg.mozilla.org/mozilla-central/rev/7cce3d53cf66
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.