Formalize Media Chrome Processes (particular focus on review+approval)

[cjpillsbury]

After some discussions with folks on managing open source projects generally and a followup discussion with @heff, I'm hoping to get a bit more structure for some of the main processes with managing Media Chrome. I'm hoping this conversation will result in clearer expectations around maintaining the project, clearer expectations around prioritization of the project, and a faster cadence of getting contributions merged and released, as well as improvements on issue and discussion responses & resolution.

Proposed High Level Needs

• We should have a clear list of core contributors
  • @heff @cjpillsbury @luwes @gkatsev (@dylanjha @mmcc ?)
• We should have a weekly dev/core contributors meeting for triage/discussion with sufficient time to handle the cadence of issues, discussions, and PRs, and potential agendas for core contributors (including e.g. revising this)
• We should clarify who should be merging (reviewer* or submitter?)
  • I'd assume the reviewer.
• We should clarify when and how issues/discussions are triaged, resolved, closed, and who should be involved (try to use same scope categories for who?)
• We should clarify our release process, cadence, and ownership of said process
• We should clarify how effort is distributed among core contributors
• We should have better alignment on priorities (active & known needs for ongoing efforts that employe media chrome, such as those from mux-elements and its users, external community users’ requests, ideas from core contributors and potentially adjacent)
• If any of the rules, scopes, or types of efforts are causing issues with managing Media Chrome, need further clarification, merit reclassification, or are causing gridlock or significant delays, we should revisit them in our weekly meeting

Scopes/Sizes

The scope/size of a contribute/PR (and potentially a discussion/issue) determines who needs to be involved in approval/rejection as well as other formal processes for discussion, final merges, and potential escalation. Below is an attempt to provide reference types for different scopes/sizes of contributions, as well as some examples. Ideally, we want to come to fast agreement on what should fall under each scope and what the rules should be for each, since we can always revisit these details in our weekly meeting (See above).

Large Scope/Size:

Rules:

• Requires consensus by core contributors, to be discussed at weekly meeting for next steps
  • Approve, Close/Reject (with reasons why), Backlog, Define requirements for final approval, Request additional information (ideally that will be sufficient for final decision), other?

Types:

• Fundamental changes to the internal architecture of media-controller or its contract with other components
  • Examples: Moving core state management into separate modules, changes to the parts/slots available on the shadow DOM, changing the relationship between media-controller and the slotted media, moving from a “push” model to a “pull” model for media state, changes to auto-hide to handle more complex cases like not hiding loading indicator
• Fundamental changes to our build/codebase paradigms
  • Examples: moving icons and/or CSS to separate files that are imported and part of the build, expecting externally-defined and included CSS for Media Chrome, migrating codebase to TypeScript
• Fundamental/breaking changes to pre-existing UI components’ “public facing” documented interfaces
  • Examples: Refactoring the muted button to expect the volume value instead of “level”, Replacing muted/volume button with a combined “media-volume-button”
• New components/features that introduce major architectural concepts
  • Examples: Settings overlay menu, Themes, Quality Selector

Medium Scope:

Rules:

• 1 core contributor reviewer/approval required
• 48hr time window for additional comments/reviews/objections by additional core contributors, starting after the first round of feedback
  • If any disagreement, escalates to Large
  • To avoid extended back and forth on PRs between core contributors, if you're not the primary reviewer but have input on the PR, try to clearly distinguish between a general concern vs. a full rejection of merging the PR/contribution. Try using language such as "fine for now", "not a blocker", "can merge but needs a an additional fast follow PR", and consider adding yourself as a reviewer and rejecting the PR if you do not believe it should be merged.

Types:

• New components/features that introduce new concepts but don’t impact core architecture
  • Examples: Loading indicator, “Click to play/pause” gesture component, Adding “supported” detection and state management to media-controller & relevant components (e.g. airplay, volume), Subtitles Selector
• New components/features that conform closely to pre-existing components/paradigms but are not planned additions
• Fixing bugs that introduce regressions (to e.g. A11Y, changes in documented or “happy path” default behavior, changes in expected default layout)
• Major improvements that have been discussed/planned but don’t count as a large scope
  • Examples: Cleaning up the CSS variable relationships (including adding e.g. palette vars), Switching the priority relationship between default height and width for media chrome components, updates to “built in” svgs to clean up height/width & transparent space

Small Scope:

Rules:

• 1 core contributor reviewer/approval required
• Reviewer may escalate to a medium if they believe it's appropriate

Types:

