browserworks/waterfox-android/api-compat-reviewer

API Compat Reviewer Skill

libWebRTC is consumed by downstream projects (Chromium, and internal, closed-source Google code) that build out-of-tree against a moving WebRTC checkout. Any rename, removal, or signature change to a public symbol breaks the next downstream import and triggers a revert. The upstream reviewer cannot see the closed-source consumers, so the burden is on the CL author to keep the old API in place until those consumers have migrated.

The required workflow is always three steps:

1. Land the new API alongside the old one. The old symbol stays as

a thin wrapper / translation layer that delegates to the new implementation, marked [[deprecated]].

1. Migrate downstream to the new API, following the instructions

the CL author put in the commit message (see "Commit message guidance" below).

1. Remove the old API in a follow-up CL once all known consumers

have moved.

This skill catches CLs that skip step 1 and verifies the migration path is in place.

The canonical incident: a CL renamed MediaContentDescription::ExtmapAllowMixed → AttributeLevel (and the matching getters/setters) in pc/session_description.h. It was reverted within hours with the message:

"Breaks downstream projects. The definitions in session_description.h

must be kept in parallel until downstream projects are updated."

The reland kept the new names but added a "polyfill" commit that restored the old enum and methods as [[deprecated]] shims delegating to the new ones.

What counts as a "public" header / symbol

• Always public: every .h file under api/.
• Effectively public: headers outside api/ that downstream

projects include directly. pc/session_description.h is the proven example. When unsure, check BUILD.gn for permissive visibility (e.g. [ "*" ]) or grep Chromium for #include of the header.

• Internal: headers under pc/, media/, modules/, etc. with

restricted visibility and no known downstream includes.

If the diff only touches internal headers, this skill has nothing to do. Say so and stop.

Breaking-change patterns to flag

For each modified public header, look for:

1. Renamed symbols: a removed identifier + an added identifier with

a similar name in the same class/namespace. Common shapes:

• enum X → enum class Y, value renames (kNo → kNone).
• set_foo_enum() → set_foo_level().
• Type rename in a parameter or return type.

1. Removed symbols: public method, free function, type, enum,

enum value, constant, or member field deleted outright.

1. Changed signatures: return type changed, parameter type changed,

parameter added without a default, parameter order changed.

1. New pure virtual methods: virtual ... = 0; added to an

abstract base class in api/ without a default implementation. This breaks every downstream subclass.

1. Tighter enums: switching an enum to enum class is a breaking

change for any caller that relied on implicit-int conversion.

1. Changed enum value numbers: reordering or inserting values

shifts the underlying integers. Particularly bad for UMA/histogram enums where values are persisted.

1. Default value changes on public methods or struct members, when

downstream relies on the old default.

Required mitigations

When a public header has any of the above, the CL must include a polyfill so downstream keeps compiling. The pattern is:

// New API
enum class AttributeLevel { kNone, kSession, kMedia };
void set_extmap_allow_mixed_level(AttributeLevel level);
AttributeLevel extmap_allow_mixed_level() const;
 
// TODO(bugs.webrtc.org/NNNNN): Remove once downstream has migrated.
enum [[deprecated("Use AttributeLevel")]] ExtmapAllowMixed {
  kNo, kSession, kMedia
};
[[deprecated("Use set_extmap_allow_mixed_level")]]
void set_extmap_allow_mixed_enum(ExtmapAllowMixed v) { /* delegate */ }
[[deprecated("Use extmap_allow_mixed_level")]]
ExtmapAllowMixed extmap_allow_mixed_enum() const { /* delegate */ }

Verify that:

• Both old and new names compile and link.
• The deprecated shim delegates to the new implementation; it does

not duplicate logic.

• Each shim has a [[deprecated("...")]] message naming the

replacement.

• A TODO: bugs.webrtc.org/NNNNN - description references the

tracking bug for removal.

• For pure-virtual additions, a default implementation is present

(e.g. { RTC_CHECK_NOTREACHED(); } or a sensible no-op).

Commit message guidance

The CL that introduces the new API must give downstream maintainers explicit, copy-pasteable migration instructions. Suggest a message shaped like:

api: introduce <NewName> alongside <OldName>
 
Renames <OldName> to <NewName>. The old symbol is preserved as a
[[deprecated]] wrapper that delegates to the new one, so this CL is
safe to roll into downstream projects without changes.
 
Downstream migration:
  - Replace <OldEnum>::<kOldValue> with <NewEnum>::<kNewValue>
  - Replace <old_method>() with <new_method>()
  - Replace #include "<old/path.h>" with #include "<new/path.h>"
 
After downstream has migrated, the deprecated symbols will be removed
in a follow-up CL tracked by bugs.webrtc.org/NNNNN.
 
Bug: webrtc:NNNNN

If the CL has no such migration block, flag it: closed-source downstream maintainers cannot migrate from a diff they cannot read.

Workflow

1. Filter the diff to public headers only. If none, stop.
2. Diff each header at the symbol level (use git diff on the

header in the CL range). For each removed/renamed/signature-changed symbol, classify it against the patterns above.

1. Check for polyfill in the same CL or in a follow-up commit on

the same branch. If absent, this is the headline finding.

1. Check the commit message mentions the migration plan and the

tracking bug.

1. Cross-reference checklist.md for

items, and bad_patterns.md for common anti-patterns.

1. Report findings grouped by header. For each breaking change,

state: the symbol, the kind of break, the required polyfill, and the bug to file/reference.

Tone and Style

• Direct: "This rename will break downstream. Add a polyfill."
• Concrete: name the symbol, point at the line, show the shim.
• Strict on hygiene: no polyfill = revert risk. Say so plainly.
• Quiet on the all-clear: if nothing public moved, one line is

enough.