Simplifying API review process for WebView WebExposed changes

[Ashley Newson]

Hello API owners,

We, the Android WebView team, are working on fixing and re-enabling WebView's webexposed tests. As part of this effort we're trying to improve alignment with the webexposed web_tests in Blink (//third_party/blink/web_tests/[virtual/stable/]webexposed).

Historically, changes to WebView's webexposed expectations have required approval from an //android_webview/ owner. We're looking to change this such that Blink API OWNERs will own the (stable) WebView expectations (with WebView ownership being removed/noparented).

Blink API owners already own and review the corresponding virtual/stable/webexposed/ Blink expectations and are more in tune with web-visible feature development, so they already have the context and may be in a stronger position to compare any discrepancies. It should also improve velocity by not requiring additional reviewers.

Design: go/webview-webexposed-revamp

Do Blink API owners have any comments or questions about this proposed change? Would you require any additional information from the WebView team in order to guide review decisions?

[Mike Taylor]

I think this SGTM. 

One thing to note: if an API that was previously approved for other platforms was to be enabled for WebView, we don't require an I2S (but PSAs are cool, and I'd recommend them). But I don't see an issue with reviewing the CLs in such cases.

--
You received this message because you are subscribed to the Google Groups "blink-api-owners-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-api-owners-d...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-api-owners-discuss/e9a4b1aa-623f-463c-9c84-3c24d8719e56n%40chromium.org.

[Chris Harrelson]

Sounds good to me too.

On Tue, Dec 10, 2024 at 8:11 AM Mike Taylor <mike...@chromium.org> wrote:

One thing to note: if an API that was previously approved for other platforms was to be enabled for WebView, we don't require an I2S (but PSAs are cool, and I'd recommend them). But I don't see an issue with reviewing the CLs in such cases.

Mike, doesn't this contradict what we're discussing on the other blink-api-owners thread about this question?

 

On 12/4/24 11:12 AM, 'Ashley Newson' via blink-api-owners-discuss wrote:

Hello API owners,

We, the Android WebView team, are working on fixing and re-enabling WebView's webexposed tests. As part of this effort we're trying to improve alignment with the webexposed web_tests in Blink (//third_party/blink/web_tests/[virtual/stable/]webexposed).

Historically, changes to WebView's webexposed expectations have required approval from an //android_webview/ owner. We're looking to change this such that Blink API OWNERs will own the (stable) WebView expectations (with WebView ownership being removed/noparented).

Blink API owners already own and review the corresponding virtual/stable/webexposed/ Blink expectations and are more in tune with web-visible feature development, so they already have the context and may be in a stronger position to compare any discrepancies. It should also improve velocity by not requiring additional reviewers.

Design: go/webview-webexposed-revamp

Do Blink API owners have any comments or questions about this proposed change? Would you require any additional information from the WebView team in order to guide review decisions?

--
You received this message because you are subscribed to the Google Groups "blink-api-owners-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-api-owners-d...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-api-owners-discuss/e9a4b1aa-623f-463c-9c84-3c24d8719e56n%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "blink-api-owners-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-api-owners-d...@chromium.org.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-api-owners-discuss/535eac2e-19e1-470e-84cd-cd833e9c9ce6%40chromium.org.

[Mike Taylor]

On 12/10/24 11:19 AM, Chris Harrelson wrote:

Sounds good to me too.

On Tue, Dec 10, 2024 at 8:11 AM Mike Taylor <mike...@chromium.org> wrote:

One thing to note: if an API that was previously approved for other platforms was to be enabled for WebView, we don't require an I2S (but PSAs are cool, and I'd recommend them). But I don't see an issue with reviewing the CLs in such cases.

Mike, doesn't this contradict what we're discussing on the other blink-api-owners thread about this question?

I don't think so - maybe "(but PSAs are cool, and I'd recommend them)" should have been written as "a PSA should be sent instead, or an update to the original I2S thread). 

Either way, someone has to approve the ../webexposed CL, right?

[Chris Harrelson]

On Tue, Dec 10, 2024 at 8:25 AM Mike Taylor <mike...@chromium.org> wrote:

On 12/10/24 11:19 AM, Chris Harrelson wrote:

Sounds good to me too.

On Tue, Dec 10, 2024 at 8:11 AM Mike Taylor <mike...@chromium.org> wrote:

