From 11b86933c1e003d3f016d401b0ee8845ae2e26df Mon Sep 17 00:00:00 2001 From: Devlin Cronin Date: Wed, 27 Mar 2024 16:12:22 -0700 Subject: [PATCH 1/2] Add guidance on the review expectations of proposals As we discussed at the in-person WECG meet-up, we should add expectations around review practices. This PR adds documentation for those practices. --- proposals/proposal_process.md | 55 +++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/proposals/proposal_process.md b/proposals/proposal_process.md index 25bbebd3..edcec42d 100644 --- a/proposals/proposal_process.md +++ b/proposals/proposal_process.md @@ -184,3 +184,58 @@ sponsored by a browser vendor) and a reviewing browser vendor are unable to reach consensus (with regard to either the API's concept or an individual aspect of it). In this scenario, the reviewing vendor must choose a stance (which will very likely be "Opposed") to allow the API to move forward. + +## Review Process + +Reviewing new proposals is one of the most important and impactful roles of the +Web Extensions Community Group. This ensures browsers have the opportunity to +align on shared functionality when desirable for new functionality and helps +prevent drift between the API surfaces. As such, it is critical to ensure +timely reviews. + +### Authors + +Authors should request a review from at least one representative from each +browser engine. + +When responding to feedback from reviewers, authors should resolve conversations +if they feel they have been fully addressed. This makes it easy to see if there +are outstanding comments. Authors can (and should) leave comments open if they +would like further input from reviewers (e.g., "Is this what you were looking +for"). + +If reviewers haven't responded within a few business days, authors should ping +the relevant reviewers. + +### Reviewers + +Reviewers should strive to respond to a review within one or two business days; +this is not a strict SLO, but is a target -- we understand that this may not +always be possible (e.g. if multiple reviewers are at a summit). + +Reviewers should indicate if they are supportive of a proposal by approving it. + +Reviewers should resolve any comment threads they began if they feel the comment +has been sufficiently addressed. This makes it easy to see if there are +outstanding comments. + +Reviewers should *not* block on other reviewers in most cases. A reviewer's +approval is only indicative of their own approval, not of the readiness of a +proposal to merge. This prevents a deadlock where reviewer A waits on reviewer +B and reviewer B waits on reviewer A. + +Reviewers should be clear whether they have blocking requests by indicating +they "Request changes" on the review. + +Reviewers may approve a proposal "with nits" if they trust the author to update +the proposal before (or shortly after) it's merged. As an example, a typo or +slight rephrasing need not block a proposal from being merged. + +### Merge Managers + +A proposal should be merged only when no reviewers have requested additional +changes. Just because a single reviewer has approved a proposal does *not* +mean it should be merged. + +When all reviewers have approved (or abstained by not leaving any comments), +a proposal should be merged. From d0f5a685f4c215d3a8d327adff2f6d8ade6a61be Mon Sep 17 00:00:00 2001 From: Devlin Cronin Date: Fri, 29 Mar 2024 11:23:53 -0700 Subject: [PATCH 2/2] Address Simeon's comments --- proposals/proposal_process.md | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/proposals/proposal_process.md b/proposals/proposal_process.md index edcec42d..05ac212b 100644 --- a/proposals/proposal_process.md +++ b/proposals/proposal_process.md @@ -189,9 +189,8 @@ aspect of it). In this scenario, the reviewing vendor must choose a stance Reviewing new proposals is one of the most important and impactful roles of the Web Extensions Community Group. This ensures browsers have the opportunity to -align on shared functionality when desirable for new functionality and helps -prevent drift between the API surfaces. As such, it is critical to ensure -timely reviews. +align on shared functionality when desirable and helps prevent drift between the +API surfaces. As such, it is critical to ensure timely reviews. ### Authors @@ -209,16 +208,19 @@ the relevant reviewers. ### Reviewers -Reviewers should strive to respond to a review within one or two business days; -this is not a strict SLO, but is a target -- we understand that this may not -always be possible (e.g. if multiple reviewers are at a summit). - -Reviewers should indicate if they are supportive of a proposal by approving it. +Reviewers should strive to respond to a review within one or two business days. +This is a target, not strict SLO; we understand that this may not always be +possible (e.g. if multiple reviewers are at a summit). Reviewers should resolve any comment threads they began if they feel the comment has been sufficiently addressed. This makes it easy to see if there are outstanding comments. +Reviewers should indicate they, individually, are happy with a proposal and +think it is in good shape to merge by approving it. (See +[Browser Vendor Support](#browser-vendor-support) for details on indicating +support or opposition.) + Reviewers should *not* block on other reviewers in most cases. A reviewer's approval is only indicative of their own approval, not of the readiness of a proposal to merge. This prevents a deadlock where reviewer A waits on reviewer