summaryrefslogtreecommitdiffstats
path: root/docs/contributing/reviewer_checklist.rst
diff options
context:
space:
mode:
authorDaniel Baumann <daniel.baumann@progress-linux.org>2024-04-07 19:33:14 +0000
committerDaniel Baumann <daniel.baumann@progress-linux.org>2024-04-07 19:33:14 +0000
commit36d22d82aa202bb199967e9512281e9a53db42c9 (patch)
tree105e8c98ddea1c1e4784a60a5a6410fa416be2de /docs/contributing/reviewer_checklist.rst
parentInitial commit. (diff)
downloadfirefox-esr-36d22d82aa202bb199967e9512281e9a53db42c9.tar.xz
firefox-esr-36d22d82aa202bb199967e9512281e9a53db42c9.zip
Adding upstream version 115.7.0esr.upstream/115.7.0esr
Signed-off-by: Daniel Baumann <daniel.baumann@progress-linux.org>
Diffstat (limited to 'docs/contributing/reviewer_checklist.rst')
-rw-r--r--docs/contributing/reviewer_checklist.rst181
1 files changed, 181 insertions, 0 deletions
diff --git a/docs/contributing/reviewer_checklist.rst b/docs/contributing/reviewer_checklist.rst
new file mode 100644
index 0000000000..cfe772dba9
--- /dev/null
+++ b/docs/contributing/reviewer_checklist.rst
@@ -0,0 +1,181 @@
+Reviewer Checklist
+==================
+
+ Submitting patches to Mozilla source code needn't be complex. This
+ article provides a list of best practices for your patch content that
+ reviewers will check for or require. Following these best practices
+ will lead to a smoother, more rapid process of review and acceptance.
+
+
+Good web citizenship
+--------------------
+
+- Make sure new web-exposed APIs actually make sense and are either
+ standards track or preffed off by default.
+- In C++, wrapper-cache as needed. If your object can be gotten from
+ somewhere without creating it in the process, it needs to be
+ wrapper-cached.
+
+
+Correctness
+-----------
+
+- The bug being fixed is a valid bug and should be fixed.
+- The patch fixes the issue.
+- The patch is not unnecessarily complicated.
+- The patch does not add duplicates of existing code ('almost
+ duplicates' could mean a refactor is needed). Commonly this results
+ in "part 0" of a bug, which is "tidy things up to make the fix easier
+ to write and review".
+- If QA needs to verify the fix, you should provide steps to reproduce
+ (STR).
+
+
+Quality
+-------
+
+- If you can unit-test it, you should unit-test it.
+- If it's JS, try to design and build so that xpcshell can exercise
+ most functionality. It's quicker.
+- Make sure the patch doesn't create any unused code (e.g., remove
+ strings when removing a feature)
+- All caught exceptions should be logged at the appropriate level,
+ bearing in mind personally identifiable information, but also
+ considering the expense of computing and recording log output.
+ [Fennec: Checking for log levels is expensive unless you're using
+ Logger.]
+
+
+Style
+-----
+
+- Follow the `style
+ guide <https://firefox-source-docs.mozilla.org/code-quality/coding-style/index.html>`__
+ for the language and module in question.
+- Follow local style for the surrounding code, even if that local style
+ isn't formally documented.
+- New files have license declarations and modelines.
+- New JS files should use strict mode.
+- Trailing whitespace (git diff and splinter view both highlight this,
+ as does hg with the color extension enabled). Whitespace can be fixed
+ easily in Mercurial using the `CheckFiles
+ extension <https://www.mercurial-scm.org/wiki/CheckFilesExtension>`__.
+ In git, you can use git rebase --whitespace=fix.
+
+
+Security issues
+---------------
+
+- There should be no writing to arbitrary files outside the profile
+ folder.
+- Be careful when reading user input, network input, or files on disk.
+ Assume that inputs will be too big, too short, empty, malformed, or
+ malicious.
+- Tag for sec review if unsure.
+- If you're writing code that uses JSAPI, chances are you got it wrong.
+ Try hard to avoid doing that.
+
+
+Privacy issues
+--------------
+
+- There should be no logging of URLs or content from which URLs may be
+ inferred.
+- [Fennec: Android Services has Logger.pii() for this purpose (e.g.,
+ logging profile dir)].
+- Tag for privacy review if needed.
+
+
+Resource leaks
+--------------
+
+- In Java, memory leaks are largely due to singletons holding on to
+ caches and collections, or observers sticking around, or runnables
+ sitting in a queue.
+- In C++, cycle-collect as needed. If JavaScript can see your object,
+ it probably needs to be cycle-collected.
+- [Fennec: If your custom view does animations, it's better to clean up
+ runnables in onDetachFromWindow().]
+- Ensure all file handles and other closeable resources are closed
+ appropriately.
+- [Fennec: When writing tests that use PaintedSurface, ensure the
+ PaintedSurface is closed when you're done with it.]
+
+
+Performance impact
+------------------
+
+- Check for main-thread IO [Fennec: Android may warn about this with
+ strictmode].
+- Remove debug logging that is not needed in production.
+
+
+Threading issues
+----------------
+
+- Enormous: correct use of locking and volatility; livelock and
+ deadlock; ownership.
+- [Fennec: All view methods should be touched only on UI thread.]
+- [Fennec: Activity lifecycle awareness (works with "never keep
+ activities"). Also test with oom-fennec
+ (`https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/) <https://hg.mozilla.org/users/blassey_mozilla.com/oom-fennec/%29>`__].
+
+
+Compatibility
+-------------
+
+- Version files, databases, messages
+- Tag messages with ids to disambiguate callers.
+- IDL UUIDs are updated when the interface is updated.
+- Android permissions should be 'grouped' into a common release to
+ avoid breaking auto-updates.
+- Android APIs added since Froyo should be guarded by a version check.
+
+
+Preffability
+------------
+
+- If the feature being worked on is covered by prefs, make sure they
+ are hooked up.
+- If working on a new feature, consider adding prefs to control the
+ behavior.
+- Consider adding prefs to disable the feature entirely in case bugs
+ are found later in the release cycle.
+- [Fennec: "Prefs" can be Gecko prefs, SharedPreferences values, or
+ build-time flags. Which one you choose depends on how the feature is
+ implemented: a pure Java service can't easily check Gecko prefs, for
+ example.]
+
+
+Strings
+-------
+
+- There should be no string changes in patches that will be uplifted
+ (including string removals).
+- Rev entity names for string changes.
+- When making UI changes, be aware of the fact that strings will be
+ different lengths in different locales.
+
+
+Documentation
+-------------
+
+- The commit message should describe what the patch is changing (not be
+ a copy of the bug summary). The first line should be a short
+ description (since only the first line is shown in the log), and
+ additional description, if needed, should be present, properly
+ wrapped, in later lines.
+- Adequately document any potentially confusing pieces of code.
+- Flag a bug with dev-doc-needed if any addon or web APIs are affected.
+- Use Javadocs extensively, especially on any new non-private methods.
+- When moving files, ensure blame/annotate is preserved.
+
+
+Accessibility
+-------------
+
+- For HTML pages, images should have the alt attribute set when
+ appropriate. Similarly, a button that is not a native HTML button
+ should have role="button" and the aria-label attribute set.
+- [Fennec: Make sure contentDescription is set for parts of the UI that
+ should be accessible]