Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Policy for reverting #187

Open
foolip opened this issue Mar 12, 2024 · 9 comments
Open

Policy for reverting #187

foolip opened this issue Mar 12, 2024 · 9 comments

Comments

@foolip
Copy link
Member

foolip commented Mar 12, 2024

In #184 @jgraham reverted an RFC that was accepted according to our documented process, because "In the case of no substantive disagreement the RFC is considered accepted after 1 week."

There had been out-of-band communication and a commitment to review that I was not aware of, but that is not part of our process, and not all wpt core team members are in those meetings. Any wpt core team member should be able to look at an RFC and merge it if there has been no feedback in 1 weeks, or 2 weeks if an extension was requested.

I think our policy should be, and implicitly already is, that RFC cannot have substantial changes, including their removal, without a new RFC.

In this instance, I think that expedient post-merge review and sending PRs with suggested changes on top of what was already landed would have been the way to go about this. Those changes might have required to be an RFC if not trivial, but not necessarily for questions and clarification.

On the process itself, the timeout is and important to allow progress even when some WPT core members aren't able to review. Notably absent from the process is any explicit or implicit requirement that all browser engines should chime in on every RFC.

@jgraham what's your take on this, how can we improve going forward?

@gsnedders
Copy link
Member

I think we need to reconsider the whole process as documented, because it's not the process we've practically been following.

I think looking at the revert question in isolation isn't really worthwhile; the underlying problem here is just that we don't document our process.

@foolip
Copy link
Member Author

foolip commented Mar 12, 2024

I've referred to the written process every time there's been a case of no feedback that I wanted to merge. What is the process we've been doing in practice if it's not the documented one?

@jgraham
Copy link
Contributor

jgraham commented Mar 12, 2024

In practice I don't recall anyone previously trying to merge an RFC without any kind of positive signals before. However there have been a lot of RFCs that went a week without feedback and later got substantive comments that resulted in us changing the approach.

I'd used the week deadline as "after getting something that looked like rough agreement on the approach, how long do you have to wait in case of further objections".

So I agree that our de-facto process has differed from the documented one. Previously that hadn't really caused any issues but in this case it did, so I agree with @gsnedders that we need to do the work to update it to something closer to what we've been doing in practice, and which everyone is happy with.

@foolip
Copy link
Member Author

foolip commented Mar 12, 2024

In #101 and #177 I was the only reviewer, and there are many more that weren't reviewed by anyone at Apple or Mozilla, usually by @Ms2ger.

I'd be happy to see updates to the process suggested via RFCs, but I do think that it's important that the absence of feedback leads to accepting RFCs, so that as long as a single wpt core team member is around to merge, progress is possible.

P.S. I can understand if #182 looks like a merge without review, but I did review it carefully in the form of a doc before the RFC was created.

@gsnedders
Copy link
Member

I noted that #168 (comment) had, per documented policy, passed without comment. Likewise #181 passed with no substantial dissent (only one point of feedback on a minor point).

In practice our policy has been to wait till at least the next infrastructure meeting; I agree in #182 we had somewhat failed insofar as there should have at least some pointer to the meeting minutes.

@jgraham
Copy link
Contributor

jgraham commented Mar 12, 2024

Cases like #101 are basically why we said that it's OK to add testdriver endpoints as long as they have backing in specification text.

Both #101 and #177 are cases where (afaict) Mozilla doesn't even have a standards position on those technologies. That does make it hard to give positive feedback; if the testing approach just looks wrong then it might be possible to say "not like that", but it's generally hard to evaluate details, and there's a small, but non-zero, risk that approving a test API will be misunderstood as support for the feature as a whole. That doesn't mean I want to force people to write implementation-specific tests until they have multi-vendor buy in for the technology, but I think similar considerations apply i.e. if you're adding testing API for specs that don't yet have multi-vendor support it's good to assume that both the spec and the testing API might need future revisions. Also, in both cases, the proposal was rather small and self contained.

At least in my mind those cases are materially different to something that adds significant new implementation complexity and which would be very difficult to fix if we end up with a suboptimal design.

I also agree that we should have commented in #182 to establish that it hadn't been forgotten, but that it needed more time for review.

@foolip
Copy link
Member Author

foolip commented Mar 12, 2024

Great, it sounds like one point of agreement is to comment on RFCs if they're discussed in meetings. I'm pretty sure I've been in meetings discussing RFCs without doing this, so it's a note to self as well.

The most important thing here is really that we can begin to use WebDriver BiDi for features that require it, so I don't wish for a process discussion to distract from that goal.

@past
Copy link
Member

past commented Mar 14, 2024

Both #101 and #177 are cases where (afaict) Mozilla doesn't even have a standards position on those technologies. That does make it hard to give positive feedback; if the testing approach just looks wrong then it might be possible to say "not like that", but it's generally hard to evaluate details, and there's a small, but non-zero, risk that approving a test API will be misunderstood as support for the feature as a whole.

I don't understand this particular concern: what is the audience here that might misunderstand Mozilla's standards position? Given that it has been clearly stated that the standards-positions repo is the only place that contains official positions, and given that it has even been incorporated in the Blink launch process, I don't see who in the community may be holding such misguided assumptions.

@jgraham
Copy link
Contributor

jgraham commented Mar 14, 2024

"People take things other than standards positions as endorsement" has certainly been a real problem historically. If Blink in particular has improved its process here so that this doesn't happen in that context going forward that's certainly welcome, but even if the changes eliminate the problem it still takes time for people's perceptions to change.

I will also say that I don't think removing that risk changes the overall story here much. It's still hard to give feedback on a proposal that doesn't have multi-vendor support, and to some extent the decision to allow WebDriver endpoints to be directly reflected in testdriver without an RFC is designed to allow people to go ahead and write tests for such APIs without running into roadblocks, as long as they're sticking to recognised paths / best practices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants