Mozilla Coding Style Guide

Mozilla Coding Style Guide

This document attempts to explain the basic styles and patterns that are used in the Mozilla codebase. New code should try to conform to these standards so that it is as easy to maintain as existing code. Of course every rule has an exception, but it's important to know the rules nonetheless!

This is particularly directed at people new to the Mozilla codebase, who are in the process of getting their code reviewed. Before getting a review, please read over this document and make sure your code conforms to the recommendations here.

General C/C++ Practices

COM and pointers

IDL

Error Handling

Strings

Naming and Formatting code

General C/C++ Practices

• Have you checked for compiler warnings? Warnings often point to real bugs.
• Are the changes 64-bit clean?
• Do the changes meet the C++ portability guidelines
• Don't use NULL for pointers. On some systems it's declared as void * and causes a compile warning when assigned to a pointer. Use 0 or nsnull instead.
• When testing a pointer, use !myPtr or (myPtr); don't use myPtr != nsnull or myPtr == nsnull.
• Do not compare == PR_TRUE or PR_FALSE. Use (x) or (!x) instead. == PR_TRUE, in fact, is *different* from if (x)!
• Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level.
• Always check the return value of new for null.
• Don't leave debug printf()s lying around.
• Use JavaDoc-style comments in any new class header files.
• When fixing a problem, check to see if the problem occurs elsewhere in the same file, and fix it everywhere if possible.
• On whether to use nsFoo aFoo (bFoo) or nsFoo aFoo = bFoo: For first tier platforms, although the former is theoretically better, there is probably no reason to prefer it over the latter form. This is good, since everyone agrees, the form with "=" looks better. More data for second tier platforms would be good.
• Forward declare classes in your header files instead of including them whenever possible. For example, if you have an interface with a void DoSomething(nsIContent* aContent) function, forward declare with class nsIContent; instead of #include "nsIContent.h"

COM and pointers

• Use nsCOMPtr<>
  If you don't know how to use it, start looking in the code for examples. The general rule is that the very act of typing NS_RELEASE should be a signal to you to question your code: "Should I be using nsCOMPtr here?". Generally the only valid use of NS_RELEASE are when you are storing refcounted pointers in a long-lived datastructure.
• Declare new XPCOM interfaces using XPIDL so they will be scriptable.
• Use nsCOMPtr for strong references, and nsWeakPtr for weak references.
• String arguments to functions should be declared as nsAString.
• Use str.IsEmpty() instead of str.Length() == 0.
• Don't use QueryInterface directly. Use CallQueryInterface or do_QueryInterface instead.
• nsresult should be declared as rv. Not res, not result, not foo.
• For constant strings, use NS_LITERAL_STRING("...") instead of NS_ConvertASCIItoUCS2("..."), AssignWithConversion("..."), EqualsWithConversion("..."), or nsAutoString()
• Use contractids instead of progids or class IDs.

IDL

• Use leading-lowercase, or "interCaps"

  When defining a method or attribute in IDL, the first letter should be lowercase, and each following word should be capitalized. For example:

          long updateStatusBar();
        

• Use attributes wherever possible

Whenever you are retrieving or setting a single value without any context, you should use attributes. Don't use two methods when you could use one attribute. Using attributes logically connects the getting and setting of a value, and makes scripted code look cleaner.

This example has too many methods:

    interface nsIFoo : nsISupports {
        long getLength();
        void setLength(in long length);
        long getColor();
    };
    

The code below will generate the exact same C++ signature, but is more script-friendly.

    interface nsIFoo : nsISupports {
        attribute long length;
        readonly attribute long color;
    };
    

• Use java-style constants

When defining scriptable constants in IDL, the name should be all uppercase, with underscores between words:

        const long ERROR_UNDEFINED_VARIABLE = 1;
    

Error handling

• Check for errors early and often

  Every time you make a call into an XPCOM function, you should check for an error condition. You need to do this even if you know that call will never fail. Why?

  • Someone may change the callee in the future to return a failure condition.
  • The object in question may live on another thread, another process, or possibly even another machine. The proxy could have failed to actually make your call in the first place.

• Use the nice macros
  Use the NS_ENSURE_SUCCESS(rv, rv) and NS_ENSURE_TRUE(expr, rv) macros in place of if (NS_FAILED(rv)) { return rv; } and if (!expr) { return rv; }, unless the failure is a normal condition (i.e. you don't want it to assert).

• Return from errors immediately

In most cases, your knee-jerk reaction should be to return from the current function when an error condition occurs. Don't do this:

    rv = foo->Call1();
    if (NS_SUCCEEDED(rv)) {
        rv = foo->Call2();
            if (NS_SUCCEEDED(rv)) {
                rv = foo->Call3();
            }
        }
    }
    return rv;
    

Instead, do this:

    rv = foo->Call1();
    NS_ENSURE_SUCCESS(rv, rv);

    rv = foo->Call2();
    NS_ENSURE_SUCCESS(rv, rv);

    rv = foo->Call3();
    NS_ENSURE_SUCCESS(rv, rv);
    

Why? Because error handling should not obfuscate the logic of the code. The author's intent in the first example was to make 3 calls in succession, but wrapping the calls in nested if() statements obscured the most likely behavior of the code.

Consider a more complicated example that actually hides a bug:

    PRBool val;
    rv = foo->GetBooleanValue(&val);
    if (NS_SUCCEEDED(rv) && val)
        foo->Call1();
    else
        foo->Call2();
    

The intent of the author may have been that foo->Call2() would only happen when val had a false value. In fact, foo->Call2() will also be called when foo->GetBooleanValue(&val) fails. This may or may not have been the author's intent, and it is not clear from this code. Here is an updated version:

    PRBool val;
    rv = foo->GetBooleanValue(&val);
    if (NS_FAILED(rv)) return rv;
    if (val)
        foo->Call1();
    else
        foo->Call2();
    

