Closed Bug 1123124 Opened 10 years ago Closed 10 years ago

Remove use of expression closures in mailnews/

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 40.0

People

(Reporter: jcranmer, Assigned: arai)

References

(
URL
)

Details

Attachments

(3 files, 1 obsolete file)

Part 1: Remove use of expression closure from mailnews/base/.
(deleted), patch
jcranmer
: review+
Details | Diff | Splinter Review
Part 3: Remove use of expression closure from mailnews/mime/.
(deleted), patch
jcranmer
: review+
Details | Diff | Splinter Review
Part 2: Remove use of expression closure from mailnews/db/.
(deleted), patch
jcranmer
: review+
Details | Diff | Splinter Review
<https://dxr.mozilla.org/comm-central/search?q=regexp%3A%22\bfunction\s*\%28%5B^%29%5D*\%29\s*%5Ba-zA-Z0-9_%28\%22%27.~!%2B-%5D%22+path%3Amail&case=true&redirect=true> (Adjust path to suit your liking) (Regex isn't quite complete, I think. No support for regexes, but hey, it's as good as I can get with a simple regex engine). In general, function (args) exp is equivalent to function (args) { return exp; }. Sometimes, it may be helpful to switch to (args) => exp, *BUT* if this is used within exp, the semantics are different (which is to say that (args) => exp is equivalent to (function (args) { return exp; }).bind(this), slightly different).
OK, if this bug is for mailnews only then the search seems to be 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%3Amailnews&case=true&redirect=true . That is a lot more manageable. Aryx, do you want to take this?
URL: https://dxr.mozilla.org/comm-central/...
Flags: needinfo?(archaeopteryx)
There is bug 1151475 for the Thunderbird part.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Prepared 3 patches for mailnews/. they're not so large, 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). patches for calendar/ and chat/ are somewhat large, and might be better to wait for a while (about 2 or 3 releases?), if they're related to thunderbird esr.
Attachment #8589447 - Flags: review?(Pidgeot18)
Attachment #8589448 - Flags: review?(Pidgeot18)
Attachment #8589449 - Flags: review?(Pidgeot18)
Comment on attachment 8589448 [details] [diff] [review] Part 2: Remove use of expression closure from mailnews/db/. Review of attachment 8589448 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/db/gloda/content/glodacomplete.xml @@ +68,5 @@ > </method> > <method name="_invalidate"> > <body> > <![CDATA[ > + setTimeout(function (self) { self.adjustHeight(); }, 0, this); Shouldn't this be () => this.adjustHeight();, like above? ::: mailnews/db/gloda/modules/datastore.js @@ +1695,1 @@ > return this._beginTransactionStatement; If I recall my JS properly, wouldn't return this._beginTransactionStatement = statemt; work?
Attachment #8589448 - Flags: review?(Pidgeot18)
Attachment #8589449 - Flags: review?(Pidgeot18) → review+
Thank you for reviewing! (In reply to Joshua Cranmer [:jcranmer] from comment #7) > Comment on attachment 8589448 [details] [diff] [review] > Part 2: Remove use of expression closure from mailnews/db/. > > Review of attachment 8589448 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mailnews/db/gloda/content/glodacomplete.xml > @@ +68,5 @@ > > </method> > > <method name="_invalidate"> > > <body> > > <![CDATA[ > > + setTimeout(function (self) { self.adjustHeight(); }, 0, this); > > Shouldn't this be () => this.adjustHeight();, like above? Yeah, thank you for pointing that out :D > ::: mailnews/db/gloda/modules/datastore.js > @@ +1695,1 @@ > > return this._beginTransactionStatement; > > If I recall my JS properly, wouldn't return this._beginTransactionStatement > = statemt; work? It won't work. _beginTransactionStatement has only getter, so setter is undefined (step 8 in following link). the assignment does nothing. and it will be warned if javascript.options.strict is true. http://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-set-p-v-receiver Object.prototype.__defineGetter__ should be replaced with standard Object.defineProperty, in separate bug (which will block bug 647423)
Attachment #8589448 - Attachment is obsolete: true
Attachment #8589457 - Flags: review?(Pidgeot18)
Attachment #8589447 - Flags: review?(Pidgeot18) → review+
Attachment #8589457 - Flags: review?(Pidgeot18) → review+
Thank you again! https://hg.mozilla.org/comm-central/rev/05a8c7d3c908 https://hg.mozilla.org/comm-central/rev/5996a8864a31 https://hg.mozilla.org/comm-central/rev/db3e708b2267
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Sorry to spam this bug, but it seems to break build process. Got this while building thunderbird today : "/home/fred/logs/mail/src/mailnews/local/src/nsParseMailbox.cpp:505:55: error: 'do_QueryObject' was not declared in this scope nsCOMPtr<nsISupports> supports = do_QueryObject(this); ^ /home/fred/logs/mail/src/mozilla/config/rules.mk:932: recipe for target 'nsParseMailbox.o' failed make[4]: *** [nsParseMailbox.o] Error 1" Is this related to gcc 4.9.1 ?
(In reply to Frederic Bezies from comment #10) > Sorry to spam this bug, but it seems to break build process. Got this while > building thunderbird today : > > "/home/fred/logs/mail/src/mailnews/local/src/nsParseMailbox.cpp:505:55: > error: 'do_QueryObject' was not declared in this scope > nsCOMPtr<nsISupports> supports = do_QueryObject(this); > ^ > /home/fred/logs/mail/src/mozilla/config/rules.mk:932: recipe for target > 'nsParseMailbox.o' failed > make[4]: *** [nsParseMailbox.o] Error 1" > > Is this related to gcc 4.9.1 ? I don't think it's related to this bug, since it changes only JavaScript code. could you file a new bug?
This change didn't break Thunderbird, but a change in mozilla-central (currently investigating). You can see this from the fact that the build with the same comm-central revision succeeded first and fail later to generate the nightly builds: https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=1def1e9c2d71 If you need a working comm-central build environment now, switch to the 'mozilla' directory and go back to the working revision by running |hg update -r e7112314d9d5|
@tooru : sorry for bug spam. @Archeopteryx : ok. Thanks for the info. I will look at it later.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: