1151476 - Remove use of expression closures in suite/

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Tooru Fujisawa [:arai]]

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

[Tooru Fujisawa [:arai]]

Attachment: Remove use of expression closure from suite/.

converting rules are following:
  * function declaration
    add `return` and braces
  * standalone function expression contans and receives `this` (Array.filter, bind, etc)
    convert to arrow function
  * standalone function expression contans 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
  * getter property
    add `return` and braces
  * setter property
    add and braces

Comment 2

[Philip Chee]

Comment on attachment 8588930 [details] [diff] [review]
Remove use of expression closure from suite/.

I think Neil Rashbrook would b a better reviewer in this instance.

Comment 3

[[e-mail]]

Comment on attachment 8588930 [details] [diff] [review]
Remove use of expression closure from suite/.

>+      if (browser.engines.some((e => e.title == engine.title))
Mismatched (. r=me with that fixed.

There are also some style nits that it would be nice if you could address. There are some specifically marked but most fall into two common patterns:
* Expression closures that didn't actually return a value
  (so they shouldn't have a return added), marked with NRV.
* Expression closures replaced with an ordinary function expression
  (so they should be written on three lines), marked with FTL.

>+  function pressCtrlTab(aShiftKey) {
>+    return EventUtils.synthesizeKey("VK_TAB", { ctrlKey: true, shiftKey: !!aShiftKey });
>+  }
> 
>+  function releaseCtrl() {
>+    return EventUtils.synthesizeKey("VK_CONTROL", { type: "keyup" });
>+  }
NRV

>+  function isChecked(aItem) { return aItem.checked; }
FTL

>-      ["cmd_close", "cmd_closeWindow"].map(function (id) this._element(id), this);
>+      ["cmd_close", "cmd_closeWindow"].map(id => this._element(id));
Nice!

>+  getLevel: function(aRow) { return this._getNodeForRow(aRow).indentLevel; },
FTL

>diff --git a/suite/common/places/browserPlacesViews.js b/suite/common/places/browserPlacesViews.js
There are several cases of FTL here, e.g. get viewElt() this.viewElt, should become
get viewElt() {
  return this._viewElt;
},

>+  getLevel: function(aRow) { return this._getNodeForRow(aRow).indentLevel; },
FTL

>+        value = options.filter(aIx => aIx >= 0);
[I'm just glad we didn't write this as aIx => 0 <= aIx !]

>+  get _remoteSites() { return [Weave.Service.serverURL, RECAPTCHA_DOMAIN]; },
FTL

>-  function countByTitle(aClosedTabList, aTitle)
>-    aClosedTabList.filter(function(aData) aData.title == aTitle).length;
>+  function countByTitle(aClosedTabList, aTitle) {
>+    return aClosedTabList.filter(aData => aData.title == aTitle).length;
>+  }
Whoa, two fixes in one, this had me confused for a moment.

>+    beginBatch: function() {
>+      return PlacesUtils.transactionManager.beginBatch(null);
>+    },
> 
>+    endBatch: function() {
>+      return PlacesUtils.transactionManager.endBatch(false);
>+    },
> 
>+    doTransaction: function(txn) {
>+      return PlacesUtils.transactionManager.doTransaction(txn);
>+    },
> 
>+    undoTransaction: function() {
>+      return PlacesUtils.transactionManager.undoTransaction();
>+    },
> 
>+    redoTransaction: function() {
>+      return PlacesUtils.transactionManager.redoTransaction();
>+    },
NRV

>+    set maxTransactionCount(val) {
>+      PlacesUtils.transactionManager.maxTransactionCount = val;
>+    },
Style nit: I'd actually like a return here.

>+    clear: function() {
>+      return PlacesUtils.transactionManager.clear();
>+    },
NRV

>+    getUndoStack: function() {
>+      return PlacesUtils.transactionManager.getUndoStack();
>+    },
> 
>+    getRedoStack: function() {
>+      return PlacesUtils.transactionManager.getRedoStack();
>+    },
[Whoops, we seem to have copied Firefox's error here... these are really List, not Stack.]

>+    AddListener: function(aListener) {
>+      return PlacesUtils.transactionManager.AddListener(aListener);
>+    },
> 
>+    RemoveListener: function(aListener) {
>+      return PlacesUtils.transactionManager.RemoveListener(aListener);
>+    }
NRV

>-  willClearItem: function(aItemName) this.items[aItemName].willClear,
>+  willClearItem: function(aItemName) { return this.items[aItemName].willClear; },
> 
>-  canClearItem: function(aItemName) this.items[aItemName].canClear,
>+  canClearItem: function(aItemName) { return this.items[aItemName].canClear; },
FTL

>-      onLocationChange: function () 0,
>-      onProgressChange: function () 0,
>-      onStatusChange: function () 0,
>-      onSecurityChange: function () 0
>+      onLocationChange: () => 0,
>+      onProgressChange: () => 0,
>+      onStatusChange: () => 0,
>+      onSecurityChange: () => 0
These don't actually return a value, so function() {}, would be more appropriate.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Remove use of expression closure from suite/. r=Neil

Thank you for reviewing!
Fixed them :)

Comment 5

[Tooru Fujisawa [:arai]]

maybe I can land this by myself after bug 1147835 is fixed?

Comment 6

[Philip Chee]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> maybe I can land this by myself after bug 1147835 is fixed?

SeaMonkey tree is perma-closed. You might have to wait forever. If you need to land this sooner let me know and I'll approve landing on CLOSED TREE.

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to Philip Chee from comment #6)
> (In reply to Tooru Fujisawa [:arai] from comment #5)
> > maybe I can land this by myself after bug 1147835 is fixed?
> 
> SeaMonkey tree is perma-closed. You might have to wait forever. If you need
> to land this sooner let me know and I'll approve landing on CLOSED TREE.

Wow, I'd like to land this before it bitrots, since it's wide-ranging :)
Can you approve the patch (attachment 8591192 [details] [diff] [review])?

Comment 8

[Philip Chee]

Comment on attachment 8591192 [details] [diff] [review]
Remove use of expression closure from suite/. r=Neil

a=me for SeaMonkey CLOSED TREE

Comment 9

[Tooru Fujisawa [:arai]]

Thank you!

https://hg.mozilla.org/comm-central/rev/32e030ae725c