In this example, the author's intent is clear, and an error condition avoids both calls to foo->Call1() and foo->Call2();

Possible exceptions: Sometimes it is not fatal if a call fails. For instance, if you are notifying a series of observers that an event has fired, it might be inconsequential that one of these notifications failed:

    for (i=0; i<length; i++) {
        // we don't care if any individual observer fails
        observers[i]->Observe(foo, bar, baz);
    }
    

Another possibility is that you are not sure if a component exists or is installed, and you wish to continue normally if the component is not found.

    nsCOMPtr<nsIMyService> service = do_CreateInstance(NS_MYSERVICE_CID, &rv);
    // if the service is installed, then we'll use it
    if (NS_SUCCEEDED(rv)) {
        // non-fatal if this fails too, ignore this error
        service->DoSomething();

        // this is important, handle this error!
        rv = service->DoSomethingImportant();
        if (NS_FAILED(rv)) return rv;
    }
        
    // continue normally whether or not the service exists
    

Strings

• Use the Auto form of strings for local values

When declaring a local, short-lived nsString class, always use nsAutoString or nsCAutoString - these versions pre-allocate a 64-byte buffer on the stack, and avoid fragmenting the heap. Don't do this:

    nsresult foo() {
      nsCString bar;
      ..
    }
    

instead:

    nsresult foo() {
      nsCAutoString bar;
      ..
    }
    

• Be wary of leaking values from non-XPCOM functions that return char* or PRUnichar*

It is an easy trap to return an allocated string from an internal helper function, and then use that function inline in your code without freeing the value. Consider this code:

    static char *GetStringValue() {
        ..
        return resultString.ToNewCString();
    }

        ..
        WarnUser(GetStringValue());
    

In the above example, WarnUser will get the string allocated from resultString.ToNewCString() and throw away the pointer. The resulting value is never freed. Instead, either use the string classes to make sure your string is automatically freed when it goes out of scope, or make sure that your string is freed.

Automatic cleanup:

    static void GetStringValue(nsAWritableCString& aResult) {
        ..
        aResult.Assign("resulting string");
    }

        ..
        nsCAutoString warning;
        GetStringValue(warning);
        WarnUser(warning.get());
    

Free the string manually:

    static char *GetStringValue() {
        ..
        return resultString.ToNewCString();
    }

        ..
        char *warning = GetStringValue();
        WarnUser(warning);
        nsMemory::Free(warning);
    

• Use NS_LITERAL_STRING() to avoid runtime string conversion.

It is very common to need to assign the value of a literal string such as "Some String" into a unicode buffer. Instead of using nsString's AssignWithConversion and AppendWithConversion, use NS_LITERAL_STRING() instead. On most platforms, this will force the compiler to compile in a raw unicode string, and assign it directly.

Incorrect:

    nsAutoString warning; warning.AssignWithConversion("danger will robinson!");
    ..
    foo->SetUnicodeValue(warning.get());
    

Correct:

    NS_NAMED_LITERAL_STRING(warning,"danger will robinson!");
    ..
    // if you'll be using the 'warning' string, you can still use it as before:
    foo->SetUnicodeValue(warning.get());

    // alternatively, use the wide string directly:
    foo->SetUnicodeValue(NS_LITERAL_STRING("danger will robinson!").get());
    

Naming and Formatting code

• Note: the following is not all set in stone, this is interim to give people a chance to look

• Use the prevailing style in a file or module, or ask the owner, if you are on someone else's turf. Module owner rules all.
• Whitespace: No tabs. No whitespace at the end of a line.
• Line Length: 80 characters or less (for Bonsai and printing).
• Control Structures:
  

  if (...) {
  } else if (...) {
  } else {
  }
  
  while (...) {
  }
  
  do {
  } while (...);
  
  for (...; ...; ...) {
  }
  
  switch (...)
  {
    case 1:
      {
        // When you need to declare a variable in a switch, put the block in braces
        int var;
      } break;
    case 2:
      ...
      break;
    default:
      break;
  }
  

  Note the space here: if (. switch in particular is not quite agreed-upon ... try really hard to find a module style for that one :)
• Classes:
  

  class nsMyClass : public X,
                         public Y
  {
  public:
    nsMyClass() : mVar(0) { ... };
    
  private:
    int mVar;
  };
  

  For small functions in a class declaration, it's OK to do the above. For larger ones use something similar to method declarations below.
• Methods:
  

  int
  nsMyClass::Method(...)
  {
    ...
  }
  

• Mode Line: Files should have an Emacs mode line comment as the first line of the file, which should set indent-tabs-mode to nil. For new files, use this, specifying 2-space indentation:
  

  /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
          

• Operators should be at the end of a line, not the beginning of the next, if an operator is at a line break.
• Follow naming prefix conventions.
  Variable prefixes:
  • k=constant (e.g. kNC_child)
  • g=global (e.g. gPrefService)
  • m=member (e.g. mLength)
  • a=argument (e.g. aCount)
  • s=static member (e.g. sPrefChecked)
  Global functions/macros/etc
  • Macros begin with NS_, and are all caps (e.g. NS_IMPL_ISUPPORTS)
  • Global (exported) functions begin with NS_ and use LeadingCaps (e.g. NS_NewISupportsArray)

Original document by Alec Flett.
Thanks to:

• pink
• smfr
• waterson
• jband
• brendan
• rogc

for additional comments.
Additions by Akkana Peck based on discussions on IRC: thanks to: bbaetz, bz, jfrancis, jkeiser, mjudge, and sdagley for comments, and to John Keiser and JST's Reviewer Simulacrum and Brendan and Mitchell's super-review document.