Remove use of expression closures in chat/

RESOLVED FIXED in Instantbird 43

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

trunk
Instantbird 43

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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.
Tooru, what's the status of this? Could you attach your WIP?
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 2

4 years ago
Hi, thank you for reminder :)
I was going to post patches after Daily gets 43, to make it easier to backport other patches, since this affects many files and it may make many differences between comm-central and esr38.

okay, I'll rebase and post it this weekend.
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 3

4 years ago
Prepared 2 patches.

converting rules are following (same as bug 1123124):
  * function declaration
    add `return` and braces
  * standalone function expression contains and receives `this` (Array.map, bind, etc)
    convert to arrow function
  * standalone function expression contains 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
Assignee: nobody → arai.unmht
Attachment #8638450 - Flags: review?(aleth)

Comment 5

4 years ago
Comment on attachment 8638450 [details] [diff] [review]
Part 1: Remove use of expression closure in chat/components/, chat/content/, and chat/modules/.

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

Thanks for taking care of this! The patch looks good, but I suspect will be somewhat bitrotted by the time gecko 43 comes along.

::: chat/components/src/imCore.js
@@ +167,1 @@
>      Services.dirsvc.get("ProfD", Ci.nsIFile),

In instances like this, ideally the line break would be removed.
Attachment #8638450 - Flags: review?(aleth) → review+
(Assignee)

Comment 6

4 years ago
Thank you aleth :)

(In reply to aleth [:aleth] from comment #5)
> but I suspect will be somewhat bitrotted by the time gecko 43 comes along.

It was just a rough plan.  if it's okay, I'm going to land this before 43.

> ::: chat/components/src/imCore.js
> @@ +167,1 @@
> >      Services.dirsvc.get("ProfD", Ci.nsIFile),
> 
> In instances like this, ideally the line break would be removed.

Okay, removed.
(In reply to Tooru Fujisawa [:arai] from comment #6)
> Thank you aleth :)
> 
> (In reply to aleth [:aleth] from comment #5)
> > but I suspect will be somewhat bitrotted by the time gecko 43 comes along.
> 
> It was just a rough plan.  if it's okay, I'm going to land this before 43.

That would be ok with me, but I guess you probably want to land both parts at once.
Comment on attachment 8638451 [details] [diff] [review]
Part 2: Remove use of expression closure in chat/protocols/.

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

Looks good. I'd like someone to try both these patches for a bit and ensure they work. (And to land them on 43, not 42.) Thanks for the work!
Attachment #8638451 - Flags: review?(clokep) → review+
(Assignee)

Comment 9

4 years ago
thank you!
I'll push them to try server after the aurora merge.

Is there any other tests that should be done manually?
Flags: needinfo?(clokep)
(In reply to Tooru Fujisawa [:arai] from comment #9)
> thank you!
> I'll push them to try server after the aurora merge.

That helps for the chat/ code, but not really the im/ code. (Still a good idea!)

> Is there any other tests that should be done manually?

Figured someone just needs to apply these, do a build and connect an IRC and (JavaScript) XMPP account and click around a bit and ensure nothing is broken. I don't expect any issues.
Flags: needinfo?(clokep)
(Assignee)

Comment 11

4 years ago
I don't see any suspicious failure in try run
  https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=640829344d29

no issue found on IRC, I'll try XMPP (haven't used, I'll learn it first)
(Assignee)

Comment 12

4 years ago
XMPP seems work :)

https://hg.mozilla.org/comm-central/rev/a66c8906fb9c
https://hg.mozilla.org/comm-central/rev/19a8584b53b8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 43

Comment 13

4 years ago
Thanks!

Comment 14

3 years ago
https://hg.mozilla.org/comm-central/rev/49be41a82721ac6caca28b8994f16998dea13340
Bug 1151473 - Remove use of expression closures in chat/, followup a missed one in skype. rs=me
You need to log in before you can comment on or make changes to this bug.