Code Review

This checklist comes from recurring reviewer feedback on landed DevTools CLs. Use it for self-review before upload and for reviewing other people's CLs.

1. Gather context

• Automated Review Agent (diff already in prompt): Do not run shell commands. Use read_file to inspect enclosing classes/functions/tests and search_files to verify callers or conventions across the repository.
• Local CLI / Interactive session (no diff in prompt):
  • Local change: git diff origin/main...HEAD (or git show HEAD for a single-commit branch). To learn about the branch and upload workflow, see the devtools-version-control skill.
  • Gerrit CL: strip the leading )]}' line from each response to get JSON:
    • Diff: git cl diff (for the current branch), or fetch the patch set.
    • Inline comments: curl -s https://chromium-review.googlesource.com/changes/devtools%2Fdevtools-frontend~<CL_NUMBER>/comments | tail -n +2
    • Messages: .../changes/devtools%2Fdevtools-frontend~<CL_NUMBER>/messages
  • Read the CL description first. Then check that the diff does what the description says, and nothing else.

2. Consult the specialized skills and rules

Load the skill or .agents/rules/ files that match what the diff touches, and apply their rules:

3. Boundaries vs. Interiors

• Enforce module boundaries strictly: Prioritize boundary invariants—module entrypoints (foo.ts, foo-meta.ts), GN visibility and dependency edges, strict core -> models -> ui -> panels layering, SettingDescriptor placement in the lowest layer where used, public widget/component APIs (DEFAULT_VIEW input/output contracts), CDP protocol contracts, and test validity.
• Avoid uncodified interior style nits: Do not flag subjective stylistic preferences inside module interiors unless backed by a lint rule or an explicit rule under .agents/rules/.

4. Shared False-Positive Guards

Check these guards before reporting any finding (including general review findings):

• Lit @SomeEvent=${handler} bindings on <devtools-widget> (or a widget's element):
  • Do not flag @SomeEvent=${handler} as "never fires" or "lowercased by the HTML parser" when the child widget class is built with Common.ObjectWrapper.eventMixin(...) on top of UI.Widget.Widget / UI.Widget.VBox / UI.Widget.HBox:
    1. eventMixin.dispatchEventToListeners (front_end/core/common/Object.ts) also calls this.dispatchDOMEvent(new CustomEvent(eventType, {detail: eventData[0]})), and Widget.dispatchDOMEvent dispatches it on widget.element (which is the <devtools-widget> host element when instantiated with widget()).
    2. Lit preserves the exact case of @, ., and ? binding names by reading the original template strings (attrNames[] in front_end/third_party/lit/lib/lit.js), not the lowercased attribute names from the HTML parser. So @PaintProfilerRequested registers addEventListener('PaintProfilerRequested', ...), and event.detail carries the first payload argument.
  • Do flag it when:
    1. The event source extends Common.ObjectWrapper.ObjectWrapper directly or holds one by composition instead of eventMixin on UI.Widget (it has no dispatchDOMEvent, so no DOM CustomEvent is fired).
    2. The listener is attached on an ancestor element above the widget's host element (new CustomEvent(...) in Object.ts does not set bubbles: true).
    3. The @SomeEvent handler reads (event as any).data instead of (event as CustomEvent<T>).detail.
• Multi-line html template whitespace (front_end/ui/lit/strip-whitespace.ts):
  • Do not flag newlines and indentation spaces between expressions and inline elements in multi-line html templates from ui/lit/lit.js. DevTools' custom html tag strips \n\s* outside <...> tags at runtime.
• Generated .css.js imports in <style>${styles}</style>:
  • Do not flag interpolating *.css.js default imports into <style> in Lit templates. Generated .css.js files export plain CSS strings, not CSSStyleSheet objects.

5. Test correctness (the most common review feedback)

• Tautological tests: A test must exercise real production code. If a mock does the thing the test asserts (for example, a mocked DOM.undo that calls removeSection() directly, followed by an assertion that the section was removed), the test checks the mock and nothing else. Mock the boundary instead (for example, have the CDP stub emit CSSModel.Events.StyleSheetChanged) and let production code react to it.
• Vacuous tests: Look for local promises or data that are never connected to DevTools logic, or stubs (like initialize) that skip the code path under test. These tests pass because nothing happens.
• Assertions that check the setup: For example, assert.isNull(pane.node()) right after pane.setNodeForTest(null). Each assertion should verify the behavior that the test name describes.
• Test lives where it belongs: A test in Foo.test.ts should exercise Foo. Move logic-only tests to the model's test file (for example, CSSMatchedStyles.test.ts). Otherwise, render the UI and assert on the DOM.
• Test covers the scenario it claims: For example, an "iframe event listeners" test must select a node inside the iframe. It is not enough that it passes on <body> of the main frame.
• Right suite: Ask whether an E2E test could be a unit test, or whether the E2E coverage is intentional for a user story. See devtools-testing-guidance.
• E2E selectors match real UI: Labels and aria-labels must match the actual strings, for example "Show user agent shadow DOM" and not "User agent shadow DOM". Check that the setting being toggled actually affects the scenario.
• Fail fast: Throw or assert when a required value is missing. Don't let the test time out later.

6. Test hygiene and cleanup

• Pair every beforeEach singleton creation with a cleanup in afterEach (for example, CSSWorkspaceBinding.removeInstance() or WorkspaceImpl.removeInstance()) so that state does not leak between tests.
• Restore fake timers in try { … } finally { clock.restore(); }, or use the sandbox or cleanup that the test runner provides. Keep the scope of fake timers small.
• Don't stub the same method twice without calling restore() first. Sinon throws Attempted to wrap … which is already wrapped when the object is the same instance.
• Remove guards that were copied from helpers for no reason (for example, if (!stub.called) … in a test that stubs the method only once).

7. Code clarity

• Don't take detours to reach a value. If a module is imported, call TextUtils.TextRange.TextRange.fromObject(...) directly instead of going through rule.style.range.constructor.
• Look for dead code, leftover debug code, commented-out blocks, and unrelated formatting churn.
• Check that the change stays in scope. Flag unrelated refactors that should be split into a separate CL.
• Check for lifecycle issues: event listeners and observers that are added without being removed, and async work that continues after a widget is detached.

8. CL hygiene

• The description explains why, including external dependencies (for example, "V8 will stop emitting X; this must land first").
• Include a Bug: or Fixed: trailer, or Bug: None. Keep lines under 72 characters.
• For migrations and stacked series, list the legacy tests or files covered, and number the series (for example, (13/16)).
• Disabled tests reference a bug.
• Presubmit, lint, and format pass. See devtools-verification.

9. Writing review comments

• Be specific: cite file and line, explain the concrete failure mode (such as a leak, a hang, or "passes vacuously"), and suggest a fix. Include a code snippet when it helps.
• Separate blocking issues from nits and questions (for example, "Should we move this to a unit test?").
• When a reviewer's concern is not an actual bug, reply with the rationale. Don't make a silent change.