Follow Mozilla coding style better

RESOLVED FIXED

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: mitcho, Unassigned)

Tracking

Dependency tree / graph

Details

(Whiteboard: tracked)

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
(Reporter)

Comment 1

8 years ago
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.
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

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

Comment 4

8 years ago
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;
})();
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.
(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

8 years ago
No need to prematurely optimize for performance. Giving function names actually decreases performance, but it helps in development/debugging.
(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.
(Reporter)

Comment 9

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

8 years ago
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.
(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

Updated

8 years ago
Depends on: 582200

Comment 12

8 years ago
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, ...

Updated

8 years ago
Blocks: 582023
(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

8 years ago
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.
(Reporter)

Updated

8 years ago
Blocks: 585689
(Reporter)

Updated

8 years ago
No longer blocks: 574217
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 17

8 years ago
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Component: TabCandy → TabCandy
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
(Assignee)

Updated

2 years ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.