DOM Workers & Storage C++ Code Style

This page describes the code style for the components maintained by the DOM Workers & Storage team. They live in-tree under the ‘dom/docs/indexedDB’ directory.

Introduction

This code style currently applies to the components living in the following directories:

• dom/file

• dom/indexedDB

• dom/localstorage

• dom/payments

• dom/quota

• dom/serviceworkers

• dom/workers

In the long-term, the code is intended to use the Mozilla Coding Style, which references the Google C++ Coding Style.

However, large parts of the code were written before rules and in particular the reference to the Google C++ Coding Style were enacted, and due to the size of the code, this misalignment cannot be fixed in the short term. To avoid that an arbitrary mixture of old-style and new-style code grows, this document makes deviations from the “global” code style explicit, and will be amended to describe migration paths in the future.

In addition, to achieve higher consistency within the components maintained by the team and to reduce style discussions during reviews, allowing them to focus on more substantial issues, more specific rules are described here that go beyond the global code style. These topics might have been deliberately or accidentally omitted from the global code style. Depending on wider agreement and applicability, these specific rules might be migrated into the global code style in the future.

Note that this document does not cover pure formatting issues. The code is and must be kept formatted automatically by clang-format using the supplied configuration file, and whatever clang-format does takes precedence over any other stated rules regarding formatting.

Deviations from the Google C++ Coding Style

Deviations not documented yet.

Deviations from the Mozilla C++ Coding Style

Mozilla style	Prevalent WAS style	Deviation scope	Evolution
We prefer using “static”, instead of anonymous C++ namespaces.	Place all symbols that should have internal linkage in a single anonymous namespace block at the top of an implementation file, rather than declaring them static.	All files	Unclear. The recommendation in the Mozilla code style says this might change in the future depending on debugger support, so this deviation might become obsolete.
All parameters passed by lvalue reference must be labeled const. […] Input parameters may be const pointers, but we never allow non-const reference parameters except when required by convention, e.g., swap().	Non-const reference parameters may be used.	All files	Unclear. Maybe at least restrict the use of non-const reference parameters to cases that are not clearly output parameters (i.e. which are assigned to).

Additions to the Google/Mozilla C++ Code Style

This section contains style guidelines that do not conflict with the Google or Mozilla C++ Code Style, but may make guidelines more specific or add guidelines on topics not covered by those style guides at all.

Naming

gtest test names

gtest constructs a full test name from different fragments. Test names are constructed somewhat differently for basic and parametrized tests.

The prefix for a test should start with an identifier of the component and class, based on the name of the source code directory, transformed to PascalCase and underscores as separators, so e.g. for a class Key in dom/indexedDB, use DOM_IndexedDB_Key as a prefix.

For basic tests constructed with TEST(test_case_name, test_name): Use the prefix as the test_case_name. Test test_name should start with the name of tested method(s), and a . Use underscores as a separator within the test_name.

Value-parametrized tests are constructed with TEST_P(parametrized_test_case_name, parametrized_test_name). They require a custom test base class, whose name is used as the parametrized_test_case_name. Start the class name with TestWithParam_, and end it with a transliteration of the parameter type (e.g. String_Int_Pair for std::pair<nsString, int>), and place it in an (anonymous) namespace.

Attention

It is important to place the class in an (anonymous) namespace, since its name according to this guideline is not unique within libxul-gtest, and name clashes are likely, which would lead to ODR violations otherwise.

A parametrized_test_name is constructed according to the same rules described for test_name above.

Instances of value-parametrized tests are constructed using INSTANTIATE_TEST_CASE_P(prefix, parametrized_test_case_name, generator, ...). As prefix, use the prefix as described above.

Similar considerations apply to type-parametrized tests. If necessary, specific rules for type-parametrized tests will be added here.

Rationale

All gtests (not only from the WAS components) are linked into libxul-gtest, which requires names to be unique within that large scope. In addition, it should be clear from the test name (e.g. in the test execution log) in what source file (or at least which directory) the test code can be found. Optimally, test names should be structured hierarchically to allow easy selection of groups of tests for execution. However, gtest has some restrictions that do not allow that completely. The guidelines try to accommodate for these as far as possible. Note that gtest recommends not to use underscores in test names in general, because this may lead to reserved names and naming conflicts, but the rules stated here should avoid that. In case of any problems arising, we can evolve the rules to accommodate for that.

Specifying types

Use of auto for declaring variables

The Google C++ Code Style on auto allows the use of auto generally with encouragements for specific cases, which still leaves a rather wide range for interpretation.

We extend this by some more encouragements and discouragements:

• DO use auto when the type is already present in the initialization expression (esp. a template argument or similar), e.g. auto c = static_cast<uint16_t>(*(iter++)) << 8; or auto x =  MakeRefPtr<MediaStreamError>(mWindow, *aError);

• DO use auto if the spelled out type were complex otherwise, e.g. a nested typedef or type alias, e.g. foo_container::value_type.

• DO NOT use auto if the type were spelled out as a builtin integer type or one of the types from <cstdint>, e.g. instead of auto foo = funcThatReturnsUint16(); use uint16_t foo = funcThatReturnsUint16();.

Note

Some disadvantages of using auto relate to the unavailability of type information outside an appropriate IDE/editor. This may be somewhat remedied by resolving Bug 1567464 which will make the type information available in searchfox. In consequence, the guidelines might be amended to promote a more widespread use of auto.

Pointer types

Plain pointers

The use of plain pointers is error-prone. Avoid using owning plain pointers. In particular, avoid using literal, non-placement new. There are various kinds of smart pointers, not all of which provide appropriate factory functions. However, where such factory functions exist, do use them (along with auto). The following is an incomplete list of smart pointer types and corresponding factory functions:

