前端代码审查最佳实践¶
简介¶
代码审查有助于确保代码解决了特定问题，没有错误或导致其他功能倒退，并且具有足够的测试覆盖率。它还提供了一个机会，可以确保添加正确的文档，无论是通过代码注释还是更改文档。审查代码的过程还有助于在开发人员之间传播有关代码工作原理的知识，无论是作为补丁作者还是审查者。
在将代码或测试添加到 Mozilla 的代码库之前，需要进行审查。批准后，代码将合并到 autoland，并且一旦测试通过，管理员将将其与其他提交合并到 mozilla-central。本指南概述了补丁作者和审查者的一套标准。
目标受众和预期成果¶
本文档主要面向 Firefox 桌面前端团队的工程师。本指南的目的是使团队围绕一组商定的补丁审查期望保持一致，这将减少混淆，防止误解，帮助更有效地合并补丁，并帮助新团队成员入职。
作为一名工程师，您将处于补丁作者和审查者的双重角色，因此建议充分了解这两个角色的期望。
前端代码审查的期望是什么？¶
以下部分概述了审查代码时的一些高级期望。另请参阅 技术和细节部分，了解有关前端代码的特定查找内容的更多详细信息。
基础知识¶
代码审查和自动化将检查此补丁是否
干净地应用于 mozilla-central 并可以构建。
具有 良好的提交消息，描述更改以及在不明显的情况下更改的原因。
在提交消息中添加更长的描述将用作 Phabricator 中的摘要。
修复了手头的问题。
在适当的情况下具有自动化测试覆盖率。 例外情况通过在 Phabricator 中使用 test-exception-* 标签来涵盖。以下是一些前端中不需要 [新] 测试覆盖率的常见例外情况
样式/图像/仅限 l10n 的更改
以我们无法在自动化测试中模拟的方式直接与操作系统交互的代码。
需要非常具体的外部环境（操作系统设置、挂钟时间、其他程序的操作等）的代码，我们无法可靠地模拟。
现有代码的重构（在存在现有测试覆盖率的情况下）
删除代码
通过 lint 和/或自动化测试。
**如果其中任何一项缺失/损坏，审查者可能会立即停止。**在提交补丁之前，代码作者应确保已涵盖这些内容。
是否必要且充分？¶
查看补丁的两种有用方法是“这是必要的吗？”和“这足够了吗？”
**必要**：补丁中的所有更改都是必需的吗？在修复错误时发现其他问题是很常见的，但修复此特定错误不需要的代码应移动到单独的错误中。审查者通常会标记
不相关的代码更改
更改不相关的空格等（注意编辑器的自动格式化）
不必要地复杂或笨拙的修复
**充分**：这是否完全修复了问题？也就是说，我们是否查看了极端情况，并且补丁/补丁集是否确实修复了所有问题，而不仅仅是部分问题或其症状？
这也意味着**考虑补丁中没有的内容**。例如，在重命名方法时，补丁作者和审查者不应仅检查补丁中的所有替换是否正确，还应检查补丁中是否实际触及了所有出现的位置（searchfox 应该使这相对简单，或者您可以应用补丁在本地并使用
grep/ack/ag等）。
审查者的期望¶
进步 > 完美¶
如上所述，补丁的上下文应由作者提供，但是重要的是要努力在确保我们保持高标准和能够取得进展之间取得平衡。如果您发现经常出现的问题可以通过自动 lint 来解决，请提交问题以添加新的规则。这有助于我们确保工程师投入审查的时间主要用于有效地确保更改解决了其目标，并且是对 Firefox 的可维护且安全的补充。
审查意见中的语言和语气¶
花时间感谢并指出好的代码更改。
专注于代码，而不是补丁作者（避免使用听起来像是在批评作者的措辞，例如“这没有意义，你为什么这样做了？”与“我很好奇你为什么采用这种方法。你能提供一些上下文吗？”）。
将评论框定为问题而不是陈述可以帮助创造一个有利于协作的环境。而不是“当
bar为undefined时，这不起作用”，而是“请检查此代码是否正确处理了bar为undefined的情况？”。使用“请”和“你怎么认为？”在很大程度上可以使他人感觉自己是同事，而不是下属。作为审查者，仔细检查您的评论。假设代码作者比您花费了更多时间来思考代码的这一部分，并且实际上可能是正确的，即使您最初认为某些内容是错误的。
明确哪些更改需要更改才能合并补丁，哪些更改是可选的。
对于不是 nits 或不言而喻的改进，请解释您为什么请求更改。了解请求更改背后的原因有助于确保您和补丁作者对补丁的作用有共同的理解，并帮助补丁作者学习。
最大程度地减少审查和处理审查意见所需的轮次。在可能的情况下，在一次通过中提供您的审查意见，而不是多次迭代。这有助于减少完成审查所需的周期以及与审查者和补丁作者相关的任何上下文切换。如果您没有时间进行全面审查，请说明您的审查目前不完整并设定期望，以便作者可以相应地计划。
当代表添加到补丁的其他审查组进行代码审查时，您的审查应主要侧重于审查组负责的领域或您的组正在使用的接口。但是，如果您在审查期间确实发现了在此范围之外的非平凡问题，那么标记它将是合理的。
审查的周转时间¶
目标是在 1 个工作日内做出回应。这不需要进行全面审查，但可以是一条评论，设定您何时能够审查补丁的期望，指出其他可能更快地审查补丁或更适合审查补丁的人。
如果您被明确要求进行审查，但由于时间限制无法完成，那么为补丁作者设定期望总是有帮助的。
技术和细节¶
本节介绍了评审人员可能会关注的补丁中的一些前端特定方面。
JS/DOM¶
使用现有的组件来实现功能设计。不要根据设计和原生 HTML 重新实现自定义按钮/卡片/切换/…样式，因为它不具有可扩展性，难以维护，并且可能会错过辅助功能、RTL 等的边缘情况。
反之亦然：并非您处理的每一块新代码都需要立即作为通用、可重用的组件进行分解。当您是某个代码块的第一个/唯一使用者时，通常不可能预测代码的哪些方面需要“通用”，API 边界应该在哪里等等。尝试立即进行操作注定会失败，因此不要花时间在这上面 - 保留到第二个潜在使用者出现时再进行。
代码行数是一个有缺陷的指标，我们不是在进行代码高尔夫 - 但当 2 行内联代码可以做到时，不要编写几个 10 行的函数。适当地平衡简洁性和可读性。
在可能的情况下，使用 DOM 属性而不是属性。
在不可能的情况下，
toggleAttribute和classList.toggle通常有助于避免重复的 if/else 块。
样式和 CSS 以及 SVG¶
使用现有的组件/类来实现功能设计（请参阅 JS/DOM 部分）。
请记住，所有 CSS 都需要在 RTL 语言中工作。使用逻辑属性（
margin-inline-start及其朋友）而不是物理属性（margin-left）。有关更详细的信息，请参阅Firefox RTL（从右到左）指南。
请参阅有关颜色、HCM 等使用的辅助功能部分。
请参阅详细的CSS 编写指南。
请参阅Firefox SVG 指南。
本地化¶
对于任何面向用户的內容，请使用本地化字符串。不要在标记中硬编码英文字符串。
对于新代码，请使用 fluent。对于修改旧代码，请使用旧代码使用的代码（但如果很直观，请考虑切换到 fluent，因为它在其他语言中提供了更好的翻译原语）。
在编写仅需要 en-US 字符串的实验性功能时，这些字符串尚未最终确定，请使用 fluent 并将 ftl 文件放在
content而不是locale目录中，并相应地打包它。当字符串最终确定后，将其移动到常规的locale目录中并像往常一样包含它们，但请确保在字符串冻结之外进行操作，同时为我们的（主要是志愿者）本地化人员提供合理的时间来提交翻译 - 不要只是在字符串冻结前一天将几十个字符串转储到locale中。如果字符串的含义发生更改，或者[在 fluent 中]您添加/删除属性，则必须更新消息标识符。
更详细的fluent 审查指南单独提供。
辅助功能 {#accessibility}¶
编写语义 HTML。
任何可以通过鼠标访问的内容都应该可以通过键盘访问，或者应该有可以通过键盘访问的等效项。
确保事物在高对比度模式下工作，并且在非高对比度模式下以及使用不同主题时，前景和背景项目之间存在足够的颜色对比度。
任何不是纯装饰性的图像（例如，没有可见文本标签的工具栏按钮图标）都需要一个
alt属性或aria-label或类似属性来为其提供可访问的名称，以及为可能不理解图标的视力正常的用户提供工具提示。任何是纯装饰性的图像（例如，文本旁边的插图）都应使用以下选项之一实现
CSS 背景图像，在辅助功能树中没有表示
使用
role=presentation从辅助功能树中移除它们的 HTMLimg元素，并且没有 alt 属性。
如果实现动画或过渡，请在
prefers-reduced-motion媒体查询后面执行此操作，以确保患有癫痫或其他运动触发敏感性的人不会因我们试图使事物变得美观的尝试而受到伤害。ARIA 的第一条规则是“不要使用 ARIA”（另请参阅“编写语义 HTML”） - 但它可以作为语义标记不足时的工具（例如，实时区域、复杂的标签结构）。
性能¶
对于启动路径上的任何内容和/或 CPU 或磁盘密集型工作（实际上不需要立即发生），请使用
requestIdleCallback和/或工作线程。使用
IOUtils进行异步文件 I/O，而不是使用 nsIFile 进行同步访问。牢记复杂性并使用合理的数据结构
例如，如果集合很大、无序并且您需要在集合中查找项目，请使用 Set 而不是数组
请记住，某些集合/循环在常见情况下可能大小合理，但具有病态的边缘情况（拥有 3652 个标签的那个人正在看着你。你知道我在说什么！）。您不需要跳过太多障碍，但使用备忘录或关联（它们是 O(1) 或 O(log n)）而不是“仅仅”遍历列表（它是 O(n)）。
除此之外，除非代码已知是热点/性能敏感的，否则不要过度优化。
例如，我们通常不使用
for (var i; i < ary.length; i++)样式的“原始”数组循环，而是更喜欢更易读的for (let item of ary)，即使这稍微降低了性能。
安全¶
如有疑问，请咨询安全团队和/或 Slack 上的
#security。在进程之间或与 Web 内容通信时要小心。具体来说
我们不能信任 Web 内容。
我们不能信任 Web 内容进程（！）您通过 IPC（JSActor 消息或类似消息）获得的任何信息都可能是完全虚假的。尽可能使用来自父进程（CanonicalBrowsingContext 等）的信息来做出安全决策。
作为推论，如果您有想要导航或与 Web 内容交互的代码，请完全在内容进程中执行此操作，而不是例如将 URI 或类似信息传递给父进程并期望它为您导航。这避免了将 API 公开给（可能已受到攻击的）内容进程，使它们能够任意导航 - 相关的“基本”API（
document.location等）将由 Gecko 本身保护，我们希望尽可能重用该基础设施。不要在父进程中加载不受信任的内容（即来自 Web 的内容）。
XSS 和标记相关建议
尽可能避免内联事件处理程序。
所有
about:页面必须具有内容安全策略 (CSP)。使用一个阻止内联样式和内联脚本的策略。不要使用
innerHTML。Gecko 内置了清理程序 API - 如果你需要处理不可信的 HTML，请参考 nsIParserUtils。不要手动编写清理输入的代码。
其他需要考虑的因素
URL 是可变的，并且不是识别来源的好方法。使用 nsIPrincipal 对象。你需要使用它来正确处理
blobURI、about:blank和数据 URI。我们不能假设对于
something.foo.sometld，something是foo的子域，或者foo.sometld是“顶级”域。某些顶级域（.co.uk和.org.uk等等是一个明显的例子，但github.io和s3.dualstack.ap-northeast-1.amazonaws.com也是！）拥有单独控制的子域。使用 nsIEffectiveTLDService 来确定“顶级”域是什么。不要使用手动字符串操作。欺骗：攻击者试图让用户相信他们正在访问不同的网站，通常是通过在地址栏或其他可信 UI 上覆盖内容或标记，或使用全屏或类似的技巧。
点击劫持/按键劫持：攻击者诱导用户反复点击或按键（“如果你快速点击这个按钮钩住小鸭子，就能赢 50 美元”），并利用它绕过安全警告。典型的防御措施：延迟启用安全关键对话框上的按钮 - 甚至还有一个 助手 用于此目的。
加密：不要自己编写加密、随机数生成器、密码/密钥管理或类似的东西。与专家交谈（其中一些在 Mozilla 工作）！
测试¶
所有补丁都必须尽可能地包含自动化测试。
在编写测试时，请确保你已经看到过你的测试失败。这有助于避免测试即使在不应该通过时也始终通过的情况。
有些补丁可能不适合编写测试，特别是那些只涉及文档、样式（CSS/图像）或仅测试更改的补丁。
对于无法编写测试的情况，例如与操作系统以我们无法自动验证的方式交互的功能，请向审阅者留下解释原因的注释。这很可能是他们提出的第一个问题，他们应该知道何时会出现这种情况，或者可能知道编写测试的其他方法。
如果你已经推送到 try，在 Phabricator 上留下 try 推送的链接可以帮助避免重复工作（注意：默认情况下，审阅者不需要创建 try 推送）。如果存在可能间歇性的不明确的测试失败，你的审阅者可以帮助找出发生了什么。
文档¶
理想情况下，代码应该无需解释即可阅读。在需要文档的地方，使用代码注释来解释单个代码片段，并使用扩展的提交消息（即提交消息的第二/第三/第 n 行）来描述更改的高级目标。
代码注释的指导原则是，在它们提供更改原因的额外上下文时添加它们，但如果注释仅重述代码正在执行什么，则避免添加它们。
如果审阅者要求提供解释，这通常表示代码应该简化、需要更多代码注释或补丁中的方法不正确。优先选择代码和/或注释更改，而不是仅在 Phabricator 中进行沟通 - 这样未来的代码阅读者就可以拥有相同的上下文。
确保任何现有的 Firefox 源代码文档材料也已更新以反映更改。
接管补丁¶
补丁只能在与原始补丁作者达成协议的情况下或作者无法及时响应（休假或病假或不再参与项目）时才能接管。
如果你确实需要接管补丁，可以使用以下方法来维护原始作者。使用 hg commit --amend --user "Other Person <[email protected]>" 或 git commit --amend --author="Other Person <[email protected]>" 在修改原始提交时。
社区参与指南¶
请记住始终遵守 社区参与指南。
慷慨和包容。始终假设善意，即使在审查过程中存在分歧。
常见问题解答¶
什么是 Nit？¶
Nit 是一个轻微的缺陷。这可能是错别字或一个小问题。这些通常是不会导致“请求更改”的事情，但应该指出来，并且预期补丁作者在补丁落地之前会解决它们。
如何为新的 lint 规则提交 bug？¶
如果你有关于 eslint（JS）或 stylelint（CSS）规则的想法，你可以在此处提交 bug，如果更具体的组件更合适。
进一步阅读¶
代码审查反模式，Simon Tatham 撰写的一篇文章，概述了代码审查中的常见反模式。