577968 - Follow Mozilla coding style better

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

Here are some statements from the Mozilla coding style guidelines (https://developer.mozilla.org/En/Developer_Guide/Coding_Style) which I know we often are not following right now:

- Control keywords if, for, while, and switch are always followed by a space to distinguish them from function calls which have no trailing space. 
- Do not compare x == true or x == false. Use (x)  or (!x) instead. x == true, in fact, is different from if (x)! Compare objects to null, numbers to 0 or strings to "" if there is chance for confusion.
- Prefer double quotes, except in in-line event handlers or when quoting double quotes.
- Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level.
- Don't leave debug printfs or dumps lying around.
- Use Javadoc-style comments

The Javadoc comments have their own bug: bug #577965

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

Committed patches to fix:

- Control keywords if, for, while, and switch are always followed by a space to
distinguish them from function calls which have no trailing space.
- Do not compare x == true or x == false.
- Don't put an else right after a return.

Comment 2

[Raymond Lee [:raymondlee]]

Here are some of my additional comments:
- We are using "var" and "let" at the same time in our code, we should standardize them and use one of them.
- Although it's impossible to have pubic and private methods and variables in Javascript, it would make the code more readable to have "_" in front of all private methods and variables. e.g. _setupKeyHandlers: function() {}.  And if you want to access a private variable in other class, it would be better to use setter and getter.

Comment 3

[Aza Raskin [:aza]]

Does anyone know if we need to switch from var to let?

Comment 4

[Ed Lee :Mardak]

I don't think it's necessary. I always use let, but it does have slightly different behaviors from var. let will only scope to the block, so its value/memory will get freed at the end of, say, an if statement when declared inside.

However, some code could potentially get cleaned up by using let instead of creating a local closure to scope variables. E.g., something like the following pattern:

(function() {
  var local;
})();

Comment 5

[Ian Gilman [:iangilman]]

The majority of the code is using var, so if we have to standardize on one, that's the path of least resistance.

Here's another item from the coding style page that's worth considering:

* Name inline functions, this makes it easier to debug them. Just assigning a function to a property doesn't name the function.

Comment 6

[Raymond Lee [:raymondlee]]

(In reply to comment #5)
> Here's another item from the coding style page that's worth considering:
> 
> * Name inline functions, this makes it easier to debug them. Just assigning a
> function to a property doesn't name the function.

I think we should avoid nested functions if possible because of the efficiency considerations. See this, especially "Efficiency considerations" section
https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference:Functions#Nested_functions_and_closures

Comment 7

[Ed Lee :Mardak]

No need to prematurely optimize for performance. Giving function names actually decreases performance, but it helps in development/debugging.

Comment 8

[Ian Gilman [:iangilman]]

(In reply to comment #7)
> No need to prematurely optimize for performance. Giving function names actually
> decreases performance, but it helps in development/debugging.

Agreed; anonymous functions are extremely useful. 

As far as naming our functions, this applies to almost every function in TabCandy, as we create our objects in this fashion:

Foo.prototype = {
  func: function() {
  }
};

In that example, func is anonymous as far as the debugger is concerned. If it was written like so:

Foo.prototype = {
  func: function Foo_func() {
  }
};

...then Foo_func would appear in the stack trace and make debugging easier.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #8)
> In that example, func is anonymous as far as the debugger is concerned. If it
> was written like so:
> 
> Foo.prototype = {
>   func: function Foo_func() {
>   }
> };
> 
> ...then Foo_func would appear in the stack trace and make debugging easier.

That is, in fact, what I've done in Trenches and some of Drag. FYI.

Comment 10

[Steffen Wilberg]

Trailing whitespace is frowned upon as well.
Searching for [ ]+$ in browser/base/content/tabcandy results in 1455 lines in 15 files. The removal patch (272 KB) is likely to bitrot every other patch out there, but saves 4955 bytes, which is quite a bit. I can push such a patch if that is not a concern.

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #10)
> Trailing whitespace is frowned upon as well.
> Searching for [ ]+$ in browser/base/content/tabcandy results in 1455 lines in
> 15 files. The removal patch (272 KB) is likely to bitrot every other patch out
> there, but saves 4955 bytes, which is quite a bit. I can push such a patch if
> that is not a concern.

Good suggestion. Fixed.

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/de5a40afd129

Comment 12

[Frank Yan (:fryn)]

I would advise using let instead of var when a variable is only used within a certain block inside a function.
Our really old code still uses var in those cases, but almost all new code uses let to avoid scope pollution.
This isn't something for which I would re-post a patch if it's already pending review, but I have gotten a couple nits about that.

Lines with only whitespace are generally frowned upon.
s/\n\s+\n/\n\n/g should work for these.

Also, our JavaScript API supports:
Array.someMethod(this, ...
which does the same thing as:
Array.prototype.someMethod.call(this, ...

Comment 13

[Ed Lee :Mardak]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/d1bf7067c78d
Clean up spaces around (), [], , in iq and utils for bug 577968.

Comment 14

[Ian Gilman [:iangilman]]

(In reply to comment #12)
> Lines with only whitespace are generally frowned upon.

How does one avoid creating lines with only white space? Every editor I know preserves your indentation when you hit return, which means adding white space to the beginning of every new line.

Comment 15

[Ed Lee :Mardak]

I know in vim, it'll auto-indent for you on a new line, but if you don't type on that line and say, hit enter again, it'll remove the spaces from the previous line.

Comment 16

[Ian Gilman [:iangilman]]

I'm closing this open-ended bug. If there are specific additional items we need to address, we can file them separately. I believe we're good on this score, however, unless anything comes out of the pending reviews.

Comment 17

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".