Closed
Bug 1151475
Opened 9 years ago
Closed 9 years ago
Remove use of expression closures in mail/
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 41.0
People
(Reporter: arai, Assigned: arai)
References
()
Details
Attachments
(5 files)
19.12 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
30.65 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
7.01 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
10.61 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
59.48 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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.
This seems to be a search query (thanks to jcranmer) that returns many of the occurrences: https://dxr.mozilla.org/comm-central/search?q=regexp%3A%22\bfunction\s*\%28[^%29]*\%29\s*[a-zA-Z0-9_%28\%22%27.~!%2B-]%22+path%3Amail%2F&case=true&redirect=true
Version: unspecified → Trunk
Comment 2•9 years ago
|
||
Arai has already pushed patches to Try-comm-central, e.g. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=acc724e0ed19
OK:) If only mozmill tests worked so the patches could actually be tested :(
Assignee: nobody → arai.unmht
Assignee | ||
Comment 4•9 years ago
|
||
Prepared 5 patches for mail/. they're not so large (same as mailnews/), so I guess it doesn't matter on backporting future patches to esr. 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 * method-like property with named function expression add `return` and braces * getter property add `return` and braces * setter property add braces Seems no regression on try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3ea958f2cad9 also, ran mozmill locally on linux 64bit, and no regression found with the patch series (19 failures with/without patches, for me).
Attachment #8589728 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8589729 -
Flags: review?(florian)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8589730 -
Flags: review?(mconley)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8589731 -
Flags: review?(mconley)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8589732 -
Flags: review?(mconley)
Comment 9•9 years ago
|
||
Comment on attachment 8589729 [details] [diff] [review] Part 2: Remove use of expression closure from mail/components/im/. Review of attachment 8589729 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8589729 -
Flags: review?(florian) → review+
Comment 10•9 years ago
|
||
Comment on attachment 8589728 [details] [diff] [review] Part 1: Remove use of expression closure from mail/base/. Review of attachment 8589728 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thx! r=mkmelin ::: mail/base/content/mailWidgets.xml @@ +29,5 @@ > .forEach(this.selectedItems.push, this.selectedItems); > > + children.filter(aChild => !aChild.hasAttribute("context")) > + .forEach(aChild => aChild.setAttribute("context", > + this.getAttribute("itemcontext"))); please re-indent this as last line two more spaces so it's not so confusing
Attachment #8589728 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Magnus Melin from comment #10) > > + children.filter(aChild => !aChild.hasAttribute("context")) > > + .forEach(aChild => aChild.setAttribute("context", > > + this.getAttribute("itemcontext"))); > > please re-indent this as last line two more spaces so it's not so confusing Thanks! fixed locally :)
Assignee | ||
Comment 12•9 years ago
|
||
Landed Part 1 and 2 :) https://hg.mozilla.org/comm-central/rev/07a6ec81889a https://hg.mozilla.org/comm-central/rev/1f40bc6a4a87
Keywords: leave-open
Comment 13•9 years ago
|
||
Comment on attachment 8589730 [details] [diff] [review] Part 3: Remove use of expression closure from mail/components/cloudfile/. Review of attachment 8589730 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay! Thank you!
Attachment #8589730 -
Flags: review?(mconley) → review+
Updated•9 years ago
|
Attachment #8589731 -
Flags: review?(mconley) → review+
Updated•9 years ago
|
Attachment #8589732 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 14•9 years ago
|
||
Thank you for reviewing :D https://hg.mozilla.org/comm-central/rev/5eb45f784b5b https://hg.mozilla.org/comm-central/rev/ecee3826ae91 https://hg.mozilla.org/comm-central/rev/3f5395827a5c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
Comment 15•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•