1151473 - Remove use of expression closures in chat/

Status: RESOLVED FIXED

(Chat Core :: 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

[Patrick Cloke [:clokep]]

Tooru, what's the status of this? Could you attach your WIP?

Comment 2

[Tooru Fujisawa [:arai]]

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.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Remove use of expression closure in chat/components/, chat/content/, and chat/modules/.

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

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove use of expression closure in chat/protocols/.

Comment 5

[aleth [:aleth]]

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.

Comment 6

[Tooru Fujisawa [:arai]]

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.

Comment 7

[Florian Quèze [:florian]]

(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 8

[Patrick Cloke [:clokep]]

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!

Comment 9

[Tooru Fujisawa [:arai]]

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

Is there any other tests that should be done manually?

Comment 10

[Patrick Cloke [: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.

Comment 11

[Tooru Fujisawa [:arai]]

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)

Comment 12

[Tooru Fujisawa [:arai]]

XMPP seems work :)

https://hg.mozilla.org/comm-central/rev/a66c8906fb9c
https://hg.mozilla.org/comm-central/rev/19a8584b53b8

Comment 13

[aleth [:aleth]]

Thanks!

Comment 14

[aleth [:aleth]]

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