Source code
Revision control
Copy as Markdown
Other Tools
---
name: api-compat-reviewer
description: Automated code review for backward-incompatible changes in WebRTC public headers. Use this skill to catch renames/removals/signature changes in api/ (and other downstream-consumed headers like pc/session_description.h) that will break Chromium and internal builds, and to verify a deprecation polyfill is in place.
---
# 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]]`.
2. **Migrate downstream** to the new API, following the instructions
the CL author put in the commit message (see "Commit message
guidance" below).
3. **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.
2. **Removed symbols**: public method, free function, type, enum,
enum value, constant, or member field deleted outright.
3. **Changed signatures**: return type changed, parameter type changed,
parameter added without a default, parameter order changed.
4. **New pure virtual methods**: `virtual ... = 0;` added to an
abstract base class in `api/` without a default implementation. This
breaks every downstream subclass.
5. **Tighter enums**: switching an `enum` to `enum class` is a breaking
change for any caller that relied on implicit-int conversion.
6. **Changed enum value numbers**: reordering or inserting values
shifts the underlying integers. Particularly bad for UMA/histogram
enums where values are persisted.
7. **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:
```cpp
// 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.
3. **Check for polyfill** in the same CL or in a follow-up commit on
the same branch. If absent, this is the headline finding.
4. **Check the commit message** mentions the migration plan and the
tracking bug.
5. **Cross-reference** [checklist.md](references/checklist.md) for
items, and [bad_patterns.md](references/bad_patterns.md) for
common anti-patterns.
6. **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.