Issue 4795 - Avoid shorthand method syntax where prototype matters

Issue 4795 - Avoid shorthand method syntax where prototype matters

[kzar, 2017-01-23 13:43:45]

Patch Set 1

[Sebastian Noack, 2017-01-23 13:54:22]

https://codereview.adblockplus.org/29372820/diff/29372821/lib/compat.js
File lib/compat.js (right):

https://codereview.adblockplus.org/29372820/diff/29372821/lib/compat.js#newco...
lib/compat.js:59: // We don't use the method definition shorthand syntax for
these functions
I had a hard time to understand what is going on here. The problem is that this
is not really a class, but more like a namespace, while nsIFileURL and
nsIHttpChannel are meant to be classes themselves, and in order to define a
class's constructor you have to use the function statement (not the method
shorthand syntax). I wonder whether this comment could better clarify the issue.

[kzar, 2017-01-23 13:59:13]

https://codereview.adblockplus.org/29372820/diff/29372821/lib/compat.js
File lib/compat.js (right):

https://codereview.adblockplus.org/29372820/diff/29372821/lib/compat.js#newco...
lib/compat.js:59: // We don't use the method definition shorthand syntax for
these functions
On 2017/01/23 13:54:22, Sebastian Noack wrote:
> I wonder whether this comment could better clarify the issue.

It surely could, I'm open to suggestions.

[Sebastian Noack, 2017-02-16 12:05:10]

https://codereview.adblockplus.org/29372820/diff/29372821/lib/compat.js
File lib/compat.js (right):

https://codereview.adblockplus.org/29372820/diff/29372821/lib/compat.js#newco...
lib/compat.js:59: // We don't use the method definition shorthand syntax for
these functions
On 2017/01/23 13:59:13, kzar wrote:
> On 2017/01/23 13:54:22, Sebastian Noack wrote:
> > I wonder whether this comment could better clarify the issue.
> 
> It surely could, I'm open to suggestions.

With our just established ESLint configuration this will also cause an error.
How about this:

  function nsIFileURL() {}
  function nsIHttpChannel() {}

  ...

  var Components =
  {
    interfaces:
    {
      ...

      nsIFileURL,
      nsIHttpChannel,

That would also make this comment redundant, IMO.

[kzar, 2017-02-17 09:40:28]

Patch Set 2 : Addressed Sebastian's feedback

https://codereview.adblockplus.org/29372820/diff/29372821/lib/compat.js
File lib/compat.js (right):

https://codereview.adblockplus.org/29372820/diff/29372821/lib/compat.js#newco...
lib/compat.js:59: // We don't use the method definition shorthand syntax for
these functions
On 2017/02/16 12:05:10, Sebastian Noack wrote:
> On 2017/01/23 13:59:13, kzar wrote:
> > On 2017/01/23 13:54:22, Sebastian Noack wrote:
> > > I wonder whether this comment could better clarify the issue.
> > 
> > It surely could, I'm open to suggestions.
> 
> With our just established ESLint configuration this will also cause an error.
> How about this:
> 
>   function nsIFileURL() {}
>   function nsIHttpChannel() {}
> 
>   ...
> 
>   var Components =
>   {
>     interfaces:
>     {
>       ...
> 
>       nsIFileURL,
>       nsIHttpChannel,
> 
> That would also make this comment redundant, IMO.

Done.

[Sebastian Noack, 2017-02-17 12:08:21]

LGTM