• Most docs work (adding docs, fixing errors in docs, improving docs
• Most tests (unit, integration, or functional)
• Minor improvements
• Fixing bugs that don’t introduce regressions (to e.g. A11Y, changes in documented or “happy path” default behavior, changes in expected default layout)
• New components/features that conform closely to pre-existing components/paradigms and have been discussed/planned
  • Examples: Airplay button, a new “container component” for common “middle-chrome” use case styling

[cjpillsbury]

My highest concerns with the current proposal:

• requiring a full "consensus" for large PRs (fine with trying it out and hoping it doesn't bottleneck us)
• not having clear enough PR feedback still resulting in unnecessary slow down
• lack of clear ownership and priorities without further discussion

[gkatsev]

We should clarify who should be merging (reviewer* or submitter?)

* I'd assume the reviewer.

Is this for PRs coming from the core team? Or any PR? If so, which reviewer should be merging things? To me, for the core team, submitter makes sense, because theoretically, they should be able to judge when/if enough review has happened. Particularly for PRs that are large enough to have a minimum time window.

Large Scope/Size:

Rules:

* Requires consensus by core contributors, to be discussed at weekly meeting for next steps

I wonder if here, there should be a distinction between just a large PR and a change that requires a massive re-achitecture. Specifically, for the latter, it may be better to have an RFC-type process that requires agreement from the core team before work begins. Theoretically, the RFC will require a lot less work to do to minimize potential wasted effort.
Then, the actual large PRs may not necessarily require approval from all of core and potentially alleviate the concern that full consensus may create a bottleneck.

• Try using language such as "fine for now", "not a blocker", "can merge but needs a an additional fast follow PR", and consider adding yourself as a reviewer and rejecting the PR if you do not believe it should be merged.

https://conventionalcomments.org/

We should clarify our release process, cadence, and ownership of said process

In addition, we should also codify things like git commit message format and whether things are merged as squash commits and what not.

[cjpillsbury]

To me, for the core team, submitter makes sense, because theoretically, they should be able to judge when/if enough review has happened. Particularly for PRs that are large enough to have a minimum time window.

Sounds 👌 to me. To make clear:

• If the contribution/PR is issued by a core contributor, it is their responsibility to merge. If the contribution/PR is issued by someone other than a core contributor, it is the responsibility of the reviewer (or one of the reviewers, if there are multiple) to merge.

[cjpillsbury]

I wonder if here, there should be a distinction between just a large PR and a change that requires a massive re-achitecture. Specifically, for the latter, it may be better to have an RFC-type process that requires agreement from the core team before work begins. Theoretically, the RFC will require a lot less work to do to minimize potential wasted effort.

Whether or not we introduce an RFC-type process, I believe that "large" should be determined by the impact/type of changes and not by the number of lines of code changed in a given PR.

I'm also cool with introducing an "RFC-type process", but want to anticipate PRs that come in that "end up being considered a large" or "get escalated to a large". Maybe that means the immediate followup is drafting an RFC? If so, my only concern is now formalizing the RFC process and ensuring we don't simply end up with RFCs that drag on indefinitely (that has happened on media-chrome for several things thus far, which is why I'm calling it out). If we decide as a group that RFCs are the way to go, I believe we need to pin these down as well:

• We should clarify when and how issues/discussions are triaged, resolved, closed, and who should be involved (try to use same scope categories for who?)
  ...
• We should have better alignment on priorities (active & known needs for ongoing efforts that employ media chrome, such as those from mux-elements and its users, external community users’ requests, ideas from core contributors and potentially adjacent)

[cjpillsbury]

https://conventionalcomments.org/
...
In addition, we should also codify things like git commit message format and whether things are merged as squash commits and what not.

I'm 100% on board for all of this, but I didn't want to be overly presumptuous or introduce "too much process" up front (defer to others) (though I think the squash vs. rebase etc. is reasonable regardless of folks' opinions on the others)

[gkatsev]

Could say something like "take a look at conventional comments for inspiration" rather than "use conventional comments"

[heff]

Thanks for writing this up, Christian. Here's some initial notes, and then I need to dig into the specific details. Once we have some high-level alignment it's probably worth breaking this up into different topics.

• To your point on impact, I think we shouldn't use "Large, medium, small" because of the tendency to link that to code size. I'd propose "Major, minor, patch" to line up with API impact, and see how that goes. For myself, I know I should prioritize my own time that way. I'm ok being a blocking reviewer on Major changes, for now, and I don't want to be a blocker on patches. I'd include doc changes in Patch, or another term below that.
• On the topic of large changes, we need a "one change per PR" rule". PRs can quickly become Large the more we try to do in them, and the time it takes to review and approve a PR grows exponentially with the number of changes. A "change" is obviously open to interpretation, but I think the guiding principle is important for review velocity. An example that comes to mind is converting the project to Typescript (not that I'm pushing for that). Converting the whole project to Typescript is still one change in my mind, and if that's all we do in the PR, it shouldn't take a ton of effort to review and test, but a temptation is often to add on little refactors and fixes in the process, which ultimately makes it take longer to review than if those fixes were brought in later and independently.
  • An RFC-like process is an interesting idea. I don't feel the need for the formality that usually comes with that, but the intentionally in communicating and aligning on the goals of the change upfront I think is really important for avoiding wasted effort. It sounds like clarity in the timing around a proposal would also be valuable.
• I like the idea of everyone on the open elements team being a "core contributor". There should still be some onboarding and learning, but I think that's ultimately better than being super selective. In reality, I think we should always be conscious if there's a specific person that should review a section. e.g. I can call out PRs that Christian should review because he wrote the core logic. I think we can do that manually for a while and not get too formal about it.

[cjpillsbury]

I'd propose "Major, minor, patch" to line up with API impact, and see how that goes.

This is great. I vote we run with this until it doesn't work.

An RFC-like process is an interesting idea. I don't feel the need for the formality that usually comes with that, but the intentionally in communicating and aligning on the goals of the change upfront I think is really important for avoiding wasted effort. It sounds like clarity in the timing around a proposal would also be valuable.

Agree with all of this.

In reality, I think we should always be conscious if there's a specific person that should review a section. e.g. I can call out PRs that Christian should review because he wrote the core logic.

I get this impulse, but I think we should balance that value with the value of shared ownership and the implicit knowledge distribution that comes along with it. I'm hoping we can establish sufficient trust across the core team to assign, reassign, and/or add reviewers appropriately while also establishing sufficient trust across the core team for any team member to be the primary reviewer for the majority of potential contributions/PRs.