Bug 1967025: Add a "layout code reviewer checklist" doc, to gather best practices for code review in layout code. r=TYLin,tlouw
As noted in the doc, this is meant to extend the general checklist at https://firefox-source-docs.mozilla.org/contributing/reviewer_checklist.html with examples and special cases that are particular to working in layout code. The list is short for now; this is just a start. Differential Revision: https://phabricator.services.mozilla.com/D249859
This commit is contained in:
committed by
dholbert@mozilla.com
parent
9c1c6ef7b9
commit
ecacf6a731
31
layout/docs/LayoutCodeReviewerChecklist.rst
Normal file
31
layout/docs/LayoutCodeReviewerChecklist.rst
Normal file
@@ -0,0 +1,31 @@
|
|||||||
|
Layout Code Reviewer Checklist
|
||||||
|
==============================
|
||||||
|
|
||||||
|
General
|
||||||
|
-------
|
||||||
|
- Follow the general `reviewer checklist
|
||||||
|
<https://firefox-source-docs.mozilla.org/contributing/reviewer_checklist.html>`__.
|
||||||
|
|
||||||
|
Security issues
|
||||||
|
---------------
|
||||||
|
|
||||||
|
- **Watch for raw pointers that may have their data deleted out from under
|
||||||
|
them**. Examples:
|
||||||
|
|
||||||
|
- If you ever have a raw pointer to a dynamically allocated object, it's good
|
||||||
|
to scrutinize whether the object might be destroyed before the last
|
||||||
|
possible use of the raw pointer. For example: if you have a local variable
|
||||||
|
that points to an object that's owned by a `frame's property table
|
||||||
|
<https://searchfox.org/mozilla-central/source/layout/base/FrameProperties.h>`__,
|
||||||
|
then consider whether the frame might remove/replace the property-table
|
||||||
|
entry (or whether the frame itself might be destroyed) inside any of the
|
||||||
|
function calls that happen while the local pointer is in scope.
|
||||||
|
- Be aware that layout flushes
|
||||||
|
(e.g. ``doc->FlushPendingNotifications(FlushType::Layout)``) can
|
||||||
|
synchronously cause the frame tree (and even the document!) to be
|
||||||
|
destroyed. Specifically: a layout flush can synchronously cause resize
|
||||||
|
events to fire; and the event-listeners for those events can run arbitrary
|
||||||
|
script, which could e.g. remove the iframe element that's hosting the
|
||||||
|
document whose layout we're in the midst of flushing; and that can cause
|
||||||
|
that document to be immediately destroyed, if there aren't any other strong
|
||||||
|
references keeping it alive.
|
||||||
@@ -15,5 +15,6 @@ contains general information about layout and the layout team at Mozilla.
|
|||||||
LayoutOverview
|
LayoutOverview
|
||||||
DynamicChangeHandling
|
DynamicChangeHandling
|
||||||
Reftest
|
Reftest
|
||||||
|
LayoutCodeReviewerChecklist
|
||||||
LayoutDebugger
|
LayoutDebugger
|
||||||
AccessibleCaret
|
AccessibleCaret
|
||||||
|
|||||||
Reference in New Issue
Block a user