One thing to note: if an API that was previously approved for other platforms was to be enabled for WebView, we don't require an I2S (but PSAs are cool, and I'd recommend them). But I don't see an issue with reviewing the CLs in such cases.

Mike, doesn't this contradict what we're discussing on the other blink-api-owners thread about this question?

I don't think so - maybe "(but PSAs are cool, and I'd recommend them)" should have been written as "a PSA should be sent instead, or an update to the original I2S thread). 

+1, please send a PSA for all changes to platform-exposed APIs. And if there is some kind of caveat or other risk about webview, please use an I2S.

Either way, someone has to approve the ../webexposed CL, right?

Totally agree on the ../webexposed part. 

[Rick Byers]

I also just reviewed this doc and it SGTM also.

IIRC when the topic of WebView API surface area came up years ago, we wanted to reinforce the assumption that new APIs should ship on all platforms including WebView and that gave rise to the "not-webview-exposed.txt" path and require developers to only do extra webview-specific work in the unusual case of APIs which were different on WebView. If I understand correctly, the new system sort of reverses that  - there's going to be a little extra work required for developers to acknowledge their new API is expected on WebView in the common case, and no warning or extra work in the rarer case of shipping a new API which is NOT exposed to WebView. @Peter Beverloo mentioned to me today that we had some examples of accidentally shipping APIs to WebView which were non-functional (like FedCM), which I agree is really bad. So I'm convinced it's worth asking for explicit confirmation when adding an API to WebView. Worst case if we accidentally fail to ship some APIs in WebView I think that's less damaging than shipping broken APIs, so let's just accept that tradeoff and re-evaluate at some point in the future.

Thanks for investing in more rigour here!

   Rick

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-api-owners-discuss/CA%2BN6QZssRh1pAicwXNBnp%3DY2Rb2TJgcsmKaFKQrcmeFUsS99KQ%40mail.gmail.com.

[Mason Freed]

On Tue, Dec 10, 2024 at 1:07 PM Rick Byers <rby...@chromium.org> wrote:

I also just reviewed this doc and it SGTM also.

IIRC when the topic of WebView API surface area came up years ago, we wanted to reinforce the assumption that new APIs should ship on all platforms including WebView and that gave rise to the "not-webview-exposed.txt" path and require developers to only do extra webview-specific work in the unusual case of APIs which were different on WebView. If I understand correctly, the new system sort of reverses that  - there's going to be a little extra work required for developers to acknowledge their new API is expected on WebView in the common case, and no warning or extra work in the rarer case of shipping a new API which is NOT exposed to WebView.

I also reviewed the docs, and I could have read it incorrectly, but I thought the plan was to have the WebView-specific tests look at the "regular" blink expectations files, and if the API is shipped on all platforms including WebView (the hopefully "normal" case) then no additional files would need to change in the WebView-specific directories. Only if an API is shipped *not* on WebView, or *only* on WebView would there be an additional expectation file that needs to be individually approved. Ashley, did I read that wrong? Because I do agree with Rick's sentiment that the easy path should be to ship on all platforms.

Thanks,

Mason

[Peter Beverloo]

I definitely agree with the sentiment that the easy path should be to ship on all platforms, however, Ashley's work has highlighted that we've consistently been exposing features on WebView by accident. That suggests to me that a guardrail should be considered, because in most cases the I2S messages were comprehensive.

The example that I shared with Rick is that WebView incorrectly exposes support for FedCM. Other (non-exhaustive) examples include the Contact API, Ink API, Compute Pressure API, gravity sensors and picture-in-picture. Looking at Ashley's latest rebase—since last April—at least three more examples are seen: the AICreateMonitor and ProtectedAudience interfaces, as well as the document.browsingTopics method part of the Topics API.

None of this would've been caught by the "not-webview-exposed.txt" expectations.

Thanks,

Peter

[Ashley Newson]

There were a few iterations of the design since were first looped in, Mason. Originally, there were still ideas floating around of not-webview-exposed and only-webview-exposed files (similar to the original implementation), but we eventually decided that these are not an ideal approach. The benefits didn't outweigh the added complexity for WebView owners, Blink owners, or developers needing to update the expectations with their changes.

If a feature change only notices WebView at the stage of updating stable API expectations, things have already been caught too late. But accidentally exposing a (broken) feature is far worse than not exposing a feature, or exposing it as early as Chrome.

