1207490 - Remove use of expression closure from browser/, except browser/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: Part 1: Remove use of expression closure from browser/base/.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove use of expression closure from browser/base/content/test/general/.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove use of expression closure from browser/base/content/test/newtab/.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Remove use of expression closure from browser/base/content/test/plugins/.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Remove use of expression closure from browser/base/content/test/social/.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Remove use of expression closure from browser/devtools/.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Remove use of expression closure from browser/experiments/.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Remove use of expression closure from browser/fuel/.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Remove use of expression closure from browser/modules/BrowserUITelemetry.jsm.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Remove use of expression closure from browser/modules/ContentLinkHandler.jsm.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Remove use of expression closure from browser/modules/DirectoryLinksProvider.jsm.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 12: Remove use of expression closure from browser/modules/Social.jsm.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 13: Remove use of expression closure from browser/modules/Windows8WindowFrameColor.jsm.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 14: Remove use of expression closure from browser/modules/WindowsJumpLists.jsm.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 15: Remove use of expression closure from browser/modules/WindowsPreviewPerTab.jsm.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 16: Remove use of expression closure from browser/modules/test/.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8664722 [details] [diff] [review]
Part 8: Remove use of expression closure from browser/fuel/.

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

::: browser/fuel/fuelApplication.js
@@ +17,5 @@
>  var Utilities = {
>    get bookmarks() {
>      let bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>                      getService(Ci.nsINavBookmarksService);
> +    this.__defineGetter__("bookmarks", () => bookmarks);

nit: here and elsewhere you can use _ => ...

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8664714 [details] [diff] [review]
Part 1: Remove use of expression closure from browser/base/.

>@@ -5193,19 +5195,25 @@ var TabsInTitlebar = {
>   _lastSizeMode: null,
> 
>   _readPref: function () {
>     this.allowedBy("pref",
>                    Services.prefs.getBoolPref(this._prefName));
>   },
> 
>   _update: function (aForce=false) {
>-    function $(id) document.getElementById(id);
>-    function rect(ele) ele.getBoundingClientRect();
>-    function verticalMargins(cstyle) parseFloat(cstyle.marginBottom) + parseFloat(cstyle.marginTop);
>+    function $(id) {
>+      return document.getElementById(id);
>+    }
>+    function rect(ele) {
>+      return ele.getBoundingClientRect();
>+    }
>+    function verticalMargins(cstyle) {
>+      return parseFloat(cstyle.marginBottom) + parseFloat(cstyle.marginTop);
>+    }

I think I'd prefer 'let $ = id => document.getElementById(id);' etc. here

Comment 19

[Tooru Fujisawa [:arai]]

thank you for reviewing :)

here's try run result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c844b363eef3
some oranges, but there seems no regression from this patch series

Comment 20

[:Gijs (he/him)]

Comment on attachment 8664721 [details] [diff] [review]
Part 7: Remove use of expression closure from browser/experiments/.

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

r=me either way, but:

::: browser/experiments/Experiments.jsm
@@ +2224,5 @@
>  
>  this.Experiments.PreviousExperimentProvider.prototype = Object.freeze({
> +  get name() {
> +    return "PreviousExperimentProvider";
> +  },

Considering we freeze this object, I don't really see a reason not to just use name: "PreviousExperimentProvider"

Is there a difference in behaviour I am unaware of?

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8664715 [details] [diff] [review]
Part 2: Remove use of expression closure from browser/base/content/test/general/.

>--- a/browser/base/content/test/general/browser_ctrlTab.js
>+++ b/browser/base/content/test/general/browser_ctrlTab.js
>@@ -85,24 +85,27 @@ function test() {
>   }
> 
>   // cleanup
>   if (gPrefService.prefHasUserValue("browser.ctrlTab.previews"))
>     gPrefService.clearUserPref("browser.ctrlTab.previews");
> 
>   /* private utility functions */
> 
>-  function pressCtrlTab(aShiftKey)
>-    EventUtils.synthesizeKey("VK_TAB", { ctrlKey: true, shiftKey: !!aShiftKey });
>+  function pressCtrlTab(aShiftKey) {
>+    return EventUtils.synthesizeKey("VK_TAB", { ctrlKey: true, shiftKey: !!aShiftKey });
>+  }
> 
>-  function releaseCtrl()
>-    EventUtils.synthesizeKey("VK_CONTROL", { type: "keyup" });
>+  function releaseCtrl() {
>+    return EventUtils.synthesizeKey("VK_CONTROL", { type: "keyup" });
>+  }

These two functions don't need to return anything.

>--- a/browser/base/content/test/general/browser_overflowScroll.js
>+++ b/browser/base/content/test/general/browser_overflowScroll.js
>@@ -1,23 +1,43 @@
> var tabstrip = gBrowser.tabContainer.mTabstrip;
> var scrollbox = tabstrip._scrollbox;
> var originalSmoothScroll = tabstrip.smoothScroll;
> var tabs = gBrowser.tabs;
> 
>-function rect(ele)           ele.getBoundingClientRect();
>-function width(ele)          rect(ele).width;
>-function left(ele)           rect(ele).left;
>-function right(ele)          rect(ele).right;
>-function isLeft(ele, msg)    is(left(ele) + tabstrip._tabMarginLeft, left(scrollbox), msg);
>-function isRight(ele, msg)   is(right(ele) - tabstrip._tabMarginRight, right(scrollbox), msg);
>-function elementFromPoint(x) tabstrip._elementFromPoint(x);
>-function nextLeftElement()   elementFromPoint(left(scrollbox) - 1);
>-function nextRightElement()  elementFromPoint(right(scrollbox) + 1);
>-function firstScrollable()   tabs[gBrowser._numPinnedTabs];
>+function rect(ele) {
>+  return ele.getBoundingClientRect();
>+}
>+function width(ele) {
>+  return rect(ele).width;
>+}
>+function left(ele) {
>+  return rect(ele).left;
>+}
>+function right(ele) {
>+  return rect(ele).right;
>+}
>+function isLeft(ele, msg) {
>+  return is(left(ele) + tabstrip._tabMarginLeft, left(scrollbox), msg);
>+}
>+function isRight(ele, msg) {
>+  return is(right(ele) - tabstrip._tabMarginRight, right(scrollbox), msg);
>+}
>+function elementFromPoint(x) {
>+  return tabstrip._elementFromPoint(x);
>+}
>+function nextLeftElement() {
>+  return elementFromPoint(left(scrollbox) - 1);
>+}
>+function nextRightElement() {
>+  return elementFromPoint(right(scrollbox) + 1);
>+}
>+function firstScrollable() {
>+  return tabs[gBrowser._numPinnedTabs];
>+}

let rect = ele => ele.getBoundingClientRect(); etc.

Comment 22

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #17)
> Comment on attachment 8664722 [details] [diff] [review]
> Part 8: Remove use of expression closure from browser/fuel/.
> 
> Review of attachment 8664722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/fuel/fuelApplication.js
> @@ +17,5 @@
> >  var Utilities = {
> >    get bookmarks() {
> >      let bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
> >                      getService(Ci.nsINavBookmarksService);
> > +    this.__defineGetter__("bookmarks", () => bookmarks);
> 
> nit: here and elsewhere you can use _ => ...

Why would that be better?

Comment 23

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #22)
> (In reply to :Gijs Kruitbosch from comment #17)
> > Comment on attachment 8664722 [details] [diff] [review]
> > Part 8: Remove use of expression closure from browser/fuel/.
> > 
> > Review of attachment 8664722 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/fuel/fuelApplication.js
> > @@ +17,5 @@
> > >  var Utilities = {
> > >    get bookmarks() {
> > >      let bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
> > >                      getService(Ci.nsINavBookmarksService);
> > > +    this.__defineGetter__("bookmarks", () => bookmarks);
> > 
> > nit: here and elsewhere you can use _ => ...
> 
> Why would that be better?

We (arai and I) discussed this over IRC already, please ignore (I'll mark the comment obsolete in a sec). :-)

Comment 24

[Tooru Fujisawa [:arai]]

thank you dao and Gijs :D

looks like Android M(13) is perm-fail, I'll investigate it shortly.

(In reply to :Gijs Kruitbosch from comment #20)
> ::: browser/experiments/Experiments.jsm
> @@ +2224,5 @@
> >  
> >  this.Experiments.PreviousExperimentProvider.prototype = Object.freeze({
> > +  get name() {
> > +    return "PreviousExperimentProvider";
> > +  },
> 
> Considering we freeze this object, I don't really see a reason not to just
> use name: "PreviousExperimentProvider"
> 
> Is there a difference in behaviour I am unaware of?

it will throw different error when setting value, but it won't matter.  I'll change it to |name: "PreviousExperimentProvider"|.

Comment 25

[Tooru Fujisawa [:arai]]

pushed some separated patch series to try.

so far, M(13) fails on all patterns, even without my patches :/
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e794bfb693d7 (with patches for bug 1207490)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ff0c2623bd0 (with patches for bug 1207491)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e86f20db8e7d (with patches for bug 1207494)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f59a74fcbc1b (without patches, only m-c)

I feel like it's not related orange.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb921354f6ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/323a9a3c1608
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a1e0b22871
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3a9fcf73d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8815627054a
https://hg.mozilla.org/integration/mozilla-inbound/rev/98d3300738e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9dda4b6e6e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2646cfe32bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ba29693dd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/932cb872289f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8593648ed49e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c63ef61734b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4cee17afa63
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2c4a65fe790
https://hg.mozilla.org/integration/mozilla-inbound/rev/97264d043924
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06d2002c03b

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74062a46a01a

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb921354f6ca
https://hg.mozilla.org/mozilla-central/rev/323a9a3c1608
https://hg.mozilla.org/mozilla-central/rev/f4a1e0b22871
https://hg.mozilla.org/mozilla-central/rev/1e3a9fcf73d8
https://hg.mozilla.org/mozilla-central/rev/b8815627054a
https://hg.mozilla.org/mozilla-central/rev/98d3300738e3
https://hg.mozilla.org/mozilla-central/rev/e9dda4b6e6e0
https://hg.mozilla.org/mozilla-central/rev/c2646cfe32bf
https://hg.mozilla.org/mozilla-central/rev/04ba29693dd5
https://hg.mozilla.org/mozilla-central/rev/932cb872289f
https://hg.mozilla.org/mozilla-central/rev/8593648ed49e
https://hg.mozilla.org/mozilla-central/rev/c63ef61734b0
https://hg.mozilla.org/mozilla-central/rev/a4cee17afa63
https://hg.mozilla.org/mozilla-central/rev/e2c4a65fe790
https://hg.mozilla.org/mozilla-central/rev/97264d043924
https://hg.mozilla.org/mozilla-central/rev/b06d2002c03b
https://hg.mozilla.org/mozilla-central/rev/74062a46a01a