Type	Factory function	Header file
mozilla::RefPtr	mozilla::MakeRefPtr	"mfbt/RefPtr.h"
mozilla::UniquePtr	mozilla::MakeUnique	"mfbt/UniquePtr.h"
std::unique_ptr	std::make_unique	<memory>
std::shared_ptr	std::make_shared	<memory>

Also, to create an already_AddRefed<> to pass as a parameter or return from a function without the need to dereference it, use MakeAndAddRef instead of creating a dereferenceable RefPtr (or similar) first and then using .forget().

Smart pointers

In function signatures, prefer accepting or returning RefPtr instead of already_AddRefed in conjunction with regular std::move rather than .forget(). This improves readability and code generation. Prevailing legimitate uses of already_AddRefed are described in its documentation.

Prefer using mozilla::UniquePtr over nsAutoPtr, since the latter is deprecated (and e.g. has no factory function, see Bug 1600079).

Use nsCOMPtr<T> iff T is an XPCOM interface type (more details on MDN <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/nsCOMPtr_versus_RefPtr>).

Enums

Use scoped resp. strongly typed enums (enum struct) rather than non-scoped enums. Use PascalCase for naming the values of scoped enums.

Evolution Process

This section explains the process to evolve the coding style described in this document. For clarity, we will distinguish coding tasks from code style evolution tasks in this section.

Managing code style evolution tasks

A code style evolution task is a task that ought to amend or revise the coding style as described in this document.

Code style evolution tasks should be managed in Bugzilla, as individual bugs for each topic. All such tasks should block the meta-bug 1586788 <https://bugzilla.mozilla.org/show_bug.cgi?id=1586788>.

When you take on to work on a code style evolution task:

• The task may already include a sketch of a resolution. If no preferred solution is obvious, discuss options to resolve it via comments on the bug first.

• When the general idea is ready to be spelled out in this document, amend or revise it accordingly.

• Submit the changes to this document as a patch to Phabricator, and put it up for review. Since this will affect a number of people, every change should be reviewed by at least two people. Ideally, this should include the owner of this style document and one person with good knowledge of the parts of the code base this style applies to.

• If there are known violations of the amendment to the coding style, consider fixing some of them, so that the amendment is tested on actual code. If the code style evolution task refers to a particular code location from a review, at least that location should be fixed to comply with the amended coding style.

• When you have two r+, land the patch.

• Report on the addition in the next team meeting to raise awareness.

Basis for code style evolution tasks

The desire or necessity to evolve the code style can originate from different activities, including - reviews - reading or writing code locally - reading the coding style - general thoughts on coding style

The code style should not be cluttered with aspects that are rarely relevant or rarely leads to discussions, as the maintenance of the code style has a cost as well. The code style should be as comprehensive as necessary to reduce the overall maintenance costs of the code and code style combined.

A particular focus is therefore on aspects that led to some discussion in a code review, as reducing the number or verbosity of necessary style discussions in reviews is a major indicator for the effectiveness of the documented style.

Evolving code style based on reviews

The goal of the process described here is to take advantage of style-related discussions that originate from a code review, but to decouple evolution of the code style from the review process, so that it does not block progress on the underlying bug.

The following should be considered when performing a review:

• Remind yourself of the code style, maybe skim through the document before starting the review, or have it open side-by-side while doing the review.

• If you find a violation of an existing rule, add an inline comment.

• Have an eye on style-relevant aspects in the code itself or after a discussions with the author. Consider if this could be generalized into a style rule, but is not yet covered by the documented global or local style. This might be something that is in a different style as opposed to other locations, differs from your personal style, etc.

• In that case, find an acceptable temporary solution for the code fragments at hand, which is acceptable for an r+ of the patch. Maybe agree with the code author on adding a comment that this should be revised later, when a rule is codified.

• Create a code style evolution task in Bugzilla as described above. In the description of the bug, reference the review comment that gave rise to it. If you can suggest a resolution, include that in the description, but this is not a necessary condition for creating the task.

Improving code style compliance when writing code

Periodically look into the code style document, and remind yourself of its rules, and give particular attention to recent changes.

When writing code, i.e. adding new code or modify existing code, remind yourself of checking the code for style compliance.

Time permitting, resolve existing violations on-the-go as part of other work in the code area. Submit such changes in dedicated patches. If you identify major violations that are too complex to resolve on-the-go, consider creating a bug dedicated to the resolution of that violation, which then can be scheduled in the planning process.

Syncing with the global Mozilla C++ Coding Style

Several aspects of the coding style described here will be applicable to the overall code base. However, amendments to the global coding style will affect a large number of code authors and may require extended discussion. Deviations from the global coding style should be limited in the long term. On the other hand, amendments that are not relevant to all parts of the code base, or where it is difficult to reach a consensus at the global scope, may make sense to be kept in the local style.

The details of synchronizing with the global style are subject to discussion with the owner and peers of the global coding style (see Bug 1587810 <https://bugzilla.mozilla.org/show_bug.cgi?id=1587810>).

FAQ

• When someone introduces new code that adheres to the current style, but the remainder of the function/class/file does not, is it their responsibility to update that remainder on-the-go?

  The code author is not obliged to update the remainder, but they are encouraged to do so, time permitting. Whether that is the case depends on a number of factors, including the number and complexity of existing style violations, the risk introduced by changing that on the go etc. Judging this is left to the code author. At the very least, the function/class/file should not be left in a worse state than before.

• Are stylistic inconsistencies introduced by applying the style as defined here only to new code considered acceptable?

  While this is certainly not optimal, accepting such inconsistencies to some degree is inevitable to allow making progress towards an improved style. Personal preferences regarding the degree may differ, but in doubt such inconsistencies should be considered acceptable. They should not block a bug from being closed.