diff options
Diffstat (limited to 'docs/code-quality/coding-style')
| -rw-r--r-- | docs/code-quality/coding-style/about-logins-rtl.png | bin | 0 -> 6257 bytes | |||
| -rw-r--r-- | docs/code-quality/coding-style/about-protections-rtl.png | bin | 0 -> 2310 bytes | |||
| -rw-r--r-- | docs/code-quality/coding-style/coding_style_cpp.rst | 1150 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/coding_style_general.rst | 18 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/coding_style_java.rst | 68 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/coding_style_js.rst | 147 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/coding_style_python.rst | 71 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/css_guidelines.rst | 572 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst | 272 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/index.rst | 20 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/rtl_guidelines.rst | 356 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/svg_guidelines.rst | 347 | ||||
| -rw-r--r-- | docs/code-quality/coding-style/using_cxx_in_firefox_code.rst | 1075 |
13 files changed, 4096 insertions, 0 deletions
diff --git a/docs/code-quality/coding-style/about-logins-rtl.png b/docs/code-quality/coding-style/about-logins-rtl.png Binary files differnew file mode 100644 index 0000000000..a5d0edd4c8 --- /dev/null +++ b/docs/code-quality/coding-style/about-logins-rtl.png diff --git a/docs/code-quality/coding-style/about-protections-rtl.png b/docs/code-quality/coding-style/about-protections-rtl.png Binary files differnew file mode 100644 index 0000000000..4fbbf5e889 --- /dev/null +++ b/docs/code-quality/coding-style/about-protections-rtl.png diff --git a/docs/code-quality/coding-style/coding_style_cpp.rst b/docs/code-quality/coding-style/coding_style_cpp.rst new file mode 100644 index 0000000000..cb4764cea5 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_cpp.rst @@ -0,0 +1,1150 @@ +================ +C++ Coding style +================ + + +This document attempts to explain the basic styles and patterns used in +the Mozilla codebase. New code should try to conform to these standards, +so it is as easy to maintain as existing code. There are exceptions, but +it's still important to know the rules! + +This article is particularly for those new to the Mozilla codebase, and +in the process of getting their code reviewed. Before requesting a +review, please read over this document, making sure that your code +conforms to recommendations. + +.. container:: blockIndicator warning + + The Firefox code base adopts parts of the `Google Coding style for C++ + code <https://google.github.io/styleguide/cppguide.html>`__, but not all of its rules. + A few rules are followed across the code base, others are intended to be + followed in new or significantly revised code. We may extend this list in the + future, when we evaluate the Google Coding Style for C++ Code further and/or update + our coding practices. However, the plan is not to adopt all rules of the Google Coding + Style for C++ Code. Some rules are explicitly unlikely to be adopted at any time. + + Followed across the code base: + + - `Formatting <https://google.github.io/styleguide/cppguide.html#Formatting>`__, + except for subsections noted here otherwise + - `Implicit Conversions <https://google.github.io/styleguide/cppguide.html#Implicit_Conversions>`__, + which is enforced by a custom clang-plugin check, unless explicitly overridden using + ``MOZ_IMPLICIT`` + + Followed in new/significantly revised code: + + - `Include guards <https://google.github.io/styleguide/cppguide.html#The__define_Guard>`__ + + Unlikely to be ever adopted: + + - `Forward declarations <https://google.github.io/styleguide/cppguide.html#Forward_Declarations>`__ + - `Formatting/Conditionals <https://google.github.io/styleguide/cppguide.html#Conditionals>`__ + w.r.t. curly braces around inner statements, we require them in all cases where the + Google style allows to leave them out for single-line conditional statements + + This list reflects the state of the Google Google Coding Style for C++ Code as of + 2020-07-17. It may become invalid when the Google modifies its Coding Style. + + +Formatting code +--------------- + +Formatting is done automatically via clang-format, and controlled via in-tree +configuration files. See :ref:`Formatting C++ Code With clang-format` +for more information. + +Unix-style linebreaks (``\n``), not Windows-style (``\r\n``). You can +convert patches, with DOS newlines to Unix via the ``dos2unix`` utility, +or your favorite text editor. + +Static analysis +--------------- + +Several of the rules in the Google C++ coding styles and the additions mentioned below +can be checked via clang-tidy (some rules are from the upstream clang-tidy, some are +provided via a mozilla-specific plugin). Some of these checks also allow fixes to +be automatically applied. + +``mach static-analysis`` provides a convenient way to run these checks. For example, +for the check called ``google-readability-braces-around-statements``, you can run: + +.. code-block:: shell + + ./mach static-analysis check --checks="-*,google-readability-braces-around-statements" --fix <file> + +It may be necessary to reformat the files after automatically applying fixes, see +:ref:`Formatting C++ Code With clang-format`. + +Additional rules +---------------- + +*The norms in this section should be followed for new code. For existing code, +use the prevailing style in a file or module, ask the owner if you are +in another team's codebase or it's not clear what style to use.* + + + + +Control structures +~~~~~~~~~~~~~~~~~~ + +Always brace controlled statements, even a single-line consequent of +``if else else``. This is redundant, typically, but it avoids dangling +else bugs, so it's safer at scale than fine-tuning. + +Examples: + +.. code-block:: cpp + + 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; + } + +``else`` should only ever be followed by ``{`` or ``if``; i.e., other +control keywords are not allowed and should be placed inside braces. + +.. note:: + + For this rule, clang-tidy provides the ``google-readability-braces-around-statements`` + check with autofixes. + + +C++ namespaces +~~~~~~~~~~~~~~ + +Mozilla project C++ declarations should be in the ``mozilla`` +namespace. Modules should avoid adding nested namespaces under +``mozilla``, unless they are meant to contain names which have a high +probability of colliding with other names in the code base. For example, +``Point``, ``Path``, etc. Such symbols can be put under +module-specific namespaces, under ``mozilla``, with short +all-lowercase names. Other global namespaces besides ``mozilla`` are +not allowed. + +No ``using`` directives are allowed in header files, except inside class +definitions or functions. (We don't want to pollute the global scope of +compilation units that use the header file.) + +.. note:: + + For parts of this rule, clang-tidy provides the ``google-global-names-in-headers`` + check. It only detects ``using namespace`` directives in the global namespace. + + +``using namespace ...;`` is only allowed in ``.cpp`` files after all +``#include``\ s. Prefer to wrap code in ``namespace ... { ... };`` +instead, if possible. ``using namespace ...;``\ should always specify +the fully qualified namespace. That is, to use ``Foo::Bar`` do not +write ``using namespace Foo; using namespace Bar;``, write +``using namespace Foo::Bar;`` + +Use nested namespaces (ex: ``namespace mozilla::widget {`` + +.. note:: + + clang-tidy provides the ``modernize-concat-nested-namespaces`` + check with autofixes. + + +Anonymous namespaces +~~~~~~~~~~~~~~~~~~~~ + +We prefer using ``static``, instead of anonymous C++ namespaces. This may +change once there is better debugger support (especially on Windows) for +placing breakpoints, etc. on code in anonymous namespaces. You may still +use anonymous namespaces for things that can't be hidden with ``static``, +such as types, or certain objects which need to be passed to template +functions. + + +C++ classes +~~~~~~~~~~~~ + +.. code-block:: cpp + + namespace mozilla { + + class MyClass : public A + { + ... + }; + + class MyClass + : public X + , public Y + { + public: + MyClass(int aVar, int aVar2) + : mVar(aVar) + , mVar2(aVar2) + { + ... + } + + // Special member functions, like constructors, that have default bodies + // should use '= default' annotation instead. + MyClass() = default; + + // Unless it's a copy or move constructor or you have a specific reason to allow + // implicit conversions, mark all single-argument constructors explicit. + explicit MyClass(OtherClass aArg) + { + ... + } + + // This constructor can also take a single argument, so it also needs to be marked + // explicit. + explicit MyClass(OtherClass aArg, AnotherClass aArg2 = AnotherClass()) + { + ... + } + + int LargerFunction() + { + ... + ... + } + + private: + int mVar; + }; + + } // namespace mozilla + +Define classes using the style given above. + +.. note:: + + For the rule on ``= default``, clang-tidy provides the ``modernize-use-default`` + check with autofixes. + + For the rule on explicit constructors and conversion operators, clang-tidy + provides the ``mozilla-implicit-constructor`` check. + +Existing classes in the global namespace are named with a short prefix +(For example, ``ns``) as a pseudo-namespace. + + +Methods and functions +~~~~~~~~~~~~~~~~~~~~~ + + +C/C++ +^^^^^ + +In C/C++, method names should use ``UpperCamelCase``. + +Getters that never fail, and never return null, are named ``Foo()``, +while all other getters use ``GetFoo()``. Getters can return an object +value, via a ``Foo** aResult`` outparam (typical for an XPCOM getter), +or as an ``already_AddRefed<Foo>`` (typical for a WebIDL getter, +possibly with an ``ErrorResult& rv`` parameter), or occasionally as a +``Foo*`` (typical for an internal getter for an object with a known +lifetime). See `the bug 223255 <https://bugzilla.mozilla.org/show_bug.cgi?id=223255>`_ +for more information. + +XPCOM getters always return primitive values via an outparam, while +other getters normally use a return value. + +Method declarations must use, at most, one of the following keywords: +``virtual``, ``override``, or ``final``. Use ``virtual`` to declare +virtual methods, which do not override a base class method with the same +signature. Use ``override`` to declare virtual methods which do +override a base class method, with the same signature, but can be +further overridden in derived classes. Use ``final`` to declare virtual +methods which do override a base class method, with the same signature, +but can NOT be further overridden in the derived classes. This should +help the person reading the code fully understand what the declaration +is doing, without needing to further examine base classes. + +.. note:: + + For the rule on ``virtual/override/final``, clang-tidy provides the + ``modernize-use-override`` check with autofixes. + + +Operators +~~~~~~~~~ + +The unary keyword operator ``sizeof``, should have its operand parenthesized +even if it is an expression; e.g. ``int8_t arr[64]; memset(arr, 42, sizeof(arr));``. + + +Literals +~~~~~~~~ + +Use ``\uXXXX`` unicode escapes for non-ASCII characters. The character +set for XUL, script, and properties files is UTF-8, which is not easily +readable. + + +Prefixes +~~~~~~~~ + +Follow these naming prefix conventions: + + +Variable prefixes +^^^^^^^^^^^^^^^^^ + +- k=constant (e.g. ``kNC_child``). Not all code uses this style; some + uses ``ALL_CAPS`` for constants. +- g=global (e.g. ``gPrefService``) +- a=argument (e.g. ``aCount``) +- C++ Specific Prefixes + + - s=static member (e.g. ``sPrefChecked``) + - m=member (e.g. ``mLength``) + - e=enum variants (e.g. ``enum Foo { eBar, eBaz }``). Enum classes + should use ``CamelCase`` instead (e.g. + ``enum class Foo { Bar, Baz }``). + + +Global functions/macros/etc +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- Macros begin with ``MOZ_``, and are all caps (e.g. + ``MOZ_WOW_GOODNESS``). Note that older code uses the ``NS_`` prefix; + while these aren't being changed, you should only use ``MOZ_`` for + new macros. The only exception is if you're creating a new macro, + which is part of a set of related macros still using the old ``NS_`` + prefix. Then you should be consistent with the existing macros. + + +Error Variables +^^^^^^^^^^^^^^^ + +- Local variables that are assigned ``nsresult`` result codes should be named ``rv`` + (i.e., e.g., not ``res``, not ``result``, not ``foo``). `rv` should not be + used for bool or other result types. +- Local variables that are assigned ``bool`` result codes should be named `ok`. + + +C/C++ practices +--------------- + +- **Have you checked for compiler warnings?** Warnings often point to + real bugs. `Many of them <https://searchfox.org/mozilla-central/source/build/moz.configure/warnings.configure>`__ + are enabled by default in the build system. +- In C++ code, use ``nullptr`` for pointers. In C code, using ``NULL`` + or ``0`` is allowed. + +.. note:: + + For the C++ rule, clang-tidy provides the ``modernize-use-nullptr`` check + with autofixes. + +- Don't use ``PRBool`` and ``PRPackedBool`` in C++, use ``bool`` + instead. +- For checking if a ``std`` container has no items, don't use + ``size()``, instead use ``empty()``. +- When testing a pointer, use ``(!myPtr)`` or ``(myPtr)``; + don't use ``myPtr != nullptr`` or ``myPtr == nullptr``. +- Do not compare ``x == true`` or ``x == false``. Use ``(x)`` or + ``(!x)`` instead. ``if (x == true)`` may have semantics different from + ``if (x)``! + +.. note:: + + clang-tidy provides the ``readability-simplify-boolean-expr`` check + with autofixes that checks for these and some other boolean expressions + that can be simplified. + +- In general, initialize variables with ``nsFoo aFoo = bFoo,`` and not + ``nsFoo aFoo(bFoo)``. + + - For constructors, initialize member variables with : ``nsFoo + aFoo(bFoo)`` syntax. + +- To avoid warnings created by variables used only in debug builds, use + the + `DebugOnly<T> <https://developer.mozilla.org/docs/Mozilla/Debugging/DebugOnly%3CT%3E>`__ + helper when declaring them. +- You should `use the static preference + API <https://firefox-source-docs.mozilla.org/modules/libpref/index.html>`__ for + working with preferences. +- One-argument constructors, that are not copy or move constructors, + should generally be marked explicit. Exceptions should be annotated + with ``MOZ_IMPLICIT``. +- Use ``char32_t`` as the return type or argument type of a method that + returns or takes as argument a single Unicode scalar value. (Don't + use UTF-32 strings, though.) +- Prefer unsigned types for semantically-non-negative integer values. +- When operating on integers that could overflow, use ``CheckedInt``. +- Avoid the usage of ``typedef``, instead, please use ``using`` instead. + +.. note:: + + For parts of this rule, clang-tidy provides the ``modernize-use-using`` + check with autofixes. + + +Header files +------------ + +Since the Firefox code base is huge and uses a monolithic build, it is +of utmost importance for keeping build times reasonable to limit the +number of included files in each translation unit to the required minimum. +Exported header files need particular attention in this regard, since their +included files propagate, and many of them are directly or indirectly +included in a large number of translation units. + +- Include guards are named per the Google coding style (i.e. upper snake + case with a single trailing underscore). They should not include a + leading ``MOZ_`` or ``MOZILLA_``. For example, ``dom/media/foo.h`` + would use the guard ``DOM_MEDIA_FOO_H_``. +- 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"``. + If a "forwarding header" is provided for a type, include that instead of + putting the literal forward declaration(s) in your header file. E.g. for + some JavaScript types, there is ``js/TypeDecls.h``, for the string types + there is ``StringFwd.h``. One reason for this is that this allows + changing a type to a type alias by only changing the forwarding header. + The following uses of a type can be done with a forward declaration only: + + - Parameter or return type in a function declaration + - Member/local variable pointer or reference type + - Use as a template argument (not in all cases) in a member/local variable type + - Defining a type alias + + The following uses of a type require a full definition: + + - Base class + - Member/local variable type + - Use with delete or new + - Use as a template argument (not in all cases) + - Any uses of non-scoped enum types + - Enum values of a scoped enum type + + Use as a template argument is somewhat tricky. It depends on how the + template uses the type. E.g. ``mozilla::Maybe<T>`` and ``AutoTArray<T>`` + always require a full definition of ``T`` because the size of the + template instance depends on the size of ``T``. ``RefPtr<T>`` and + ``UniquePtr<T>`` don't require a full definition (because their + pointer member always has the same size), but their destructor + requires a full definition. If you encounter a template that cannot + be instantiated with a forward declaration only, but it seems + it should be possible, please file a bug (if it doesn't exist yet). + + Therefore, also consider the following guidelines to allow using forward + declarations as widely as possible. +- Inline function bodies in header files often pull in a lot of additional + dependencies. Be mindful when adding or extending inline function bodies, + and consider moving the function body to the cpp file or to a separate + header file that is not included everywhere. Bug 1677553 intends to provide + a more specific guideline on this. +- Consider the use of the `Pimpl idiom <https://en.cppreference.com/w/cpp/language/pimpl>`__, + i.e. hide the actual implementation in a separate ``Impl`` class that is + defined in the implementation file and only expose a ``class Impl;`` forward + declaration and ``UniquePtr<Impl>`` member in the header file. +- Do not use non-scoped enum types. These cannot be forward-declared. Use + scoped enum types instead, and forward declare them when possible. +- Avoid nested types that need to be referenced from outside the class. + These cannot be forward declared. Place them in a namespace instead, maybe + in an extra inner namespace, and forward declare them where possible. +- Avoid mixing declarations with different sets of dependencies in a single + header file. This is generally advisable, but even more so when some of these + declarations are used by a subset of the translation units that include the + combined header file only. Consider such a badly mixed header file like: + + .. code-block:: cpp + + /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=8 sts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #ifndef BAD_MIXED_FILE_H_ + #define BAD_MIXED_FILE_H_ + + // Only this include is needed for the function declaration below. + #include "nsCOMPtr.h" + + // These includes are only needed for the class definition. + #include "nsIFile.h" + #include "mozilla/ComplexBaseClass.h" + + namespace mozilla { + + class WrappedFile : public nsIFile, ComplexBaseClass { + // ... class definition left out for clarity + }; + + // Assuming that most translation units that include this file only call + // the function, but don't need the class definition, this should be in a + // header file on its own in order to avoid pulling in the other + // dependencies everywhere. + nsCOMPtr<nsIFile> CreateDefaultWrappedFile(nsCOMPtr<nsIFile>&& aFileToWrap); + + } // namespace mozilla + + #endif // BAD_MIXED_FILE_H_ + + +An example header file based on these rules (with some extra comments): + +.. code-block:: cpp + + /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=8 sts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #ifndef DOM_BASE_FOO_H_ + #define DOM_BASE_FOO_H_ + + // Include guards should come at the very beginning and always use exactly + // the style above. Otherwise, compiler optimizations that avoid rescanning + // repeatedly included headers might not hit and cause excessive compile + // times. + + #include <cstdint> + #include "nsCOMPtr.h" // This is needed because we have a nsCOMPtr<T> data member. + + class nsIFile; // Used as a template argument only. + enum class nsresult : uint32_t; // Used as a parameter type only. + template <class T> + class RefPtr; // Used as a return type only. + + namespace mozilla::dom { + + class Document; // Used as a template argument only. + + // Scoped enum, not as a nested type, so it can be + // forward-declared elsewhere. + enum class FooKind { Small, Big }; + + class Foo { + public: + // Do not put the implementation in the header file, it would + // require including nsIFile.h + Foo(nsCOMPtr<nsIFile> aFile, FooKind aFooKind); + + RefPtr<Document> CreateDocument(); + + void SetResult(nsresult aResult); + + // Even though we will default this destructor, do this in the + // implementation file since we would otherwise need to include + // nsIFile.h in the header. + ~Foo(); + + private: + nsCOMPtr<nsIFile> mFile; + }; + + } // namespace mozilla::dom + + #endif // DOM_BASE_FOO_H_ + + +Corresponding implementation file: + +.. code-block:: cpp + + /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=8 sts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #include "mozilla/dom/Foo.h" // corresponding header + + #include "mozilla/Assertions.h" // Needed for MOZ_ASSERT. + #include "mozilla/dom/Document.h" // Needed because we construct a Document. + #include "nsError.h" // Needed because we use NS_OK aka nsresult::NS_OK. + #include "nsIFile.h" // This is needed because our destructor indirectly calls delete nsIFile in a template instance. + + namespace mozilla::dom { + + // Do not put the implementation in the header file, it would + // require including nsIFile.h + Foo::Foo(nsCOMPtr<nsIFile> aFile, FooKind aFooKind) + : mFile{std::move(aFile)} { + } + + RefPtr<Document> Foo::CreateDocument() { + return MakeRefPtr<Document>(); + } + + void Foo::SetResult(nsresult aResult) { + MOZ_ASSERT(aResult != NS_OK); + + // do something with aResult + } + + // Even though we default this destructor, do this in the + // implementation file since we would otherwise need to include + // nsIFile.h in the header. + Foo::~Foo() = default; + + } // namespace mozilla::dom + + +Include directives +------------------ + +- Ordering: + + - In an implementation file (cpp file), the very first include directive + should include the corresponding header file, followed by a blank line. + - Any conditional includes (depending on some ``#ifdef`` or similar) follow + after non-conditional includes. Don't mix them in. + - Don't place comments between non-conditional includes. + + Bug 1679522 addresses automating the ordering via clang-format, which + is going to enforce some stricter rules. Expect the includes to be reordered. + If you include third-party headers that are not self-contained, and therefore + need to be included in a particular order, enclose those (and only those) + between ``// clang-format off`` and ``// clang-format on``. This should not be + done for Mozilla headers, which should rather be made self-contained if they + are not. + +- Brackets vs. quotes: C/C++ standard library headers are included using + brackets (e.g. ``#include <cstdint>``), all other include directives use + (double) quotes (e.g. ``#include "mozilla/dom/Document.h``). +- Exported headers should always be included from their exported path, not + from their source path in the tree, even if available locally. E.g. always + do ``#include "mozilla/Vector.h"``, not ``#include "Vector.h"``, even + from within `mfbt`. +- Generally, you should include exactly those headers that are needed, not + more and not less. Unfortunately this is not easy to see. Maybe C++20 + modules will bring improvements to this, but it will take a long time + to be adopted. +- The basic rule is that if you literally use a symbol in your file that + is declared in a header A.h, include that header. In particular in header + files, check if a forward declaration or including a forwarding header is + sufficient, see section :ref:`Header files`. + + There are cases where this basic rule is not sufficient. Some cases where + you need to include additional headers are: + + - You reference a member of a type that is not literally mentioned in your + code, but, e.g. is the return type of a function you are calling. + + There are also cases where the basic rule leads to redundant includes. Note + that "redundant" here does not refer to "accidentally redundant" headers, + e.g. at the time of writing ``mozilla/dom/BodyUtil.h`` includes + ``mozilla/dom/FormData.h``, but it doesn't need to (it only needs a forward + declaration), so including ``mozilla/dom/FormData.h`` is "accidentally + redundant" when including ``mozilla/dom/BodyUtil.h``. The includes of + ``mozilla/dom/BodyUtil.h`` might change at any time, so if a file that + includes ``mozilla/dom/BodyUtil.h`` needs a full definition of + ``mozilla::dom::FormData``, it should includes ``mozilla/dom/FormData.h`` + itself. In fact, these "accidentally redundant" headers MUST be included. + Relying on accidentally redundant includes makes any change to a header + file extremely hard, in particular when considering that the set of + accidentally redundant includes differs between platforms. + But some cases in fact are non-accidentally redundant, and these can and + typically should not be repeated: + + - The includes of the header file do not need to be repeated in its + corresponding implementation file. Rationale: the implementation file and + its corresponding header file are tightly coupled per se. + + Macros are a special case. Generally, the literal rule also applies here, + i.e. if the macro definition references a symbol, the file containing the + macro definition should include the header defining the symbol. E.g. + ``NS_IMPL_CYCLE_COLLECTING_NATIVE_RELEASE`` defined in ``nsISupportsImpl.h`` + makes use of ``MOZ_ASSERT`` defined in ``mozilla/Assertions.h``, so + ``nsISupportsImpl.h`` includes ``mozilla/Assertions.h``. However, this + requires human judgment of what is intended, since technically only the + invocations of the macro reference a symbol (and that's how + include-what-you-use handles this). It might depend on the + context or parameters which symbol is actually referenced, and sometimes + this is on purpose. In these cases, the user of the macro needs to include + the required header(s). + + + +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`` is when you are storing refcounted pointers in a + long-lived datastructure. +- Declare new XPCOM interfaces using :doc:`XPIDL </xpcom/xpidl>`, so they + will be scriptable. +- Use :doc:`nsCOMPtr </xpcom/refptr>` for strong references, and + ``nsWeakPtr`` for weak references. +- Don't use ``QueryInterface`` directly. Use ``CallQueryInterface`` or + ``do_QueryInterface`` instead. +- Use :ref:`Contract IDs <contract_ids>`, + instead of CIDs with ``do_CreateInstance``/``do_GetService``. +- Use pointers, instead of references for function out parameters, even + for primitive types. + + +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: + +.. code-block:: cpp + + 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 an attribute. Using attributes logically connects the getting and +setting of a value, and makes scripted code look cleaner. + +This example has too many methods: + +.. code-block:: cpp + + 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. + +.. code-block:: cpp + + 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: + +.. code-block:: cpp + + const long ERROR_UNDEFINED_VARIABLE = 1; + + +See also +~~~~~~~~ + +For details on interface development, as well as more detailed style +guides, see the `Interface development +guide <https://developer.mozilla.org/docs/Mozilla/Developer_guide/Interface_development_guide>`__. + + +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 make + your call in the first place. + +Also, when you make a new function which is failable (i.e. it will +return a ``nsresult`` or a ``bool`` that may indicate an error), you should +explicitly mark the return value should always be checked. For example: + +:: + + // for IDL. + [must_use] nsISupports + create(); + + // for C++, add this in *declaration*, do not add it again in implementation. + [[nodiscard]] nsresult + DoSomething(); + +There are some exceptions: + +- Predicates or getters, which return ``bool`` or ``nsresult``. +- IPC method implementation (For example, ``bool RecvSomeMessage()``). +- Most callers will check the output parameter, see below. + +.. code-block:: cpp + + nsresult + SomeMap::GetValue(const nsString& key, nsString& value); + +If most callers need to check the output value first, then adding +``[[nodiscard]]`` might be too verbose. In this case, change the return value +to void might be a reasonable choice. + +There is also a static analysis attribute ``[[nodiscard]]``, which can +be added to class declarations, to ensure that those declarations are +always used when they are returned. + + +Use the NS_WARN_IF macro when errors are unexpected. +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ``NS_WARN_IF`` macro can be used to issue a console warning, in debug +builds if the condition fails. This should only be used when the failure +is unexpected and cannot be caused by normal web content. + +If you are writing code which wants to issue warnings when methods fail, +please either use ``NS_WARNING`` directly, or use the new ``NS_WARN_IF`` macro. + +.. code-block:: cpp + + if (NS_WARN_IF(somethingthatshouldbefalse)) { + return NS_ERROR_INVALID_ARG; + } + + if (NS_WARN_IF(NS_FAILED(rv))) { + return rv; + } + +Previously, the ``NS_ENSURE_*`` macros were used for this purpose, but +those macros hide return statements, and should not be used in new code. +(This coding style rule isn't generally agreed, so use of ``NS_ENSURE_*`` +can be valid.) + + +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: + +.. code-block:: cpp + + rv = foo->Call1(); + if (NS_SUCCEEDED(rv)) { + rv = foo->Call2(); + if (NS_SUCCEEDED(rv)) { + rv = foo->Call3(); + } + } + return rv; + +Instead, do this: + +.. code-block:: cpp + + rv = foo->Call1(); + if (NS_FAILED(rv)) { + return rv; + } + + rv = foo->Call2(); + if (NS_FAILED(rv)) { + return rv; + } + + rv = foo->Call3(); + if (NS_FAILED(rv)) { + return rv; + } + +Why? 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. Wrapping the calls in nested if() statements, instead +obscured the most likely behavior of the code. + +Consider a more complicated example to hide a bug: + +.. code-block:: cpp + + bool 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. It is not clear from this code. Here is +an updated version: + +.. code-block:: cpp + + bool 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 trivial that one of these notifications failed: + +.. code-block:: cpp + + for (size_t i = 0; i < length; ++i) { + // we don't care if any individual observer fails + observers[i]->Observe(foo, bar, baz); + } + +Another possibility, is you are not sure if a component exists or is +installed, and you wish to continue normally, if the component is not +found. + +.. code-block:: cpp + + 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 +------- + +.. note:: + + This section overlaps with the more verbose advice given in + :doc:`String guide </xpcom/stringguide>`. + These should eventually be merged. For now, please refer to that guide for + more advice. + +- String arguments to functions should be declared as ``[const] nsA[C]String&``. +- Prefer using string literals. In particular, use empty string literals, + i.e. ``u""_ns`` or ``""_ns``, instead of ``Empty[C]String()`` or + ``const nsAuto[C]String empty;``. Use ``Empty[C]String()`` only if you + specifically need a ``const ns[C]String&``, e.g. with the ternary operator + or when you need to return/bind to a reference or take the address of the + empty string. +- For 16-bit literal strings, use ``u"..."_ns`` or, if necessary + ``NS_LITERAL_STRING_FROM_CSTRING(...)`` instead of ``nsAutoString()`` + or other ways that would do a run-time conversion. + See :ref:`Avoid runtime conversion of string literals <Avoid runtime conversion of string literals>` below. +- To compare a string with a literal, use ``.EqualsLiteral("...")``. +- Use ``str.IsEmpty()`` instead of ``str.Length() == 0``. +- Use ``str.Truncate()`` instead of ``str.SetLength(0)``, + ``str.Assign(""_ns)`` or ``str.AssignLiteral("")``. +- Don't use functions from ``ctype.h`` (``isdigit()``, ``isalpha()``, + etc.) or from ``strings.h`` (``strcasecmp()``, ``strncasecmp()``). + These are locale-sensitive, which makes them inappropriate for + processing protocol text. At the same time, they are too limited to + work properly for processing natural-language text. Use the + alternatives in ``mozilla/TextUtils.h`` and in ``nsUnicharUtils.h`` + in place of ``ctype.h``. In place of ``strings.h``, prefer the + ``nsStringComparator`` facilities for comparing strings or if you + have to work with zero-terminated strings, use ``nsCRT.h`` for + ASCII-case-insensitive comparison. + + +Use the ``Auto`` form of strings for local values +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When declaring a local, short-lived ``nsString`` class, always use +``nsAutoString`` or ``nsAutoCString``. These pre-allocate a 64-byte +buffer on the stack, and avoid fragmenting the heap. Don't do this: + +.. code-block:: cpp + + nsresult + foo() + { + nsCString bar; + .. + } + +instead: + +.. code-block:: cpp + + nsresult + foo() + { + nsAutoCString 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 using that function inline in your code, +without freeing the value. Consider this code: + +.. code-block:: cpp + + 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: + +.. code-block:: cpp + + static void + GetStringValue(nsAWritableCString& aResult) + { + .. + aResult.Assign("resulting string"); + } + + .. + nsAutoCString warning; + GetStringValue(warning); + WarnUser(warning.get()); + +Free the string manually: + +.. code-block:: cpp + + static char* + GetStringValue() + { + .. + return resultString.ToNewCString(); + } + + .. + char* warning = GetStringValue(); + WarnUser(warning); + nsMemory::Free(warning); + +.. _Avoid runtime conversion of string literals: + +Avoid runtime conversion of string literals +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +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 +``AssignLiteral`` and ``AppendLiteral``, use a user-defined literal like `u"foo"_ns` +instead. On most platforms, this will force the compiler to compile in a +raw unicode string, and assign it directly. In cases where the literal is defined +via a macro that is used in both 8-bit and 16-bit ways, you can use +`NS_LITERAL_STRING_FROM_CSTRING` to do the conversion at compile time. + +Incorrect: + +.. code-block:: cpp + + nsAutoString warning; + warning.AssignLiteral("danger will robinson!"); + ... + foo->SetStringValue(warning); + ... + bar->SetUnicodeValue(warning.get()); + +Correct: + +.. code-block:: cpp + + constexpr auto warning = u"danger will robinson!"_ns; + ... + // if you'll be using the 'warning' string, you can still use it as before: + foo->SetStringValue(warning); + ... + bar->SetUnicodeValue(warning.get()); + + // alternatively, use the wide string directly: + foo->SetStringValue(u"danger will robinson!"_ns); + ... + + // if a macro is the source of a 8-bit literal and you cannot change it, use + // NS_LITERAL_STRING_FROM_CSTRING, but only if necessary. + #define MY_MACRO_LITERAL "danger will robinson!" + foo->SetStringValue(NS_LITERAL_STRING_FROM_CSTRING(MY_MACRO_LITERAL)); + + // If you need to pass to a raw const char16_t *, there's no benefit to + // go through our string classes at all, just do... + bar->SetUnicodeValue(u"danger will robinson!"); + + // .. or, again, if a macro is the source of a 8-bit literal + bar->SetUnicodeValue(u"" MY_MACRO_LITERAL); + + +Usage of PR_(MAX|MIN|ABS|ROUNDUP) macro calls +--------------------------------------------- + +Use the standard-library functions (``std::max``), instead of +``PR_(MAX|MIN|ABS|ROUNDUP)``. + +Use ``mozilla::Abs`` instead of ``PR_ABS``. All ``PR_ABS`` calls in C++ code have +been replaced with ``mozilla::Abs`` calls, in `bug +847480 <https://bugzilla.mozilla.org/show_bug.cgi?id=847480>`__. All new +code in ``Firefox/core/toolkit`` needs to ``#include "nsAlgorithm.h"`` and +use the ``NS_foo`` variants instead of ``PR_foo``, or +``#include "mozilla/MathAlgorithms.h"`` for ``mozilla::Abs``. + +Use of SpiderMonkey rooting typedefs +------------------------------------ +The rooting typedefs in ``js/public/TypeDecls.h``, such as ``HandleObject`` and +``RootedObject``, are deprecated both in and outside of SpiderMonkey. They will +eventually be removed and should not be used in new code. diff --git a/docs/code-quality/coding-style/coding_style_general.rst b/docs/code-quality/coding-style/coding_style_general.rst new file mode 100644 index 0000000000..950cd6ccd3 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_general.rst @@ -0,0 +1,18 @@ + +Mode line +~~~~~~~~~ + +Files should have Emacs and vim mode line comments as the first two +lines of the file, which should set ``indent-tabs-mode`` to ``nil``. For new +files, use the following, specifying two-space indentation: + +.. code-block:: cpp + + /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +Be sure to use the correct ``Mode`` in the first line, don't use ``C++`` in +JavaScript files. diff --git a/docs/code-quality/coding-style/coding_style_java.rst b/docs/code-quality/coding-style/coding_style_java.rst new file mode 100644 index 0000000000..f2206d8e2d --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_java.rst @@ -0,0 +1,68 @@ +================= +Java Coding style +================= + +- We use the `Java Coding + Style <https://www.oracle.com/technetwork/java/codeconvtoc-136057.html>`__. + Quick summary: + + - FirstLetterUpperCase for class names. + - camelCase for method and variable names. + - One declaration per line: + + .. code-block:: java + + int x, y; // this is BAD! + int a; // split it over + int b; // two lines + +- Braces should be placed like so (generally, opening braces on same + line, closing braces on a new line): + + .. code-block:: java + + public void func(int arg) { + if (arg != 0) { + while (arg > 0) { + arg--; + } + } else { + arg++; + } + } + +- Places we differ from the Java coding style: + + - Start class variable names with 'm' prefix (e.g. + mSomeClassVariable) and static variables with 's' prefix (e.g. + sSomeStaticVariable) + - ``import`` statements: + + - Do not use wildcard imports like \`import java.util.*;\` + - Organize imports by blocks separated by empty line: + org.mozilla.*, android.*, com.*, net.*, org.*, then java.\* + This is basically what Android Studio does by default, except + that we place org.mozilla.\* at the front - please adjust + Settings -> Editor -> Code Style -> Java -> Imports + accordingly. + - Within each import block, alphabetize import names with + uppercase before lowercase. For example, ``com.example.Foo`` is + before ``com.example.bar`` + + - 4-space indents. + - Spaces, not tabs. + - Don't restrict yourself to 80-character lines. Google's Android + style guide suggests 100-character lines, which is also the + default setting in Android Studio. Java code tends to be long + horizontally, so use appropriate judgement when wrapping. Avoid + deep indents on wrapping. Note that aligning the wrapped part of a + line, with some previous part of the line (rather than just using + a fixed indent), may require shifting the code every time the line + changes, resulting in spurious whitespace changes. + +- For additional specifics on Firefox for Android, see the `Coding + Style guide for Firefox on + Android <https://wiki.mozilla.org/Mobile/Fennec/Android#Coding_Style>`__. +- The `Android Coding + Style <https://source.android.com/source/code-style.html>`__ has some + useful guidelines too. diff --git a/docs/code-quality/coding-style/coding_style_js.rst b/docs/code-quality/coding-style/coding_style_js.rst new file mode 100644 index 0000000000..09d7a6fc8a --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_js.rst @@ -0,0 +1,147 @@ +======================= +JavaScript Coding style +======================= + +Coding style +~~~~~~~~~~~~ + +`prettier <https://prettier.io/>`_ is the tool used to reformat the JavaScript code. + + +Methods and functions +~~~~~~~~~~~~~~~~~~~~~ + +In JavaScript, functions should use camelCase, but should not capitalize +the first letter. Methods should not use the named function expression +syntax, because our tools understand method names: + +.. code-block:: cpp + + doSomething: function (aFoo, aBar) { + ... + } + +In-line functions should have spaces around braces, except before commas +or semicolons: + +.. code-block:: cpp + + function valueObject(aValue) { return { value: aValue }; } + + +JavaScript objects +~~~~~~~~~~~~~~~~~~ + +.. code-block:: cpp + + var foo = { prop1: "value1" }; + + var bar = { + prop1: "value1", + prop2: "value2" + }; + +Constructors for objects should be capitalized and use Pascal Case: + +.. code-block:: cpp + + function ObjectConstructor() { + this.foo = "bar"; + } + + +Operators +~~~~~~~~~ + +In JavaScript, overlong expressions not joined by ``&&`` and +``||`` should break so the operator starts on the second line and +starting in the same column as the beginning of the expression in the +first line. This applies to ``?:``, binary arithmetic operators +including ``+``, and member-of operators. Rationale: an operator at the +front of the continuation line makes for faster visual scanning, as +there is no need to read to the end of line. Also there exists a +context-sensitive keyword hazard in JavaScript; see {{bug(442099, "bug", +19)}}, which can be avoided by putting . at the start of a continuation +line, in long member expression. + +In JavaScript, ``==`` is preferred to ``===``. + +Unary keyword operators, such as ``typeof``, should have their operand +parenthesized; e.g. ``typeof("foo") == "string"``. + +Literals +~~~~~~~~ + +Double-quoted strings (e.g. ``"foo"``) are preferred to single-quoted +strings (e.g. ``'foo'``), in JavaScript, except to avoid escaping +embedded double quotes, or when assigning inline event handlers. + + +Prefixes +~~~~~~~~ + +- k=constant (e.g. ``kNC_child``). Not all code uses this style; some + uses ``ALL_CAPS`` for constants. +- g=global (e.g. ``gPrefService``) +- a=argument (e.g. ``aCount``) + +- JavaScript Specific Prefixes + + - \_=member (variable or function) (e.g. ``_length`` or + ``_setType(aType)``) + - k=enumeration value (e.g. ``const kDisplayModeNormal = 0``) + - on=event handler (e.g. ``function onLoad()``) + - Convenience constants for interface names should be prefixed with + ``nsI``: + + .. code-block:: javascript + + const nsISupports = Components.interfaces.nsISupports; + const nsIWBN = Components.interfaces.nsIWebBrowserNavigation; + + + +Other advices +~~~~~~~~~~~~~ + +- Do not compare ``x == true`` or ``x == false``. Use ``(x)`` or + ``(!x)`` instead. ``x == true``, is certainly different from if + ``(x)``! Compare objects to ``null``, numbers to ``0`` or strings to + ``""``, if there is chance for confusion. +- Make sure that your code doesn't generate any strict JavaScript + warnings, such as: + + - Duplicate variable declaration. + - Mixing ``return;`` with ``return value;`` + - Undeclared variables or members. If you are unsure if an array + value exists, compare the index to the array's length. If you are + unsure if an object member exists, use ``"name"`` in ``aObject``, + or if you are expecting a particular type you may use + ``typeof(aObject.name) == "function"`` (or whichever type you are + expecting). + +- Use ``['value1, value2']`` to create a JavaScript array in preference + to using + ``new {{JSxRef("Array", "Array", "Syntax", 1)}}(value1, value2)`` + which can be confusing, as ``new Array(length)`` will actually create + a physically empty array with the given logical length, while + ``[value]`` will always create a 1-element array. You cannot actually + guarantee to be able to preallocate memory for an array. +- Use ``{ member: value, ... }`` to create a JavaScript object; a + useful advantage over ``new {{JSxRef("Object", "Object", "", 1)}}()`` + is the ability to create initial properties and use extended + JavaScript syntax, to define getters and setters. +- If having defined a constructor you need to assign default + properties, it is preferred to assign an object literal to the + prototype property. +- Use regular expressions, but use wisely. For instance, to check that + ``aString`` is not completely whitespace use + ``/\S/.{{JSxRef("RegExp.test", "test(aString)", "", 1)}}``. Only use + {{JSxRef("String.search", "aString.search()")}} if you need to know + the position of the result, or {{JSxRef("String.match", + "aString.match()")}} if you need to collect matching substrings + (delimited by parentheses in the regular expression). Regular + expressions are less useful if the match is unknown in advance, or to + extract substrings in known positions in the string. For instance, + {{JSxRef("String.slice", "aString.slice(-1)")}} returns the last + letter in ``aString``, or the empty string if ``aString`` is empty. diff --git a/docs/code-quality/coding-style/coding_style_python.rst b/docs/code-quality/coding-style/coding_style_python.rst new file mode 100644 index 0000000000..3a818fcfd4 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_python.rst @@ -0,0 +1,71 @@ +=================== +Python Coding style +=================== + +Coding style +~~~~~~~~~~~~ + + :ref:`black` is the tool used to reformat the Python code. + +Linting +~~~~~~~ + +The Python linting is done by :ref:`Flake8` and :ref:`pylint` +They are executed by mozlint both at review phase and in the CI. + +Indentation +~~~~~~~~~~~ + +Four spaces in Python code. + + +Makefile/moz.build practices +---------------------------- + +- Changes to makefile and moz.build variables do not require + build-config peer review. Any other build system changes, such as + adding new scripts or rules, require review from the build-config + team. +- Suffix long ``if``/``endif`` conditionals with #{ & #}, so editors + can display matched tokens enclosing a block of statements. + + :: + + ifdef CHECK_TYPE #{ + ifneq ($(flavor var_type),recursive) #{ + $(warning var should be expandable but detected var_type=$(flavor var_type)) + endif #} + endif #} + +- moz.build are python and follow normal Python style. +- List assignments should be written with one element per line. Align + closing square brace with start of variable assignment. If ordering + is not important, variables should be in alphabetical order. + + .. code-block:: python + + var += [ + 'foo', + 'bar' + ] + +- Use ``CONFIG['CPU_ARCH'] {=arm}`` to test for generic classes of + architecture rather than ``CONFIG['OS_TEST'] {=armv7}`` (re: bug 886689). + + +Other advices +~~~~~~~~~~~~~ + +- Install the + `mozext <https://hg.mozilla.org/hgcustom/version-control-tools/file/default/hgext/mozext>`__ + Mercurial extension, and address every issue reported on commit + or the output of ``hg critic``. +- Follow `PEP 8 <https://www.python.org/dev/peps/pep-0008/>`__. Please run :ref:`black` for this. +- Do not place statements on the same line as ``if/elif/else`` + conditionals to form a one-liner. +- Global vars, please avoid them at all cost. +- Exclude outer parenthesis from conditionals.Use + ``if x > 5:,``\ rather than ``if (x > 5):`` +- Use string formatters, rather than var + str(val). + ``var = 'Type %s value is %d'% ('int', 5).`` +- Testing/Unit tests, please write them and make sure that they are executed in the CI. diff --git a/docs/code-quality/coding-style/css_guidelines.rst b/docs/code-quality/coding-style/css_guidelines.rst new file mode 100644 index 0000000000..e962f24d69 --- /dev/null +++ b/docs/code-quality/coding-style/css_guidelines.rst @@ -0,0 +1,572 @@ +CSS Guidelines +============== + +This document contains guidelines defining how CSS inside the Firefox +codebase should be written, it is notably relevant for Firefox front-end +engineers. + +Basics +------ + +Here are some basic tips that can optimize reviews if you are changing +CSS: + +- Avoid ``!important`` but if you have to use it, make sure it's + obvious why you're using it (ideally with a comment). The + `Overriding CSS`_ section contains more information about this. +- Avoid magic numbers; prefer automatic sizing or alignment methods. + Some examples to avoid: + + - absolutely positioned elements + - hardcoded values such as: ``vertical-align: -2px;`` . The reason + you should avoid such "hardcoded" values is that, they don't + necessarily work for all font-size configurations. + +- Avoid setting styles in JavaScript. It's generally better to set a + class and then specify the styles in CSS. +- ``classList`` is generally better than ``className``. There's less + chance of overwriting an existing class. +- Only use generic selectors such as ``:last-child``, when it is what + you mean semantically. If not, using a semantic class name is more + descriptive and usually better. + +Boilerplate +~~~~~~~~~~~ + +Make sure each file starts with the standard copyright header (see +`License Boilerplate <https://www.mozilla.org/MPL/headers/>`__). + +Before adding more CSS +~~~~~~~~~~~~~~~~~~~~~~ + +It is good practice to check if the CSS that is being written is needed, +it can be the case that a common component has been already written +could be reused with or without changes. Most of the time, the common +component already follows the a11y/theme standards defined in this +guide. So, when possible, always prefer editing common components to +writing your own. + +Also, it is good practice to introduce a common class when the new +element you are styling reuses some styles from another element, this +allows the maintenance cost and the amount of code duplication to be +reduced. + +Formatting +---------- + +Spacing & Indentation +~~~~~~~~~~~~~~~~~~~~~ + +- 2 spaces indentation is preferred +- Add a space after each comma, **except** within color functions: + +.. code:: css + + linear-gradient(to bottom, black 1px, rgba(255,255,255,0.2) 1px) + +- Always add a space before ``!important``. + +Omit units on 0 values +~~~~~~~~~~~~~~~~~~~~~~ + +Do this: + +.. code:: css + + margin: 0; + +Not this: + +.. code:: css + + margin: 0px; + +Use expanded syntax +~~~~~~~~~~~~~~~~~~~ + +It is often harder to understand what the shorthand is doing and the +shorthand can also hide some unwanted default values. It is good to +privilege expanded syntax to make your intentions explicit. + +Do this: + +.. code:: css + + border-color: red; + +Not this: + +.. code:: css + + border: red; + +Put multiple selectors on different lines +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Do this: + +.. code:: css + + h1, + h2, + h3 { + font-family: sans-serif; + text-align: center; + } + +Not this: + +.. code:: css + + h1, h2, h3 { + font-family: sans-serif; + text-align: center; + } + +Naming standards for class names +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +- ``lower-case-with-dashes`` is the most common. +- But ``camelCase`` is also used sometimes. Try to follow the style of + existing or related code. + +Other tips +~~~~~~~~~~ + +- Assume ``="true"`` in attribute selectors. + + - Example: Use ``option[checked]``, not ``option[checked="true"]``. + +- Avoid ID selectors unless it is really the wanted goal, since IDs + have higher specificity and therefore are harder to override. +- Using descendant selectors is good practice for performance when + possible: + + - For example: + ``.autocomplete-item[selected] > .autocomplete-item-title`` would + be more efficient than + ``.autocomplete-item[selected] .autocomplete-item-title`` + +Overriding CSS +-------------- + +Before overriding any CSS rules, check whether overriding is really +needed. Sometimes, when copy-pasting older code, it happens that the +code in question contains unnecessary overrides. This could be because +the CSS that it was overriding got removed in the meantime. In this +case, dropping the override should work. + +It is also good practice to look at whether the rule you are overriding +is still needed: maybe the UX spec for the component has changed and +that rule can actually be updated or removed. When this is the case, +don't be afraid to remove or update that rule. + +Once the two things above have been checked, check if the other rule you +are overriding contains ``!important``, if that is case, try putting it +in question, because it might have become obsolete. + +Afterwards, check the specificity of the other selector; if it is +causing your rule to be overridden, you can try reducing its +specificity, either by simplifying the selector or by changing where the +rule is placed in the stylesheet. If this isn't possible, you can also +try introducing a ``:not()`` to prevent the other rule from applying, +this is especially relevant for different element states (``:hover``, +``:active``, ``[checked]`` or ``[disabled]``). However, never try to +increase the selector of the rule you are adding as it can easily become +hard to understand. + +Finally, once you have checked all the things above, you can permit +yourself to use ``!important`` along with a comment why it is needed. + +Using CSS variables +------------------- + +Adding new variables +~~~~~~~~~~~~~~~~~~~~ + +Before adding new CSS variables, please consider the following +questions: + +#. **Is the variable value changed at runtime?** + *(Either from JavaScript or overridden by another CSS file)* + **If the answer is no**, consider using a preprocessor variable or + inlining the value. + +#. **Is the variable value used multiple times?** + **If the answer is no and the value isn't changed at runtime**, then + you likely don't need a CSS variable. + +#. **Is there an alternative to using the variable like inheriting or + using the ``currentcolor`` keyword?** + Using inheriting or using ``currentcolor`` will prevent repetition of + the value and it is usually good practice to do so. + +In general, it's good to first think of how some CSS could be written +cleanly without the CSS variable(s) and then think of how the CSS +variable could improve that CSS. + +Using variables +~~~~~~~~~~~~~~~ + +Use the variable according to its naming +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Do this: + +.. code:: css + + xul|tab:hover { + background-color: var(--in-content-box-background-hover); + } + +Not this: + +.. code:: css + + #certificateErrorDebugInformation { + background-color: var(--in-content-box-background-hover); + } + +Localization +------------ + +Text Direction +~~~~~~~~~~~~~~ + +- For margins, padding and borders, use + ``inline-start``/``inline-end`` rather than ``left``/``right``. + *Example:* Use ``margin-inline-start: 3px;`` instead of + ``margin-left: 3px``. +- For RTL-aware positioning (left/right), use + ``inset-inline-start``/``inset-inline-end``. +- For RTL-aware float layouts, ``float: inline-start|inline-end`` can + be used instead of ``float: left|right``. +- The RTL-aware equivalents of + ``border-{top/bottom}-{left/right}-radius`` are + ``border-{start/end}-{start/end}-radius`` +- When there is no special RTL-aware property available, use the pseudo + ``:-moz-locale-dir(ltr|rtl)`` (for XUL files) or ``:dir(ltr|rtl)`` + (for HTML files). +- Remember that while a tab content's scrollbar still shows on the + right in RTL, an overflow scrollbar will show on the left. +- Write ``padding: 0 3px 4px;`` instead of + ``padding: 0 3px 4px 3px;``. This makes it more obvious that the + padding is symmetrical (so RTL won't be an issue). + +.. note:: + + See `CSS Logical Properties and + Values <https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Logical_Properties>`__ + for more information. + +Testing +~~~~~~~ + +To test for RTL layouts, you can go to ``about:config`` and set +``intl.uidirection`` to ``-1``. + +Writing cross-platform CSS +-------------------------- + +Firefox supports many different platforms and each of those platforms +can contain many different configurations: + +- Windows 7, 8 and 10 + + - Default theme + - Aero basic (Windows 7, 8) + - Windows classic (Windows 7) + - High contrast (All versions) + +- Linux +- macOS + +File structure +~~~~~~~~~~~~~~ + +- The ``browser/`` directory contains styles specific to Firefox +- The ``toolkit/`` directory contains styles that are shared across all + toolkit applications (Thunderbird and SeaMonkey) + +Under each of those two directories, there is a ``themes`` directory +containing 4 sub-directories: + +- ``shared`` +- ``linux`` +- ``osx`` +- ``windows`` + +The ``shared`` directories contain styles shared across all 3 platforms, +while the other 3 directories contain styles respective to their +platform. + +For new CSS, when possible try to privilege using the ``shared`` +directory, instead of writing the same CSS for the 3 platform specific +directories, especially for large blocks of CSS. + +Content CSS vs. Theme CSS +^^^^^^^^^^^^^^^^^^^^^^^^^ + +The following directories also contain CSS: + +- ``browser/base/content/`` +- ``toolkit/content/`` + +These directories contain content CSS, that applies on all platforms, +which is styling deemed to be essential for the browser to behave +correctly. To determine whether some CSS is theme-side or content-side, +it is useful to know that certain CSS properties are going to lean one +way or the other: color - 99% of the time it will be theme CSS, overflow +- 99% content. + ++-----------------+--------------+----------------+----------------+ +| 99% theme | 70% theme | 70% content | 99% content | ++=================+==============+================+================+ +| font-\*, color, | line-height, | cursor, width, | overflow, | +| \*-color, | padding, | max-width, | direction, | +| border-\*, | margin | top, | display, | +| -moz-appearance | | bottom [2]_, | \*-align, | +| [1]_ | | etc | align-\*, | +| | | | \*-box-\*, | +| | | | flex-\*, order | ++-----------------+--------------+----------------+----------------+ + +If some CSS is layout or functionality related, then it is likely +content CSS. If it is esthetics related, then it is likely theme CSS. + +When importing your stylesheets, it's best to import the content CSS +before the theme CSS, that way the theme values get to override the +content values (which is probably what you want), and you're going to +want them both after the global values, so your imports will look like +this: + +.. code:: html + + <?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?> + <?xml-stylesheet href="chrome://browser/content/path/module.css" type="text/css"?> + <?xml-stylesheet href="chrome://browser/skin/path/module.css" type="text/css"?> + +.. [1] -moz-appearance is tricky. Generally, when specifying + -moz-appearance: foo; you're giving hints as to how something should + act, however -moz-appearance: none; is probably saying 'ignore + browser preconceptions - I want a blank sheet', so that's more + visual. However -moz-appearance values aren't implemented and don't + behave consistently across platforms, so idealism aside + -moz-appearance should always be in theme CSS. + +.. [2] However there is probably a better way than using absolute + positioning. + +Colors +~~~~~~ + +For common areas of the Firefox interface (panels, toolbar buttons, +etc.), mozilla-central often comes with some useful CSS variables that +are adjusted with the correct values for different platform +configurations, so using those CSS variables can definitively save some +testing time, as you can assume they already work correctly. + +Using the ``currentcolor`` keyword or inheriting is also good practice, +because sometimes the needed value is already in the color or on the +parent element. This is especially useful in conjunction with icons +using ``-moz-context-properties: fill;`` where the icon can adjust to +the right platform color automatically from the text color. It is also +possible to use ``currentcolor`` with other properties like +``opacity`` or ``fill-opacity`` to have different +opacities of the platform color. + +High contrast mode +~~~~~~~~~~~~~~~~~~ + +Content area +^^^^^^^^^^^^ + +On Windows high contrast mode, in the content area, Gecko does some +automatic color adjustments regarding page colors. Part of those +adjustments include making all ``box-shadow`` invisible, so this is +something to be aware of if you create a focus ring or a border using +the ``box-shadow`` property: consider using a ``border`` or an +``outline`` if you want the border/focus ring to stay visible in +high-contrast mode. An example of such bug is `bug +1516767 <https://bugzilla.mozilla.org/show_bug.cgi?id=1516767>`__. + +Another adjustment to be aware of is that Gecko removes all the +``background-image`` when high contrast mode is enabled. Consider using +an actual ``<img>`` tag (for HTML documents) or ``list-style-image`` +(for XUL documents) if rendering the image is important. + +If you are not using Windows, one way to test against those adjustments +on other platforms is: + +- Going to about:preferences +- Clicking on the "Colors..." button in the "Fonts & Colors" + sub-section of the "Language and Appearance" section +- Under "Override the colors specified by the page with your selections + above", select the "Always" option + +Chrome area +^^^^^^^^^^^ + +The automatic adjustments previously mentioned only apply to pages +rendered in the content area. The chrome area of Firefox uses colors as +authored, which is why using pre-defined variables, ``currentcolor`` or +inheritance is useful to integrate with the system theme with little +hassle. + +If not, as a last resort, using `system +colors <https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#system_colors>`__ +also works for non-default Windows themes or Linux. In general, the +following colors are used: + +- ``-moz-Field``: textbox or field background colors, also used as the + background color of listboxes or trees. +- ``-moz-FieldText``: textbox or field text colors, also used as the + text color of listboxes or trees. +- ``-moz-Dialog``: window or dialog background color. +- ``-moz-DialogText``: window or dialog text color. +- ``GrayText``: used on disabled items as text color. Do not use it on + text that is not disabled to desemphsize text, because it does not + guarantee a sufficient contrast ratio for non-disabled text. +- ``ThreeDShadow``: Used as border on elements. +- ``ThreeDLightShadow``: Used as light border on elements. + +Using the background/text pairs is especially important to ensure the +contrast is respected in all situations. Never mix custom text colors +with a system background color and vice-versa. + +Note that using system colors is only useful for the chrome area, since +content area colors are overridden by Gecko anyway. + +Writing media queries +~~~~~~~~~~~~~~~~~~~~~ + +Boolean media queries +^^^^^^^^^^^^^^^^^^^^^ + +Do this: + +.. code:: css + + @media (-moz-mac-yosemite-theme: 0) { + +Not this: + +.. code:: css + + @media not all and (-moz-mac-yosemite-theme) { + +Privilege CSS for most common configuration +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +It is better to put the most common configuration (latest version of an +OS, or default theme for example) outside of the media query. In the +following example, ``-moz-mac-yosemite-theme`` targets macOS 10.10 and +higher, so it should be privileged over the styling for macOS 10.9. + +Do this: + +.. code:: css + + @media (-moz-mac-yosemite-theme: 0) { + #placesList { + box-shadow: inset -2px 0 0 hsla(0,0%,100%,.2); + } + } + +Not this: + +.. code:: css + + #placesList { + box-shadow: inset -2px 0 0 hsla(0,0%,100%,.2); + } + + @media (-moz-mac-yosemite-theme) { + #placesList { + box-shadow: none; + } + } + +Theme support +------------- + +Firefox comes built-in with 3 themes: default, light and dark. The +built-in light/dark themes are a bit special as they load the +``compacttheme.css`` stylesheet. In addition to this, Firefox supports a +variety of WebExtension themes that can be installed from AMO. For +testing purposes, `here is an example of a WebExtension +theme. <https://addons.mozilla.org/en-US/firefox/addon/arc-dark-theme-we/>`__ + +Writing theme-friendly CSS +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +- Some CSS variables that are pre-adjusted for different platforms are + also pre-adjusted for themes, so it's again a good idea to use them + for theme support. +- The text color of elements often contains valuable information from + the theme colors, so ``currentcolor``/inheritance is again a good + idea for theme support. +- Never write CSS specially for the built-in light/dark theme in + ``compacttheme.css`` unless that CSS isn't supposed to affect + WebExtension themes. +- These selectors can be used to target dark areas: + + - ``:-moz-lwtheme-brighttext``: dark window frame. + - ``:root[lwt-toolbar-field-brighttext]``: dark address bar and + searchbar. + - ``:root[lwt-popup-brighttext]``: dark arrow panels and + autocomplete panels. + - ``:root[lwt-sidebar-brighttext]``: dark sidebars. + +- If you'd like a different shade of a themed area and no CSS variable + is adequate, using colors with alpha transparency is usually a good + idea, as it will preserve the original theme author's color hue. + +Variables +~~~~~~~~~ + +For clarity, CSS variables that are only used when a theme is enabled +have the ``--lwt-`` prefix. + +Layout & performance +-------------------- + +Layout +~~~~~~ + +Mixing XUL flexbox and HTML flexbox can lead to undefined behavior. + +CSS selectors +~~~~~~~~~~~~~ + +When targeting the root element of a page, using ``:root`` is the most +performant way of doing so. + +Reflows and style flushes +~~~~~~~~~~~~~~~~~~~~~~~~~ + +See :ref:`Performance best practices for Firefox front-end engineers` +for more information about this. + +Misc +---- + +Text aliasing +~~~~~~~~~~~~~ + +When convenient, avoid setting the ``opacity`` property on +text as it will cause text to be aliased differently. + +HDPI support +~~~~~~~~~~~~ + +It's recommended to use SVG since it keeps the CSS clean when supporting +multiple resolutions. See the :ref:`SVG Guidelines` for more information +on SVG usage. + +However, if only 1x and 2x PNG assets are available, you can use this +``@media`` query to target higher density displays (HDPI): + +.. code:: css + + @media (min-resolution: 1.1dppx) diff --git a/docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst b/docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst new file mode 100644 index 0000000000..ca64835410 --- /dev/null +++ b/docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst @@ -0,0 +1,272 @@ +===================================== +Formatting C++ Code With clang-format +===================================== + +Mozilla uses the Google coding style for whitespace, which is enforced +using `clang-format <https://clang.llvm.org/docs/ClangFormat.html>`__. A +specific version of the binary will be installed when +``./mach clang-format`` or ``./mach bootstrap`` are run. We build our +own binaries and update them as needed. + +Options are explicitly defined `in clang-format +itself <https://github.com/llvm-mirror/clang/blob/e8a55f98df6bda77ee2eaa7f7247bd655f79ae0e/lib/Format/Format.cpp#L856>`__. +If the options are changed in clang upstream, this might cause some +changes in the Firefox tree. For this reason, it is best to use the +mozilla-provided binaries. + +Manual formatting +----------------- + +We provide a mach subcommand for running clang-format from the +command-line. This wrapper handles ensuring the correct version of +clang-format is installed and run. + +If clang-format isn’t installed, the binaries will be automatically +downloaded from taskcluster and installed into ~/.mozbuild. We build our +own clang-format binaries. + + +Formatting local changes +~~~~~~~~~~~~~~~~~~~~~~~~ + +:: + + $ ./mach clang-format + +When run without arguments, it will run on a local diff. This could miss +some reformatting (for example, when blocks are touched). +(`searchfox <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/python/mozbuild/mozbuild/code-analysis/mach_commands.py#1620>`__) + + +Formatting specific paths +~~~~~~~~~~~~~~~~~~~~~~~~~ + +:: + + $ ./mach clang-format -p <path> # Format <path> in-place + $ ./mach clang-format -p <path> -s # Show changes + +The command also accepts a ``-p`` argument to reformat a specific +directory or file, and a ``-s`` flag to show the changes instead of +applying them to the working directory +(`searchfox <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/python/mozbuild/mozbuild/code-analysis/mach_commands.py#1633>`__) + + +Formatting specific commits / revisions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +:: + + $ ./mach clang-format -c HEAD # Format a single git commit + $ ./mach clang-format -c HEAD~~..HEAD # Format a range of git commits + $ ./mach clang-format -c . # Format a single mercurial revision + +The command accepts a ``-c`` argument that takes a revision number or +commit range, and will format the lines modified by those commits. +(`searchfox <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/python/mozbuild/mozbuild/code-analysis/mach_commands.py#1635>`__) + + +Scripting Clang-Format +~~~~~~~~~~~~~~~~~~~~~~ + +Clang format expects that the path being passed to it is the path +on-disk. If this is not the case, for example when formatting a +temporary file, the "real" path must be specified. This can be done with +the ``--assume-filename <path>`` argument. + + +Configuring the clang-format commit hook +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +To run clang-format at commit phase, run ``mach boostrap`` or just add +the following line in the ``hgrc`` file: + +.. code:: ini + + [extensions] + clang-format = ~/.mozbuild/version-control-tools/hgext/clang-format + +We use a hg extension as they are more flexible than hooks. + +With git, the configuration is the following: + +:: + + # From the root git directory: + $ ln -s $(pwd)/tools/lint/hooks_clang_format.py .git/hooks/pre-commit + +You'll likely need to install the ``python-hglib`` package for your OS, +or else you may get errors like ``abort: No module named hglib.client!`` +when you try to commit. + + +Editor integration +------------------ + +It is possible to configure many editors to support running +``clang-format`` automatically on save, or when run from within the +editor. + + +Editor plugins +~~~~~~~~~~~~~~ + +- `Atom <https://atom.io/packages/clang-format>`__ +- `BBEdit <http://clang.llvm.org/docs/ClangFormat.html#bbedit-integration>`__ + + - `clang-format-bbedit.applescript <https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/clang-format-bbedit.applescript>`__ + +- Eclipse + + - Install the + `CppStyle <https://marketplace.eclipse.org/content/cppstyle>`__ + plugin + - In Preferences -> C/C++ -> CppStyle, set the clang-format path to + ~/.mozbuild/clang-tools/clang-tidy/bin/clang-format + - (Optional) check "Run clang-format on file save" + +- `Emacs <http://clang.llvm.org/docs/ClangFormat.html#emacs-integration>`__ + + - `clang-format.el <https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/clang-format.el>`__ + (Or install + `clang-format <http://melpa.org/#/clang-format>`__ from MELPA) + - `google-c-style <http://melpa.org/#/google-c-style>`__ from MELPA + +- `Sublime Text <https://packagecontrol.io/packages/Clang%20Format>`__ + + - `alternative + tool <https://github.com/rosshemsley/SublimeClangFormat>`__ + +- `Vim <http://clang.llvm.org/docs/ClangFormat.html#vim-integration>`__ + + - `clang-format.py <https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/clang-format.py>`__ + - `vim-clang-format <https://github.com/rhysd/vim-clang-format>`__ + +- `Visual + Studio <https://marketplace.visualstudio.com/items?itemName=LLVMExtensions.ClangFormat>`__ + + - `llvm.org plugin <http://llvm.org/builds/>`__ + - `Integrated support in Visual Studio + 2017 <https://blogs.msdn.microsoft.com/vcblog/2018/03/13/clangformat-support-in-visual-studio-2017-15-7-preview-1/>`__ + +- `Visual Studio + Code <https://marketplace.visualstudio.com/items?itemName=xaver.clang-format>`__ +- `XCode <https://github.com/travisjeffery/ClangFormat-Xcode>`__ +- `Script for patch + reformatting <http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting>`__ + + - `clang-format-diff.py <https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/clang-format-diff.py>`__ + + +Configuration +~~~~~~~~~~~~~ + +These tools generally run clang-format themselves, and won't use +``./mach clang-format``. The binary installed by our tooling will be +located at ``~/.mozbuild/clang-tools/clang-tidy/bin/clang-format``. + +You typically shouldn't need to specify any other special configuration +in your editor besides the clang-format binary. Most of the +configuration that clang-format relies on for formatting is stored +inside our source tree. More specifically, using the .clang-format file +located in the root of the repository. Please note that this doesn't +include the list of ignored files and directories (provided by +.clang-format-ignore which is a feature provided by the mach command +wrapper). + +Coding style configuration is done within clang-format itself. When we +change the configuration (incorrect configuration, new feature in clang, +etc), we use `local +overrides <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/.clang-format>`__. + + +Ignored files & directories +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +We maintain a `list of ignored directories and +files <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/.clang-format-ignore>`__, +which is used by ``./mach clang-format``. This is generally only used +for code broken by clang-format, and third-party code. + + +Ignored code hunks +~~~~~~~~~~~~~~~~~~ + +Sections of code may have formatting disabled using comments. If a +section must not be formatted, the following comments will disable the +reformat: + +:: + + // clang-format off + my code which should not be reformatted + // clang-format on + +You can find an `example of code not +formatted <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/xpcom/io/nsEscape.cpp#22>`__. + + +Merging formatted and unformatted code +-------------------------------------- + +During the transition to using chromium style enforced by clang-format +for all code in tree, it will often be necessary to rebase non-formatted +code onto a formatted tree. + + +Mercurial +~~~~~~~~~ + +The ``format-source`` extension, now bundled with +``version-control-tools``, and installed by ``./mach bootstrap``, may be +used to seamlessly handle this situation. More details may be found in +this +`document <https://docs.google.com/document/d/13AwAsvKMhH0mflDlfatBqn6LmZHiQih76oxM4zfrPl4/edit>`__. + +The parent changeset of the reformat has been tagged as +``PRE_TREEWIDE_CLANG_FORMAT``. + + +Git +~~~ + +To perform a rebase onto mozilla-central after the merge, a handy merge +driver, ``clang-format-merge``, has been written: + +.. code:: shell + + $ git clone https://github.com/emilio/clang-format-merge + $ /path/to/clang-format-merge/git-wrapper rebase <upstream> + +The wrapper should clean up after itself, and the clone may be deleted +after the rebase is complete. + + +Ignore lists +------------ + +To make sure that the blame/annotate features of Mercurial or git aren't +affected. Two files are maintained to keep track of the reformatting +commits. + + +With Mercurial +~~~~~~~~~~~~~~ + +| The list is stored in + `https://searchfox.org/mozilla-central/source/.hg-annotate-ignore-revs </en-US/docs/>`__ +| Commit messages should also contain the string ``# ignore-this-changeset`` + +The syntax in this file is generated using the following syntax: + +:: + + $ hg log --template '{node} - {author|person} - {desc|strip|firstline}\n' + +With git +~~~~~~~~ + +The list is stored in +`https://searchfox.org/mozilla-central/source/.git-blame-ignore-revs </en-US/docs/>`__ +and contains git revisions for both gecko-dev and the git cinnabar +repository. diff --git a/docs/code-quality/coding-style/index.rst b/docs/code-quality/coding-style/index.rst new file mode 100644 index 0000000000..e62ce910ca --- /dev/null +++ b/docs/code-quality/coding-style/index.rst @@ -0,0 +1,20 @@ +Coding style +============ + +Firefox code is using different programming languages. +For each language, we are enforcing a specific coding style. + +Getting Help +------------ + +If you need help or have questions, please don’t hesitate to contact us via Matrix +in the "Lint and Formatting" room +(`#lint:mozilla.org <https://chat.mozilla.org/#/room/#lint:mozilla.org>`_). + + +.. toctree:: + :caption: Coding Style User Guide + :maxdepth: 2 + :glob: + + * diff --git a/docs/code-quality/coding-style/rtl_guidelines.rst b/docs/code-quality/coding-style/rtl_guidelines.rst new file mode 100644 index 0000000000..8b2a6bcf04 --- /dev/null +++ b/docs/code-quality/coding-style/rtl_guidelines.rst @@ -0,0 +1,356 @@ +RTL Guidelines +============== + +RTL languages such as Arabic, Hebrew, Persian and Urdu are read and +written from right-to-left, and the user interface for these languages +should be mirrored to ensure the content is easy to understand. + +When a UI is changed from LTR to RTL (or vice-versa), it’s often called +mirroring. An RTL layout is the mirror image of an LTR layout, and it +affects layout, text, and graphics. + +In RTL, anything that relates to time should be depicted as moving from +right to left. For example, forward points to the left, and backwards +points to the right. + +Mirroring layout +~~~~~~~~~~~~~~~~ + +When a UI is mirrored, these changes occur: + +- Text fields icons are displayed on the opposite side of a field +- Navigation buttons are displayed in reverse order +- Icons that communicate direction, like arrows, are mirrored +- Text is usually aligned to the right + +In CSS, while it's possible to apply a rule for LTR and a separate one +specifically for RTL, it's usually better to use CSS `Logical Properties <https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Logical_Properties>`_ +which provide the ability to control layout through logical, rather than +physical mappings. + ++---------------------------------------------------------+--------------------------------------------------+ +| Do | Don't do | ++---------------------------------------------------------+--------------------------------------------------+ +| ``margin-inline-start: 5px`` | ``margin-left: 5px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``padding-inline-end: 5px`` | ``padding-right: 5px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``float: inline-start`` | ``float: left`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``inset-inline-start: 5px`` | ``left: 5px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``border-inline-end: 1px`` | ``border-right: 1px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``border-{start/end}-{start/end}-radius: 2px`` | ``border-{top/bottom}-{left/right}-radius: 2px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``padding: 1px 2px`` | ``padding: 1px 2px 1px 2px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``margin-block: 1px 3px`` && ``margin-inline: 4px 2px`` | ``margin: 1px 2px 3px 4px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``text-align: start`` or ``text-align: match-parent`` | ``text-align: left`` | +| (depends on the context) | | ++---------------------------------------------------------+--------------------------------------------------+ + +When there is no special RTL-aware property available, or when +left/right properties must be used specifically for RTL, use the pseudo +``:-moz-locale-dir(rtl)`` (for XUL documents) or ``:dir(rtl)`` (for HTML +documents). + +For example, this rule covers LTR to display searchicon.svg 7 pixels +from the left: + +.. code:: css + + .search-box { + background-image: url(chrome://path/to/searchicon.svg); + background-position: 7px center; + } + +but an additional rule is necessary to cover RTL and place the search +icon on the right: + +.. code:: css + + .search-box:dir(rtl) { + background-position-x: right 7px; + } + +.. warning:: + + It may be inappropriate to use logical properties when embedding LTR + within RTL contexts. This is described further in the document. + +Mirroring elements +~~~~~~~~~~~~~~~~~~ + +RTL content also affects the direction in which some icons and images +are displayed, particularly those depicting a sequence of events. + +What to mirror +^^^^^^^^^^^^^^ + +- Icons or animations that imply directionality or motion like + back/forward buttons or progress bars +- Icons that imply text direction, like + `reader-mode.svg <https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/browser/themes/shared/icons/reader-mode.svg>`__ +- Icons that imply location of UI elements in the screen, like + `sidebars-right.svg <https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/browser/themes/shared/icons/sidebars-right.svg>`__, + `open-in-new.svg <https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/toolkit/themes/shared/icons/open-in-new.svg>`__, + `default theme's preview.svg <https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/toolkit/mozapps/extensions/default-theme/preview.svg>`__ + or + `pane-collapse.svg <https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/devtools/client/debugger/images/pane-collapse.svg>`__ +- Icons representing objects that are meant to be handheld should look + like they're being right-handed, like the `magnifying glass + icon <https://searchfox.org/mozilla-central/rev/e7c61f4a68b974d5fecd216dc7407b631a24eb8f/toolkit/themes/windows/global/icons/search-textbox.svg>`__ +- Twisties in their collapsed state. Note that if the context in which + they appear is LTR (e.g. code in a devtools HTML view), they should + not be mirrored, even if the user might be using an RTL locale. + +What NOT to mirror +^^^^^^^^^^^^^^^^^^ + +- Text/numbers +- Icons containing text/numbers +- Icons/animations that are direction neutral +- Icons that wouldn't look differently if they'd be mirrored, like `X + buttons <https://searchfox.org/mozilla-central/rev/a78233c11a6baf2c308fbed17eb16c6e57b6a2ac/devtools/client/debugger/images/close.svg>`__ + or the `bookmark + star <https://searchfox.org/mozilla-central/rev/a78233c11a6baf2c308fbed17eb16c6e57b6a2ac/browser/themes/shared/icons/bookmark-hollow.svg>`__ + icon, or any other symmetric icon +- Icons that should look the same as LTR, like icons related to code + (which is always LTR) like + `tool-webconsole.svg <https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/devtools/client/themes/images/tool-webconsole.svg>`__ +- Checkmark icons +- Video/audio player controls +- Product logos +- Order of size dimensions (e.g., ``1920x1080`` should not become + ``1080x1920``) +- Order of size units (e.g., ``10 px`` should not become ``px 10`` + (unless the size unit is localizable)) + +How +^^^ + +The most common way to mirror images is by flipping the X axis: + +.. code:: css + + transform: scaleX(-1); + +Or, if you're already using ``transform`` with a different value on the same +element, you can also use `scale`: + +.. code:: css + + scale: -1 1; + +Note that mirroring images that way doesn't work when the image is a part of +an element with text using ``background-image``, because then the text would +be mirrored along with the image, and the image would be positioned incorrectly. +For such cases, try to use a different method for displaying the image, +like having it as an element all on its own. +If that's not possible, add a separate pre-mirrored image asset and specify +it in a separate ``:dir(rtl)`` rule: + +.. code:: css + + .element-with-icon { + background-image: url("path/to/image/image.svg"); + } + + .element-with-icon:dir(rtl) { + background-image: url("path/to/image/image-rtl.svg"); + } + +For animations like a progress bar, when using ``@keyframes`` to change +the ``transform: translateX()`` states, make sure to add a different +``@keyframes`` suited for RTL, and target that in a separate ``:dir()`` rule: + +.. code:: css + + #progressbar { + animation: progressbar-animation 1s linear infinite; + } + + #progressbar:dir(rtl) { + animation-name: progressbar-animation-rtl; + } + + @keyframes progressbar-animation { + 0% { + transform: translateX(-100px); + } + 100% { + transform: translateX(0); + } + } + + @keyframes progressbar-animation-rtl { + 0% { + transform: translateX(100px); + } + 100% { + transform: translateX(0); + } + } + +Likewise, if you're using ``transform-origin``, make sure to specify the +correct origin for RTL: + +.. code:: css + + #progressbar { + transform-origin: 0 0; + } + + #progressbar:dir(rtl) { + transform-origin: 100% 0; + } + +LTR text inside RTL contexts +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +By default, in RTL locales, some symbols like ``/`` and ``.`` will be moved +around and won't be displayed in the order that they were typed in. This +may be problematic for URLs for instance, where you don't want dots to +change position. + +Here's a non-exhaustive list of elements that should be displayed like +they would be in LTR locales: + +- Paths (e.g., C:\\Users\\username\\Desktop) +- Full URLs +- Code and code containers (like the DevTools' Inspector or the CSS + rules panel) +- about:config preference names and values +- Telephone numbers +- Usernames & passwords (most sites on the web expect LTR + usernames/passwords, but there may be exceptions) +- Other text fields where only LTR text is expected + +To make sure these are displayed correctly, you can use one of the +following on the relevant element: + +- ``direction: ltr`` +- ``dir="ltr"`` in HTML + +Since the direction of such elements is forced to LTR, the text will +also be aligned to the left, which is undesirable from an UI +perspective, given that is inconsistent with the rest of the RTL UI +which has text usually aligned to the right. You can fix this using +``text-align: match-parent``. In the following screenshot, both text +fields (username and password) and the URL have their direction set to +LTR (to display text correctly), but the text itself is aligned to the +right for consistency with the rest of the UI: + +.. image:: about-logins-rtl.png + :alt: about:logins textboxes in RTL layout + +However, since the direction in LTR, this also means that the start/end +properties will correspond to left/right respectively, which is probably +not what you expect. This means you have to use extra rules instead of +using logical properties. + +Here's a full code example: + +.. code:: css + + .url { + direction: ltr; /* Force text direction to be LTR */ + + /* `start` (the default value) will correspond to `left`, + * so we match the parent's direction in order to align the text to the right */ + text-align: match-parent; + } + + /* :dir(ltr/rtl) isn't meaningful on .url, since it has direction: ltr, hence + * why it is matched on .container. */ + .container:dir(ltr) .url { + padding-left: 1em; + } + + .container:dir(rtl) .url { + padding-right: 1em; + } + +.. note:: + + The LTR rule is separate from the global rule to avoid having the + left padding apply on RTL without having to reset it in the RTL rule. + +Auto-directionality +^^^^^^^^^^^^^^^^^^^ + +Sometimes, the text direction on an element should vary dynamically +depending on the situation. This can be the case for a search input for +instance, a user may input a query in an LTR language, but may also +input a query in a RTL language. In this case, the search input has to +dynamically pick the correct directionality based on the first word, in +order to display the query text correctly. The typical way to do this is +to use ``dir="auto"`` in HTML. It is essential that +``text-align: match-parent`` is set, to avoid having the text alignment +change based on the query, and logical properties also cannot be used on +the element itself given they can change meaning depending on the query. + +Testing +~~~~~~~ + +To test for RTL layouts in Firefox, you can go to about:config and +set ``intl.l10n.pseudo`` to ``bidi``, or select the ``Enable "bidi" locale`` +option in the 3-dots menu in the :doc:`Browser Toolbox </devtools-user/browser_toolbox/index>`. +The Firefox UI should immediately flip, but a restart may be required +to take effect in some Firefox features and interactions. + +.. note:: + + When testing with ``intl.l10n.pseudo`` set to ``bidi``, you may see some + oddities regarding text ordering due to the nature of displaying LTR + text in RTL layout. + + .. image:: about-protections-rtl.png + :alt: about:protections in RTL layout- English vs. Hebrew + + This shouldn't be an issue when using an actual RTL build or language pack. + +How to spot RTL-related issues +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- Punctuation marks should appear on the left side of a + word/sentence/paragraph on RTL, so if a *localizable* string appears + in the UI with a dot, colon, ellipsis, question or exclamation mark + on the right side of the text, this probably means that the text + field is forced to be displayed as LTR. +- If icons/images/checkmarks do not appear on the opposite side of + text, when compared to LTR. +- If buttons (like the close button, "OK" and "Cancel" etc.) do not + appear on the opposite side of the UI and not in the opposite order, + when compared to LTR. +- If paddings/margins/borders are not the same from the opposite side, + when compared to LTR. +- Although Hebrew uses ``1 2 3``, all the other RTL locales we support + should use ``١ ٢ ٣`` as digits. So if you see ``1 2 3`` on any such + locale, that likely indicates a bug. +- If navigating in the UI using the left/right arrow keys does not + select the correct element (i.e., pressing Left selects an item on + the right). +- If navigating in the UI using the Tab key does not focus elements + from right to left, in an RTL context. +- If code is displayed as RTL (e.g., ``;padding: 20px`` - the semicolon + should appear on the right side of the code). Code can still be + aligned to the right if it appears in an RTL context. + +See also +~~~~~~~~ + +- `RTL Best + Practices <https://docs.google.com/document/d/1Rc8rvwsLI06xArFQouTinSh3wNte9Sqn9KWi1r7xY4Y/edit#heading=h.pw54h41h12ct>`__ +- Building RTL-Aware Web Apps & Websites: `Part + 1 <https://hacks.mozilla.org/2015/09/building-rtl-aware-web-apps-and-websites-part-1/>`__, + `Part + 2 <https://hacks.mozilla.org/2015/10/building-rtl-aware-web-apps-websites-part-2/>`__ + +Credits +~~~~~~~ + +Google's `Material Design guide for +RTL <https://material.io/design/usability/bidirectionality.html>`__ diff --git a/docs/code-quality/coding-style/svg_guidelines.rst b/docs/code-quality/coding-style/svg_guidelines.rst new file mode 100644 index 0000000000..f34317c91a --- /dev/null +++ b/docs/code-quality/coding-style/svg_guidelines.rst @@ -0,0 +1,347 @@ +SVG Guidelines +============== + +Pros and cons of SVG for images +------------------------------- + +When used as a document format there is usually a compelling reason that +makes SVG the only solution. When used as an `image +format <https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_as_an_Image>`__, +it is sometimes less obvious whether it would be best to use SVG or a +raster image format for any given image. The vector format SVG and +raster formats like PNG both have their place. When choosing whether or +not to use SVG it is best to understand the advantages and disadvantages +of both. + +File size + Whether SVG or a raster format will produce a smaller file for a + given image depends very much on the image. For example, consider an + image of a path with a gradient fill. The size of an SVG of this + image will be the same regardless of the dimensions of the image. On + the other hand the size of a raster file of the same image will + likely vary tremendously depending on the dimensions of the image + since the larger the dimensions the more pixel data the file needs to + store. At very small dimensions (the extreme case being 1px x 1px) + the raster file will likely be much smaller than the SVG file since + it only needs to store one pixel of data. At large dimensions the + raster file may be much larger than the SVG file. +Scalability, with caveats + One of the primary advantages of SVG is that as it is scaled it does + not pixelate. However, this is not to say that it always does away + with the need to have a collection of raster images for display at + different scales. This can be particularly true for icons. While SVG + may scale well enough for flat-ish icons without a lot of detail, for + icons that try to pack in a lot of detail graphic artists generally + `want to be able to pixel + tweak <https://www.pushing-pixels.org/2011/11/04/about-those-vector-icons.html>`__. +Performance + While SVG provides a lot of flexibility in terms of scaling, + themability, etc. this flexibility depends on doing computations for + SVG images at the time they're displayed, rather than at the time the + author creates them. Consider an image that involves some complex + gradients and filters. If saved as a raster image then the work to + rasterize the gradients and filters takes place on the authors + computer before the result is stored in the raster file. This work + doesn't need to be redone when the image is displayed on someone + else's computer. On the other hand, if the image is saved as an SVG + image then all this work needs to be done each time the SVG is + displayed on someone else's computer. This isn't to say that SVG + images are always slower than raster equivalents. In fact it can be + faster to send vector information from an SVG to a user's GPU than it + is to extract raster data from an equivalent raster image. And even + when an SVG image is slower than a raster equivalent, the difference + is usually not noticeable. However, just don't fall into the trap of + thinking that SVGs are faster than equivalent raster images, or vice + versa. Once again, "it depends". + +Authoring guidelines +-------------------- + +A lot of SVG files (particularly those generated by SVG editors) ship +without being cleaned up and can contain a ton of junk that bloats the +file size and slows down rendering. In general the best way to combat +this is to first run SVG files through a linter such as +`svgo <https://github.com/svg/svgo>`__ (see the Tools section below). +However, when authoring SVGs by hand here are some best practices to +help keep them lightweight. These rules are based on some real examples +seen in Mozilla's code. + +Basics +~~~~~~ + +- Two spaces indenting +- No useless whitespaces or line breaks (see below for more details) +- Adding a license header +- Use double quotes + +Whitespace and line breaks +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Whitespace +^^^^^^^^^^ + +In addition to trailing whitespace at the end of lines, there are a few +more cases more specific to SVGs: + +- Trailing whitespaces in attribute values (usually seen in path + definitions) +- Excessive whitespace in path or polygon points definition + +Whitespace examples +^^^^^^^^^^^^^^^^^^^ + +This path: + +.. code:: html + + <path d=" M5,5 L1,1z "> + +can be cut down to this: + +.. code:: html + + <path d="M5,5 L1,1z"> + +Similarly, this polygon: + +.. code:: html + + <polygon points=" 0,0 4,4 4,0 "/> + +can be cut down to this: + +.. code:: html + + <polygon points="0,0 4,4 4,0"/> + +Line breaks +^^^^^^^^^^^ + +You should only use line breaks for logical separation or if they help +make the file readable. You should avoid line breaks between every +single element or within attribute values. It's recommended to put the +attributes on the same line as their tag names, if possible. You should +also put the shortest attributes first, so they are easier to spot. + +Unused tags and attributes +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Editor metadata +^^^^^^^^^^^^^^^ + +Vector editors (Inkscape, Adobe Illustrator, Sketch) usually add a bunch +of metadata in SVG files while saving them. Metadata can mean many +things, including: + +- The typical "Created with *editor*" comments +- Non-standard editor specific tags and attributes (``sketch:foo``, + ``illustrator:foo``, ``sopodi:foo``, …) +- The `XML + namespace <https://developer.mozilla.org/en-US/docs/Web/SVG/Namespaces_Crash_Course>`__ + definition that comes with the latter (``xmlns:sketch``, + ``xmlns:sopodi``, …) + +Other metadata +^^^^^^^^^^^^^^ + +In addition to non-standard editor metadata, standard compliant metadata +also exists. Typical examples of this are ``<title>`` and ``<desc>`` +tags. Although this kind of data is supported by the browser, it can +only be displayed when the SVG is opened in a new tab. Plus, in most of +the cases, the filename is quite descriptive So it's recommended to +remove that kind of metadata since it doesn't bring much value. + +You shouldn't include DOCTYPEs in your SVGs either; they are a source of +many issues, and the SVG WG recommends not to include them. See `SVG +Authoring +guidelines <https://jwatt.org/svg/authoring/#doctype-declaration>`__. + +Avoid the use of CDATA sections +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +`CDATA +sections <https://developer.mozilla.org/en-US/docs/Web/API/CDATASection>`__ +are used to avoid parsing some text as HTML. Most of time, CDATA isn't +needed, for example, the content in ``<style>`` tags doesn't need to be +wrapped in a CDATA section as the content inside the tag is already +correctly parsed as CSS. + +Invisible shapes +^^^^^^^^^^^^^^^^ + +There are two kinds of invisible shapes: The off-screen ones and the +uncolored ones. + +The offscreen shapes are hard to spot, even with an automated tool, and +are usually context aware. Those kinds of shapes are visible but off the +`SVG view +box <https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/viewBox>`__. +Here's `an +example <https://hg.mozilla.org/mozilla-central/diff/9fb143f3b36a/browser/themes/shared/heartbeat-star-lit.svg>`__ +of a file with offscreen shapes. + +On the other hand, the uncolored ones are easier to spot, since they +usually come with styles making them invisible. They must meet two +conditions: they must be devoid of any fill (or a transparent one) or +stroke. + +Unused attributes on root ``<svg>`` element +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The root ``<svg>`` element can also host many useless attributes. Here's +an +`example <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/components/loop/content/shared/img/icons-10x10.svg>`__ +taking into account the list below: + +- ``version`` +- ``x="0"`` and ``y="0"`` +- ``enable-background`` (unsupported by Gecko and now deprecated by the + Filter Effects specification) +- ``id`` (id on root element has no effect) +- ``xmlns:xlink`` attribute when there are no ``xlink:href`` attributes + used throughout the file +- Other unused `XML + Namespace <https://developer.mozilla.org/en-US/docs/Web/SVG/Namespaces_Crash_Course>`__ + definitions +- ``xml:space`` when there is no text used in the file + +Other +^^^^^ + +- Empty tags, this may be obvious, but those are sometimes found in + SVGs +- Unreferenced ids (usually on gradient stops, but also on shapes or + paths) +- ``clip-rule`` attribute when the element *is not* a descendant of a + ``<clipPath>`` +- ``fill-rule`` attribute when the element *is* a descendant of a + ``<clipPath>`` +- Unreferenced/Unused clip paths, masks or defs + (`example <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/toolkit/themes/shared/reader/RM-Plus-24x24.svg>`__) + +Styling +~~~~~~~ + +Styling basics +^^^^^^^^^^^^^^ + +- Privilege short lowercase hex for colors +- Don't use excessive precision for numeric values (usually comes from + illustrator) +- Use descriptive IDs +- Avoid inline styles and use class names or SVG attributes + +Styling examples +'''''''''''''''' + +Here are some examples for excessive number precision: + +- 5.000000e-02 → 0.05 (as seen + `here <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/themes/shared/devtools/images/tool-network.svg#l1.31>`__) +- -3.728928e-10 → 0 (as seen + `here <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/themes/shared/aboutNetError_alert.svg#l1.12>`__) +- translate(0.000000, -1.000000) → translate(0, -1) (as seen + `here <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/themes/shared/heartbeat-icon.svg#l1.13>`__) + +As for descriptive IDs: + +- For gradients: SVG_ID1 → gradient1 (as seen + `here <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/themes/shared/aboutNetError_alert.svg#l1.12>`__) + +Use of class names +^^^^^^^^^^^^^^^^^^ + +- Avoid using a class if that class is only used once in the file +- If that class only sets a fill or a stroke, it's better to set the + fill/stroke directly on the actual shape, instead of introducing a + class just for that shape. You can also use SVG grouping to avoid + duplicating those attributes +- Avoid introducing variants of the same file (color/style variants), + and use sprites instead (with class names) + +Default style values +^^^^^^^^^^^^^^^^^^^^ + +There's usually no need to set the default style value unless you're +overriding a style. Here are some commonly seen examples: + +- ``style="display: none;"`` on ``<defs>`` elements (a ``<defs>`` + element is hidden by default) +- ``type="text/css"`` on ``<style>`` elements +- ``stroke: none`` or ``stroke-width: 0`` + +SVG grouping +~~~~~~~~~~~~ + +Style grouping +^^^^^^^^^^^^^^ + +Group similarly styled shapes under one ``<g>`` tag; this avoids having +to set the same class/styles on many shapes. + +Avoid excessive grouping +^^^^^^^^^^^^^^^^^^^^^^^^ + +Editors can sometimes do excessive grouping while exporting SVGs. This +is due to the way editors work. + +Nested groups +''''''''''''' + +Avoid multiple-level nesting of groups, these make the SVG less +readable. + +Nested transforms +''''''''''''''''' + +Some editors use ``<g>`` tags to do nested transforms, which is usually +not needed. You can avoid this by doing basic algebra, for example: + +.. code:: xml + + <g transform="translate(-62, -310)"><shape transform="translate(60, 308)"/></g> + +can be cut down to: + +.. code:: xml + + <shape transform="translate(-2,-2)"/> + +because: -62+60 = -310+308 = -2 + +Performance tips +~~~~~~~~~~~~~~~~ + +These rules are optional, but they help speeding up the SVG. + +- Avoid using a ``<use>`` tag when that ``<use>`` tag is being + referenced only once in the whole file. +- Instead of using CSS/SVG + `transforms <https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/transform>`__, + apply directly the transform on the path/shape definition. + +Tools +~~~~~ + +Tools can help to clean SVG files. Note, however that some of the rules +stated above can be hard to detect with automated tools since they +require too much context-awareness. To this date, there doesn't seem to +be a tool that handles all of the above. However, there are some +utilities that cover parts of this document: + +- Mostly complete command line tool: https://github.com/svg/svgo +- Alternatives to SVGO: + + - https://github.com/RazrFalcon/svgcleaner + - https://github.com/scour-project/scour + +- GUI for command line tool (use with "Prettify code" and "Remove + ``<title>``" options on): https://jakearchibald.github.io/svgomg/ +- Good alternative to SVGO/SVGOMG: + https://petercollingridge.appspot.com/svg-editor +- Fixes the excessive number precision: + https://simon.html5.org/tools/js/svg-optimizer/ +- Converts inline styles to SVG + attributes: https://www.w3.org/wiki/SvgTidy +- RaphaelJS has a couple of utilities that may be useful: + `raphael.js <https://dmitrybaranovskiy.github.io/raphael/>`__ diff --git a/docs/code-quality/coding-style/using_cxx_in_firefox_code.rst b/docs/code-quality/coding-style/using_cxx_in_firefox_code.rst new file mode 100644 index 0000000000..92decbdf1e --- /dev/null +++ b/docs/code-quality/coding-style/using_cxx_in_firefox_code.rst @@ -0,0 +1,1075 @@ +Using C++ in Mozilla code +========================= + +C++ language features +--------------------- + +Mozilla code only uses a subset of C++. Runtime type information (RTTI) +is disabled, as it tends to cause a very large increase in codesize. +This means that ``dynamic_cast``, ``typeid()`` and ``<typeinfo>`` cannot +be used in Mozilla code. Also disabled are exceptions; do not use +``try``/``catch`` or throw any exceptions. Libraries that throw +exceptions may be used if you are willing to have the throw instead be +treated as an abort. + +On the side of extending C++, we compile with ``-fno-strict-aliasing``. +This means that when reinterpreting a pointer as a differently-typed +pointer, you don't need to adhere to the "effective type" (of the +pointee) rule from the standard (aka. "the strict aliasing rule") when +dereferencing the reinterpreted pointer. You still need make sure that +you don't violate alignment requirements and need to make sure that the +data at the memory location pointed to forms a valid value when +interpreted according to the type of the pointer when dereferencing the +pointer for reading. Likewise, if you write by dereferencing the +reinterpreted pointer and the originally-typed pointer might still be +dereferenced for reading, you need to make sure that the values you +write are valid according to the original type. This value validity +issue is moot for e.g. primitive integers for which all bit patterns of +their size are valid values. + +- As of Mozilla 59, C++14 mode is required to build Mozilla. +- As of Mozilla 67, MSVC can no longer be used to build Mozilla. +- As of Mozilla 73, C++17 mode is required to build Mozilla. + +This means that C++17 can be used where supported on all platforms. The +list of acceptable features is given below: + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 3 + + * - + - GCC + - Clang + - + * - Current minimal requirement + - 8.1 + - 7.0 + - + * - Feature + - GCC + - Clang + - Can be used in code + * - ``type_t &&`` + - 4.3 + - 2.9 + - Yes (see notes) + * - ref qualifiers on methods + - 4.8.1 + - 2.9 + - Yes + * - default member-initializers (except for bit-fields) + - 4.7 + - 3.0 + - Yes + * - default member-initializers (for bit-fields) + - 8 + - 6 + - **No** + * - variadic templates + - 4.3 + - 2.9 + - Yes + * - Initializer lists + - 4.4 + - 3.1 + - Yes + * - ``static_assert`` + - 4.3 + - 2.9 + - Yes + * - ``auto`` + - 4.4 + - 2.9 + - Yes + * - lambdas + - 4.5 + - 3.1 + - Yes + * - ``decltype`` + - 4.3 + - 2.9 + - Yes + * - ``Foo<Bar<T>>`` + - 4.3 + - 2.9 + - Yes + * - ``auto func() -> int`` + - 4.4 + - 3.1 + - Yes + * - Templated aliasing + - 4.7 + - 3.0 + - Yes + * - ``nullptr`` + - 4.6 + - 3.0 + - Yes + * - ``enum foo : int16_t`` {}; + - 4.4 + - 2.9 + - Yes + * - ``enum class foo {}``; + - 4.4 + - 2.9 + - Yes + * - ``enum foo;`` + - 4.6 + - 3.1 + - Yes + * - ``[[attributes]]`` + - 4.8 + - 3.3 + - **No** (see notes) + * - ``constexpr`` + - 4.6 + - 3.1 + - Yes + * - ``alignas`` + - 4.8 + - 3.3 + - Yes + * - ``alignof`` + - 4.8 + - 3.3 + - Yes, but see notes ; only clang 3.6 claims as_feature(cxx_alignof) + * - Delegated constructors + - 4.7 + - 3.0 + - Yes + * - Inherited constructors + - 4.8 + - 3.3 + - Yes + * - ``explicit operator bool()`` + - 4.5 + - 3.0 + - Yes + * - ``char16_t/u"string"`` + - 4.4 + - 3.0 + - Yes + * - ``R"(string)"`` + - 4.5 + - 3.0 + - Yes + * - ``operator""()`` + - 4.7 + - 3.1 + - Yes + * - ``=delete`` + - 4.4 + - 2.9 + - Yes + * - ``=default`` + - 4.4 + - 3.0 + - Yes + * - unrestricted unions + - 4.6 + - 3.1 + - Yes + * - ``for (auto x : vec)`` (`be careful about the type of the iterator <https://stackoverflow.com/questions/15176104/c11-range-based-loop-get-item-by-value-or-reference-to-const>`__) + - 4.6 + - 3.0 + - Yes + * - ``override``/``final`` + - 4.7 + - 3.0 + - Yes + * - ``thread_local`` + - 4.8 + - 3.3 + - **No** (see notes) + * - function template default arguments + - 4.3 + - 2.9 + - Yes + * - local structs as template parameters + - 4.5 + - 2.9 + - Yes + * - extended friend declarations + - 4.7 + - 2.9 + - Yes + * - ``0b100`` (C++14) + - 4.9 + - 2.9 + - Yes + * - `Tweaks to some C++ contextual conversions` (C++14) + - 4.9 + - 3.4 + - Yes + * - Return type deduction (C++14) + - 4.9 + - 3.4 + - Yes (but only in template code when you would have used ``decltype (complex-expression)``) + * - Generic lambdas (C++14) + - 4.9 + - 3.4 + - Yes + * - Initialized lambda captures (C++14) + - 4.9 + - 3.4 + - Yes + * - Digit separator (C++14) + - 4.9 + - 3.4 + - Yes + * - Variable templates (C++14) + - 5.0 + - 3.4 + - Yes + * - Relaxed constexpr (C++14) + - 5.0 + - 3.4 + - Yes + * - Aggregate member initialization (C++14) + - 5.0 + - 3.3 + - Yes + * - Clarifying memory allocation (C++14) + - 5.0 + - 3.4 + - Yes + * - [[deprecated]] attribute (C++14) + - 4.9 + - 3.4 + - **No** (see notes) + * - Sized deallocation (C++14) + - 5.0 + - 3.4 + - **No** (see notes) + * - Concepts (Concepts TS) + - 6.0 + - — + - **No** + * - Inline variables (C++17) + - 7.0 + - 3.9 + - Yes + * - constexpr_if (C++17) + - 7.0 + - 3.9 + - Yes + * - constexpr lambdas (C++17) + - — + - — + - **No** + * - Structured bindings (C++17) + - 7.0 + - 4.0 + - Yes + * - Separated declaration and condition in ``if``, ``switch`` (C++17) + - 7.0 + - 3.9 + - Yes + * - `Fold expressions <https://en.cppreference.com/w/cpp/language/fold>`__ (C++17) + - 6.0 + - 3.9 + - Yes + * - [[fallthrough]], [[maybe_unused]], [[nodiscard]] (C++17) + - 7.0 + - 3.9 + - Yes + * - Aligned allocation/deallocation (C++17) + - 7.0 + - 4.0 + - **No** (see notes) + * - Designated initializers (C++20) + - 8.0 (4.7) + - 10.0 (3.0) + - Yes [*sic*] (see notes) + * - #pragma once + - 3.4 + - Yes + - **Not** until we `normalize headers <https://groups.google.com/d/msg/mozilla.dev.platform/PgDjWw3xp8k/eqCFlP4Kz1MJ>`__ + * - `Source code information capture <https://en.cppreference.com/w/cpp/experimental/lib_extensions_2#Source_code_information_capture>`__ + - 8.0 + - — + - **No** + +Sources +~~~~~~~ + +* GCC: https://gcc.gnu.org/projects/cxx-status.html +* Clang: https://clang.llvm.org/cxx_status.html + +Notes +~~~~~ + +rvalue references + Implicit move method generation cannot be used. + +Attributes + Several common attributes are defined in + `mozilla/Attributes.h <https://searchfox.org/mozilla-central/source/mfbt/Attributes.h>`__ + or nscore.h. + +Alignment + Some alignment utilities are defined in `mozilla/Alignment.h + <https://searchfox.org/mozilla-central/source/mfbt/Alignment.h>`__. + + .. caution:: + ``MOZ_ALIGNOF`` and ``alignof`` don't have the same semantics. Be careful of what you + expect from them. + +``[[deprecated]]`` + If we have deprecated code, we should be removing it rather than marking it as + such. Marking things as ``[[deprecated]]`` also means the compiler will warn + if you use the deprecated API, which turns into a fatal error in our + automation builds, which is not helpful. + +Sized deallocation + Our compilers all support this (custom flags are required for GCC and Clang), + but turning it on breaks some classes' ``operator new`` methods, and `some + work <https://bugzilla.mozilla.org/show_bug.cgi?id=1250998>`__ would need to + be done to make it an efficiency win with our custom memory allocator. + +Aligned allocation/deallocation + Our custom memory allocator doesn't have support for these functions. + +Thread locals + ``thread_local`` is not supported on Android. + +Designated initializers + Despite their late addition to C++ (and lack of *official* support by + compilers until relatively recently), `C++20's designated initializers + <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0329r4.pdf>`__ are + merely a subset of `a feature originally introduced in C99 + <https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html>`__ -- and this + subset has been accepted without comment in C++ code since at least GCC 4.7 + and Clang 3.0. + + +C++ and Mozilla standard libraries +---------------------------------- + +The Mozilla codebase contains within it several subprojects which follow +different rules for which libraries can and can't be used it. The rules +listed here apply to normal platform code, and assume unrestricted +usability of MFBT or XPCOM APIs. + +.. warning:: + + The rest of this section is a draft for expository and exploratory + purposes. Do not trust the information listed here. + +What follows is a list of standard library components provided by +Mozilla or the C++ standard. If an API is not listed here, then it is +not permissible to use it in Mozilla code. Deprecated APIs are not +listed here. In general, prefer Mozilla variants of data structures to +standard C++ ones, even when permitted to use the latter, since Mozilla +variants tend to have features not found in the standard library (e.g., +memory size tracking) or have more controllable performance +characteristics. + +A list of approved standard library headers is maintained in +`config/stl-headers.mozbuild <https://searchfox.org/mozilla-central/source/config/stl-headers.mozbuild>`__. + + +Data structures +~~~~~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL equivalent + - Notes + * - ``nsAutoTArray`` + - ``nsTArray.h`` + - + - Like ``nsTArray``, but will store a small amount as stack storage + * - ``nsAutoTObserverArray`` + - ``nsTObserverArray.h`` + - + - Like ``nsTObserverArray``, but will store a small amount as stack storage + * - ``mozilla::BloomFilter`` + - ``mozilla/BloomFilter.h`` + - + - Probabilistic set membership (see `Wikipedia <https://en.wikipedia.org/wiki/Bloom_filter#Counting_filters>`__) + * - ``nsClassHashtable`` + - ``nsClassHashtable.h`` + - + - Adaptation of nsTHashtable, see :ref:`XPCOM Hashtable Guide` + * - ``nsCOMArray`` + - ``nsCOMArray.h`` + - + - Like ``nsTArray<nsCOMPtr<T>>`` + * - ``nsDataHashtable`` + - ``nsClassHashtable.h`` + - ``std::unordered_map`` + - Adaptation of ``nsTHashtable``, see :ref:`XPCOM Hashtable Guide` + * - ``nsDeque`` + - ``nsDeque.h`` + - ``std::deque<void *>`` + - + * - ``mozilla::EnumSet`` + - ``mozilla/EnumSet.h`` + - + - Like ``std::set``, but for enum classes. + * - ``mozilla::Hash{Map,Set}`` + - `mozilla/HashTable.h <https://searchfox.org/mozilla-central/source/mfbt/HashTable.h>`__ + - ``std::unordered_{map,set}`` + - A general purpose hash map and hash set. + * - ``nsInterfaceHashtable`` + - ``nsInterfaceHashtable.h`` + - ``std::unordered_map`` + - Adaptation of ``nsTHashtable``, see :ref:`XPCOM Hashtable Guide` + * - ``mozilla::LinkedList`` + - ``mozilla/LinkedList.h`` + - ``std::list`` + - Doubly-linked list + * - ``nsRef PtrHashtable`` + - ``nsRefPtrHashtable.h`` + - ``std::unordered_map`` + - Adaptation of ``nsTHashtable``, see :ref:`XPCOM Hashtable Guide` + * - ``mozilla::SegmentedVector`` + - ``mozilla/SegmentedVector.h`` + - ``std::deque`` w/o O(1) pop_front + - Doubly-linked list of vector elements + * - ``mozilla::SplayTree`` + - ``mozilla/SplayTree.h`` + - + - Quick access to recently-accessed elements (see `Wikipedia <https://en.wikipedia.org/wiki/Splay_tree>`__) + * - ``nsTArray`` + - ``nsTArray.h`` + - ``std::vector`` + - + * - ``nsTHashtable`` + - ``nsTHashtable.h`` + - ``std::unordered_{map,set}`` + - See :ref:`XPCOM Hashtable Guide`, you probably want a subclass + * - ``nsTObserverArray`` + - ``nsTObserverArray.h`` + - + - Like ``nsTArray``, but iteration is stable even through mutation + * - ``nsTPriorityQueue`` + - ``nsTPriorityQueue.h`` + - ``std::priority_queue`` + - Unlike the STL class, not a container adapter + * - ``mozilla::Vector`` + - ``mozilla/Vector.h`` + - ``std::vector`` + - + * - ``mozilla::Buffer`` + - ``mozilla/Buffer.h`` + - + - Unlike ``Array``, has a run-time variable length. Unlike ``Vector``, does not have capacity and growth mechanism. Unlike ``Span``, owns its buffer. + + +Safety utilities +~~~~~~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL equivalent + - Notes + * - ``mozilla::Array`` + - ``mfbt/Array.h`` + - + - safe array index + * - ``mozilla::AssertedCast`` + - ``mfbt/Casting.h`` + - + - casts + * - ``mozilla::CheckedInt`` + - ``mfbt/CheckedInt.h`` + - + - avoids overflow + * - ``nsCOMPtr`` + - ``xpcom/base/nsCOMPtr.h`` + - ``std::shared_ptr`` + - + * - ``mozilla::EnumeratedArray`` + - ``mfbt/EnumeratedArray.h`` + - ``mozilla::Array`` + - + * - ``mozilla::Maybe`` + - ``mfbt/Maybe.h`` + - ``std::optional`` + - + * - ``mozilla::RangedPtr`` + - ``mfbt/RangedPtr.h`` + - + - like ``mozilla::Span`` but with two pointers instead of pointer and length + * - ``mozilla::RefPtr`` + - ``mfbt/RefPtr.h`` + - ``std::shared_ptr`` + - + * - ``mozilla::Span`` + - ``mozilla/Span.h`` + - ``gsl::span``, ``absl::Span``, ``std::string_view``, ``std::u16string_view`` + - Rust's slice concept for C++ (without borrow checking) + * - ``StaticRefPtr`` + - ``xpcom/base/StaticPtr.h`` + - + - ``nsRefPtr`` w/o static constructor + * - ``mozilla::UniquePtr`` + - ``mfbt/UniquePtr.h`` + - ``std::unique_ptr`` + - + * - ``mozilla::WeakPtr`` + - ``mfbt/WeakPtr.h`` + - ``std::weak_ptr`` + - + * - ``nsWeakPtr`` + - ``xpcom/base/nsWeakPtr.h`` + - ``std::weak_ptr`` + - + + +Strings +~~~~~~~ + +See the :doc:`Mozilla internal string guide </xpcom/stringguide>` for +usage of ``nsAString`` (our copy-on-write replacement for +``std::u16string``) and ``nsACString`` (our copy-on-write replacement +for ``std::string``). + +Be sure not to introduce further uses of ``std::wstring``, which is not +portable! (Some uses exist in the IPC code.) + + +Algorithms +~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 + + * - ``mozilla::BinarySearch`` + - ``mfbt/BinarySearch.h`` + * - ``mozilla::BitwiseCast`` + - ``mfbt/Casting.h`` (strict aliasing-safe cast) + * - ``mozilla/MathAlgorithms.h`` + - (rotate, ctlz, popcount, gcd, abs, lcm) + * - ``mozilla::RollingMean`` + - ``mfbt/RollingMean.h`` () + + +Concurrency +~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL/boost equivalent + - Notes + * - ``mozilla::Atomic`` + - mfbt/Atomic.h + - ``std::atomic`` + - + * - ``mozilla::CondVar`` + - xpcom/threads/CondVar.h + - ``std::condition_variable`` + - + * - ``mozilla::DataMutex`` + - xpcom/threads/DataMutex.h + - ``boost::synchronized_value`` + - + * - ``mozilla::Monitor`` + - xpcom/threads/Monitor.h + - + - + * - ``mozilla::Mutex`` + - xpcom/threads/Mutex.h + - ``std::mutex`` + - + * - ``mozilla::ReentrantMonitor`` + - xpcom/threads/ReentrantMonitor.h + - + - + * - ``mozilla::StaticMutex`` + - xpcom/base/StaticMutex.h + - ``std::mutex`` + - Mutex that can (and in fact, must) be used as a global/static variable. + + +Miscellaneous +~~~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL/boost equivalent + - Notes + * - ``mozilla::AlignedStorage`` + - mfbt/Alignment.h + - ``std::aligned_storage`` + - + * - ``mozilla::MaybeOneOf`` + - mfbt/MaybeOneOf.h + - ``std::optional<std::variant<T1, T2>>`` + - ~ ``mozilla::Maybe<union {T1, T2}>`` + * - ``mozilla::Pair`` + - mfbt/Pair.h + - ``std::tuple<T1, T2>`` + - minimal space! + * - ``mozilla::TimeStamp`` + - xpcom/ds/TimeStamp.h + - ``std::chrono::time_point`` + - + * - + - mozilla/PodOperations.h + - + - C++ versions of ``memset``, ``memcpy``, etc. + * - + - mozilla/ArrayUtils.h + - + - + * - + - mozilla/Compression.h + - + - + * - + - mozilla/Endian.h + - + - + * - + - mozilla/FloatingPoint.h + - + - + * - + - mozilla/HashFunctions.h + - ``std::hash`` + - + * - + - mozilla/Move.h + - ``std::move``, ``std::swap``, ``std::forward`` + - + + +Mozilla data structures and standard C++ ranges and iterators +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Some Mozilla-defined data structures provide STL-style +`iterators <https://en.cppreference.com/w/cpp/named_req/Iterator>`__ and +are usable in `range-based for +loops <https://en.cppreference.com/w/cpp/language/range-for>`__ as well +as STL `algorithms <https://en.cppreference.com/w/cpp/algorithm>`__. + +Currently, these include: + +.. list-table:: + :widths: 16 16 16 16 16 + :header-rows: 1 + + * - Name + - Header + - Bug(s) + - Iterator category + - Notes + * - ``nsTArray`` + - ``xpcom/ds/n sTArray.h`` + - `1126552 <https://bugzilla.mozilla.org/show_bug.cgi?id=1126552>`__ + - Random-access + - Also reverse-iterable. Also supports remove-erase pattern via RemoveElementsAt method. Also supports back-inserting output iterators via ``MakeBackInserter`` function. + * - ``nsBaseHashtable`` and subclasses: ``nsClassHashtable`` ``nsDataHashtable`` ``nsInterfaceHashtable`` ``nsJSThingHashtable`` ``nsRefPtrHashtable`` + - ``xpcom/ds/nsBaseHashtable.h`` ``xpcom/ds/nsClassHashtable.h`` ``xpcom/ds/nsDataHashtable.h`` ``xpcom/ds/nsInterfaceHashtable.h`` ``xpcom/ds/nsJSThingHashtable.h`` ``xpcom/ds/nsRefPtrHashtable.h`` + - `1575479 <https://bugzilla.mozilla.org/show_bug.cgi?id=1575479>`__ + - Forward + - + * - ``nsCOMArray`` + - ``xpcom/ds/nsCOMArray.h`` + - `1342303 <https://bugzilla.mozilla.org/show_bug.cgi?id=1342303>`__ + - Random-access + - Also reverse-iterable. + * - ``Array`` ``EnumerationArray`` ``RangedArray`` + - ``mfbt/Array.h`` ``mfbt/EnumerationArray.h`` ``mfbt/RangedArray.h`` + - `1216041 <https://bugzilla.mozilla.org/show_bug.cgi?id=1216041>`__ + - Random-access + - Also reverse-iterable. + * - ``Buffer`` + - ``mfbt/Buffer.h`` + - `1512155 <https://bugzilla.mozilla.org/show_bug.cgi?id=1512155>`__ + - Random-access + - Also reverse-iterable. + * - ``DoublyLinkedList`` + - ``mfbt/DoublyLinkedList.h`` + - `1277725 <https://bugzilla.mozilla.org/show_bug.cgi?id=1277725>`__ + - Forward + - + * - ``EnumeratedRange`` + - ``mfbt/EnumeratedRange.h`` + - `1142999 <https://bugzilla.mozilla.org/show_bug.cgi?id=1142999>`__ + - *Missing* + - Also reverse-iterable. + * - ``IntegerRange`` + - ``mfbt/IntegerRange.h`` + - `1126701 <https://bugzilla.mozilla.org/show_bug.cgi?id=1126701>`__ + - *Missing* + - Also reverse-iterable. + * - ``SmallPointerArray`` + - ``mfbt/SmallPointerArray.h`` + - `1331718 <https://bugzilla.mozilla.org/show_bug.cgi?id=1331718>`__ + - Random-access + - + * - ``Span`` + - ``mfbt/Span.h`` + - `1295611 <https://bugzilla.mozilla.org/show_bug.cgi?id=1295611>`__ + - Random-access + - Also reverse-iterable. + +Note that if the iterator category is stated as "missing", the type is +probably only usable in range-based for. This is most likely just an +omission, which could be easily fixed. + +Useful in this context are also the class template ``IteratorRange`` +(which can be used to construct a range from any pair of iterators) and +function template ``Reversed`` (which can be used to reverse any range), +both defined in ``mfbt/ReverseIterator.h`` + + +Further C++ rules +----------------- + + +Don't use static constructors +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +(You probably shouldn't be using global variables to begin with. Quite +apart from the weighty software-engineering arguments against them, +globals affect startup time! But sometimes we have to do ugly things.) + +Non-portable example: + +.. code-block:: c++ + + FooBarClass static_object(87, 92); + + void + bar() + { + if (static_object.count > 15) { + ... + } + } + +Once upon a time, there were compiler bugs that could result in +constructors not being called for global objects. Those bugs are +probably long gone by now, but even with the feature working correctly, +there are so many problems with correctly ordering C++ constructors that +it's easier to just have an init function: + +.. code-block:: c++ + + static FooBarClass* static_object; + + FooBarClass* + getStaticObject() + { + if (!static_object) + static_object = + new FooBarClass(87, 92); + return static_object; + } + + void + bar() + { + if (getStaticObject()->count > 15) { + ... + } + } + + +Don't use exceptions +~~~~~~~~~~~~~~~~~~~~ + +See the introduction to the "C++ language features" section at the start +of this document. + + +Don't use Run-time Type Information +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See the introduction to the "C++ language features" section at the start +of this document. + +If you need runtime typing, you can achieve a similar result by adding a +``classOf()`` virtual member function to the base class of your +hierarchy and overriding that member function in each subclass. If +``classOf()`` returns a unique value for each class in the hierarchy, +you'll be able to do type comparisons at runtime. + + +Don't use the C++ standard library (including iostream and locale) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See the section "C++ and Mozilla standard libraries". + + +Use C++ lambdas, but with care +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +C++ lambdas are supported across all our compilers now. Rejoice! We +recommend explicitly listing out the variables that you capture in the +lambda, both for documentation purposes, and to double-check that you're +only capturing what you expect to capture. + + +Use namespaces +~~~~~~~~~~~~~~ + +Namespaces may be used according to the style guidelines in :ref:`C++ Coding style`. + + +Don't mix varargs and inlines +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +What? Why are you using varargs to begin with?! Stop that at once! + + +Make header files compatible with C and C++ +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Non-portable example: + +.. code-block:: c++ + + /*oldCheader.h*/ + int existingCfunction(char*); + int anotherExistingCfunction(char*); + + /* oldCfile.c */ + #include "oldCheader.h" + ... + + // new file.cpp + extern "C" { + #include "oldCheader.h" + }; + ... + +If you make new header files with exposed C interfaces, make the header +files work correctly when they are included by both C and C++ files. + +(If you need to include a C header in new C++ files, that should just +work. If not, it's the C header maintainer's fault, so fix the header if +you can, and if not, whatever hack you come up with will probably be +fine.) + +Portable example: + +.. code-block:: c++ + + /* oldCheader.h*/ + PR_BEGIN_EXTERN_C + int existingCfunction(char*); + int anotherExistingCfunction(char*); + PR_END_EXTERN_C + + /* oldCfile.c */ + #include "oldCheader.h" + ... + + // new file.cpp + #include "oldCheader.h" + ... + +There are number of reasons for doing this, other than just good style. +For one thing, you are making life easier for everyone else, doing the +work in one common place (the header file) instead of all the C++ files +that include it. Also, by making the C header safe for C++, you document +that "hey, this file is now being included in C++". That's a good thing. +You also avoid a big portability nightmare that is nasty to fix... + + +Use override on subclass virtual member functions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ``override`` keyword is supported in C++11 and in all our supported +compilers, and it catches bugs. + + +Always declare a copy constructor and assignment operator +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Many classes shouldn't be copied or assigned. If you're writing one of +these, the way to enforce your policy is to declare a deleted copy +constructor as private and not supply a definition. While you're at it, +do the same for the assignment operator used for assignment of objects +of the same class. Example: + +.. code-block:: c++ + + class Foo { + ... + private: + Foo(const Foo& x) = delete; + Foo& operator=(const Foo& x) = delete; + }; + +Any code that implicitly calls the copy constructor will hit a +compile-time error. That way nothing happens in the dark. When a user's +code won't compile, they'll see that they were passing by value, when +they meant to pass by reference (oops). + + +Be careful of overloaded methods with like signatures +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +It's best to avoid overloading methods when the type signature of the +methods differs only by one "abstract" type (e.g. ``PR_Int32`` or +``int32``). What you will find as you move that code to different +platforms, is suddenly on the Foo2000 compiler your overloaded methods +will have the same type-signature. + + +Type scalar constants to avoid unexpected ambiguities +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Non-portable code: + +.. code-block:: c++ + + class FooClass { + // having such similar signatures + // is a bad idea in the first place. + void doit(long); + void doit(short); + }; + + void + B::foo(FooClass* xyz) + { + xyz->doit(45); + } + +Be sure to type your scalar constants, e.g., ``uint32_t(10)`` or +``10L``. Otherwise, you can produce ambiguous function calls which +potentially could resolve to multiple methods, particularly if you +haven't followed (2) above. Not all of the compilers will flag ambiguous +method calls. + +Portable code: + +.. code-block:: c++ + + class FooClass { + // having such similar signatures + // is a bad idea in the first place. + void doit(long); + void doit(short); + }; + + void + B::foo(FooClass* xyz) + { + xyz->doit(45L); + } + + +Use nsCOMPtr in XPCOM code +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See the ``nsCOMPtr`` `User +Manual <https://developer.mozilla.org/en-US/docs/Using_nsCOMPtr>`__ for +usage details. + + +Don't use identifiers that start with an underscore +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This rule occasionally surprises people who've been hacking C++ for +decades. But it comes directly from the C++ standard! + +According to the C++ Standard, 17.4.3.1.2 Global Names +[lib.global.names], paragraph 1: + +Certain sets of names and function signatures are always reserved to the +implementation: + +- Each name that contains a double underscore (__) or begins with an + underscore followed by an uppercase letter (2.11) is reserved to the + implementation for any use. +- **Each name that begins with an underscore is reserved to the + implementation** for use as a name in the global namespace. + + +Stuff that is good to do for C or C++ +------------------------------------- + + +Avoid conditional #includes when possible +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Don't write an ``#include`` inside an ``#ifdef`` if you could instead +put it outside. Unconditional includes are better because they make the +compilation more similar across all platforms and configurations, so +you're less likely to cause stupid compiler errors on someone else's +favorite platform that you never use. + +Bad code example: + +.. code-block:: c++ + + #ifdef MOZ_ENABLE_JPEG_FOUR_BILLION + #include <stdlib.h> // <--- don't do this + #include "jpeg4e9.h" // <--- only do this if the header really might not be there + #endif + +Of course when you're including different system files for different +machines, you don't have much choice. That's different. + + +Every .cpp source file should have a unique name +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Every object file linked into libxul needs to have a unique name. Avoid +generic names like nsModule.cpp and instead use nsPlacesModule.cpp. + + +Turn on warnings for your compiler, and then write warning free code +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +What generates a warning on one platform will generate errors on +another. Turn warnings on. Write warning-free code. It's good for you. +Treat warnings as errors by adding +``ac_add_options --enable-warnings-as-errors`` to your mozconfig file. + + +Use the same type for all bitfields in a ``struct`` or ``class`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Some compilers do not pack the bits when different bitfields are given +different types. For example, the following struct might have a size of +8 bytes, even though it would fit in 1: + +.. code-block:: c++ + + struct { + char ch: 1; + int i: 1; + }; + + +Don't use an enum type for a bitfield +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The classic example of this is using ``PRBool`` for a boolean bitfield. +Don't do that. ``PRBool`` is a signed integer type, so the bitfield's +value when set will be ``-1`` instead of ``+1``, which---I know, +*crazy*, right? The things C++ hackers used to have to put up with... + +You shouldn't be using ``PRBool`` anyway. Use ``bool``. Bitfields of +type ``bool`` are fine. + +Enums are signed on some platforms (in some configurations) and unsigned +on others and therefore unsuitable for writing portable code when every +bit counts, even if they happen to work on your system. |
