1207498 - Remove use of expression closure from toolkit/components/.

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tooru Fujisawa [:arai]]

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).

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: MozReview Request: Bug 1207498 - Part 1: Remove use of expression closure from toolkit/components/, except tests. r?Gijs

Bug 1207498 - Part 1: Remove use of expression closure from toolkit/components/, except tests. r?Gijs

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: MozReview Request: Bug 1207498 - Part 2: Remove use of expression closure from tests in toolkit/components/. r?Gijs

Bug 1207498 - Part 2: Remove use of expression closure from tests in toolkit/components/. r?Gijs

Comment 3

[:Gijs (he/him)]

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

Comment 4

[:Gijs (he/him)]

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...

Comment 5

[Tooru Fujisawa [:arai]]

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.

Comment 6

[:Gijs (he/him)]

(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...). :-)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/036615ba3257
https://hg.mozilla.org/integration/mozilla-inbound/rev/55b45f61cfd2

Comment 8

[Tooru Fujisawa [:arai]]

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...

Comment 9

[Tooru Fujisawa [:arai]]

Forgot to link to the log

https://treeherder.mozilla.org/logviewer.html#?job_id=14770263&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=14770360&repo=mozilla-inbound

Comment 10

[Tooru Fujisawa [:arai]]

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 :)

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5188b4db877
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cce3d53cf66

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5188b4db877
https://hg.mozilla.org/mozilla-central/rev/7cce3d53cf66