With our finalized design, we move much closer to mirroring what Blink does, but using WebView instead of Content Shell. Feature changes that equally affect Chrome, Content Shell, WebView (and any other Blink-based platform) should have the same expectation changes/diffs for both Blink and WebView.

[Ashley Newson]

It sounds like there are 3 SGTMs!

I have drafted a PSA to send to blink-dev, included below. If there's no objections or suggestions, I'll send it at some point on Monday (2024-12-16) and enable the tests on CQ shortly after. I'll enable the tests on CQ without requiring owner approvals for a few trial months. (As the existing tests have been outright disabled for a while, this isn't relaxing any existing restrictions.)

### BEGIN PSA ###
Subject: PSA: New WebView webexposed tests to run on CQ

TL;DR: If you make web-visible changes, you may need to update Android WebView's webexposed expectations as you do for Blink.

We are re-introducing webexposed tests for Android WebView that align closer to Blink:

• WebView will have both stable and non-stable expectations.
• Expectations will be checked in the commit queue.
• Unstable expectation changes will not require owner approval.
• Starting in 2025Q1, stable expectation changes will require Blink API Owner approval. Before then, whilst we evaluate running the tests in CQ, we will not require owner approval.

If any changes to a WebView expectation file are required, one or both of the following tests will fail and provide you with patches to apply.

- org.chromium.android_webview.test.WebExposedTest#testGlobalInterfaceListingStable
- org.chromium.android_webview.test.WebExposedTest#testGlobalInterfaceListingUnstable
#### END PSA ####

[Rick Byers]

On Wed, Dec 11, 2024 at 1:11 PM Ashley Newson <ashley...@google.com> wrote:

It sounds like there are 3 SGTMs!

I have drafted a PSA to send to blink-dev, included below. If there's no objections or suggestions, I'll send it at some point on Monday (2024-12-16) and enable the tests on CQ shortly after. I'll enable the tests on CQ without requiring owner approvals for a few trial months. (As the existing tests have been outright disabled for a while, this isn't relaxing any existing restrictions.)

### BEGIN PSA ###
Subject: PSA: New WebView webexposed tests to run on CQ

TL;DR: If you make web-visible changes, you may need to update Android WebView's webexposed expectations as you do for Blink.

Nit: Maybe "other platforms" instead of "blink"? WebView is very much a supported platform in Blink :-)

We are re-introducing webexposed tests for Android WebView that align closer to Blink:

ditto 

• WebView will have both stable and non-stable expectations.
• Expectations will be checked in the commit queue.
• Unstable expectation changes will not require owner approval.
• Starting in 2025Q1, stable expectation changes will require Blink API Owner approval. Before then, whilst we evaluate running the tests in CQ, we will not require owner approval.

If any changes to a WebView expectation file are required, one or both of the following tests will fail and provide you with patches to apply.

- org.chromium.android_webview.test.WebExposedTest#testGlobalInterfaceListingStable
- org.chromium.android_webview.test.WebExposedTest#testGlobalInterfaceListingUnstable
#### END PSA ####

LGTM

[Mason Freed]

On Wed, Dec 11, 2024 at 7:58 AM Ashley Newson <ashley...@google.com> wrote:

There were a few iterations of the design since were first looped in, Mason. Originally, there were still ideas floating around of not-webview-exposed and only-webview-exposed files (similar to the original implementation), but we eventually decided that these are not an ideal approach. The benefits didn't outweigh the added complexity for WebView owners, Blink owners, or developers needing to update the expectations with their changes.

Oh ok, thanks for the additional info. It has indeed been a while since I reviewed, and it sounds like things changed. As long as a) non-stable changes don't require special owner approval, and b) the blink owners can simultaneously approve stable changes, I think I'm happy. Sounds like both of those are true!

Thanks for all of the work getting this working and aligned.

Thanks,

Mason

[Ashley Newson]

The WebView webexposed tests have been running in CI and CQ for a couple of months now without issue. All seems to be working as expected and the tests are already having a positive effect.

I'm happy on my end to transfer the ownership of the stable expectations over to Blink API owners, as per the plan in the PSA.

Are there any remaining comments or questions, now that relevant CLs have had a chance to popup in reviewers' queues?

[Chris Harrelson]

Hi Ashley

I agree, let's do the transfer to API owners. Can you send a CL that does that?

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-api-owners-discuss/e0682e8e-24a8-47d1-b031-ac4dfa04a984n%40chromium.org.