Whenever we need to call from the glue into the support library to call a new API we need to check the version of the support library. We should store this version in a way that doesn't make different support library versions used by the same app clash. We should probably store the support library version in the WebViewProvider so that it's interpreted per-WebView - e.g. storing it in the WebViewFactoryProvider would mean we stored only one value per app/process, which is not OK if an app uses several versions of the library.

If we have any static callbacks in the WebView APK we will have to pass the support library version to the WebView APK for each such callback. I would assume we have none or very few such callbacks though.

Oooh, as it stands right now our support library implementation passes the set of features supported to the webview apk code at start-up - so that there's one set of features per process. Under this model, if there are several different versions of the support library (statically compiled into the app), the webview apk code might call methods that are not available in some of those support library versions.

So if we want to support the use of several versions of the support library in the same process we'll have to pass feature flags in a more fine-tuned way.

Torne/Toby do you think this is something we should be supporting? (using several different versions of the support library in the same app / process).

I think we probably do need to support this to make it at all easy for app developers to use. If the support library catches on then it's very likely that ad SDKs will embed it, and that'll be very restrictive if only one version is usable (quite a few apps embed multiple ad SDKs, even, which may have different versions of the support library entirely outside of the app developer's control).

I'm not sure how we'd even prevent people from getting it wrong if we didn't support this.

So, yeah, it seems like it'd be a good idea to make sure that we minimise the amount of state/feature flags/whatever that refer back to the support library from the implementation.

Maybe even having feature flags in that direction may not be the best idea? Can we just detect callback presence dynamically somehow without it being crappy performance, so that we just call things if they're there and not if they aren't?

> If we have any static callbacks in the WebView APK

What are static callbacks?

> Can we just detect callback presence dynamically somehow without it being crappy performance, so that we just call things if they're there and not if they aren't?

Yeah, that's possible. We would get NoSuchMethodException here [1]. Conceivably, we could catch the RuntimeException, check if it was caused by NoSuchMethod, and fallback to the default behavior for the callback. There's no extra cost if the method is there. If the method isn't there, then we still pay the cost of method detection, which is probably acceptable cost since we would pay that anyway if the method *were* there.

[1] https://cs.chromium.org/chromium/src/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/util/BoundaryInterfaceReflectionUtil.java?q=dupemethod+f:android_webview&sq=package:chromium&l=60

I think what I meant with 'static callbacks' was static methods defined in the support library, used by the WebView APK code, but because of the design of the boundary interfaces such methods won't exist (whenever the webview apk code calls into the support library that call will happen through a boundary interface, whose methods are always non-static).


If we go with the approach of using reflection to check whether a callback exists I think we should do so explicitly - i.e. at least pass some parameter to createInvocationHandlerFor() that explicitly says that the method being called might not exist - since every method guarded by a feature flag should throw an exception if the method is being called but doesn't exist.


One slightly annoying part of this (IMO) would be that if someone decides to create a WebViewClientCompat for support library version 100, and then happens to pass that into support library version 28 (not sure how this could happen - but I thought I'd raise the question anyway), then if we have a webview apk supporting version 100 the webview apk code would be calling into all the new callbacks from version 100, even if the support library only supports version 28.

I think the alternative to using reflection to look up which callbacks to use would be to create a feature-flag boundary interface:

FeatureFlagHolderBoundaryInterface{
   String[] getFeatures();
}

that all callback-objects implement (e.g. VisualStateCallbackCompatAdapter / WebViewClientCompatAdapter) so that we can check feature flags on a per-object basis rather than globally for the entire support library.


Do you (Nate and Torne) wanna have a VC about this tomorrow (Tuesday)? The support library deadlines are approaching quickly (early next week).

Sure, can talk about this tomorrow if you want.

Yup, feel free to set something up on my calendar.

> If we go with the approach of using reflection to check whether a callback exists I think we should do so explicitly

On second thought, I may have spoken too soon. I'm not sure if that idea works nicely, because it means we can't tell if a feature is supported until we branch to the support lib glue (at which point, it's inconvenient to get back to the webkit glue).

I wrote a short doc on callback feature detection [1] to compare a few potential solutions. Feel free to comment.

> I think the alternative to using reflection to look up which callbacks to use would be to create a feature-flag boundary interface

Yes, this could work nicely (I hadn't thought about other callback objects). I had the same idea for WebViewClient only, but we can definitely generalize to an interface as you've suggested. I think this is the best option.

[1] https://docs.google.com/document/d/1_ZOEr4hechL7N-r_-6WWLW-lZu-cBJPddpofaNmjw8A/edit?usp=sharing

Awesome, thanks for putting together that doc Nate, that's really helpful (and saved me a bunch of time this morning!).


An additional thought: we can't have the webview apk code store any kind of global state related to the support-library-side, the only such state I can think of right now is SupportLibraryInfo (which currently only holds feature flags):

https://cs.corp.google.com/android/frameworks/support/webkit/src/main/java/androidx/webkit/internal/SupportLibraryInfo.java


I.e. SupportLibWebViewFactoryProvider cannot depend on anything from the support library side.
https://cs.chromium.org/chromium/src/android_webview/support_library/java/src/org/chromium/support_lib_glue/SupportLibWebViewChromiumFactory.java

I can't think of any state we would need to store there - but it would be nice to have a mental image of why this is the case (i.e. why it's not a problem that we cannot store any global state related to the support-library-side).

I added a calendar event for later today, thanks guys!

Gustav and I decided that callable APIs will probably work as-is with multiple support lib versions, so this bug only impacts callbacks (Gustav filed http://b/77539415 to track removing the global state we currently have).

If we're all on board with the doc from comment #8, then I'll start working on that.

Changing the title, because this isn't really about adding a test (and it's not obvious how we could realistically test this).

I started adding feature flags for the different support library APIs now and one thought that arised:
do we want public (app/developer-facing) feature flags for callbacks (like ServiceWorkerClientCompat.shouldInterceptRequest)?
I imagine we do want a public feature-flag for each callbacks to let apps know when a specific callback will be working, e.g. we might choose to replace a certain callback in the future and then it would be nice to let apps know that it's gone - but on the other hand maybe that\s not a case we need to support? I.e. whenever we remove a callback we will probably replace it with another one - so our glue layer code would call into just one of those callbacks depending on what version of the support library is being used - so the developer would always get the correct callback anyway..

> whenever we remove a callback we will probably replace it with another one

This is not the only use-case. Apps may need to know when a particular callback is not supported by a legacy APK. The example in my mind is onSafeBrowsingHit: suppose an app has such a strong requirement for custom interstitials that they would rather disable Safe Browsing entirely on devices which don't support the required callback. In such a case, the app would need feature flags.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/986597eeb3fe7b0cff59d50f8f9e42e3c9a4b38c

commit 986597eeb3fe7b0cff59d50f8f9e42e3c9a4b38c
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Apr 05 17:16:45 2018

AW: add boundary interface for callback feature detection

This adds the FeatureFlagHolderBoundaryInterface. Support-lib-created
callback objects will implement this to indicate, on a per-object
granularity, which features they support.

Doc: http://go/wv-callback-feature-detection

Bug:  752521 
Test: N/A
Change-Id: Iac86615052069d091a0db66f41bc9c852498815f
Reviewed-on: https://chromium-review.googlesource.com/997021
Reviewed-by: Gustav Sennton <gsennton@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548462}
[modify] https://crrev.com/986597eeb3fe7b0cff59d50f8f9e42e3c9a4b38c/android_webview/support_library/boundary_interfaces/BUILD.gn
[add] https://crrev.com/986597eeb3fe7b0cff59d50f8f9e42e3c9a4b38c/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/FeatureFlagHolderBoundaryInterface.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4a2e1e9c74cde018bbb232f48e56c3df6b66ff5

commit b4a2e1e9c74cde018bbb232f48e56c3df6b66ff5
Author: Gustav Sennton <gsennton@google.com>
Date: Thu Apr 05 21:58:51 2018

[Android WebView] remove global references to the support library

To support several versions of the WebView Support Library within the
same app/process we need to ensure the support library glue doesn't hold
any global state referencing the (app-side of the) support library.

With this CL we remove the only current global reference to the support
library: SupportLibraryInfo. The only information kept within that
reference was a list of feature flags. Those feature flags will now live
in individual callback objects instead, like WebViewClientCompat.

Corresponding Android Support Library CL:
https://googleplex-android-review.git.corp.google.com/#/c/platform/frameworks/support/+/3849471/

Bug:  752521 
Change-Id: I9f8c67784fff9900f99b7c12f09f8cb1a39e64ea
Reviewed-on: https://chromium-review.googlesource.com/995212
Commit-Queue: Gustav Sennton <gsennton@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548576}
[modify] https://crrev.com/b4a2e1e9c74cde018bbb232f48e56c3df6b66ff5/android_webview/support_library/boundary_interfaces/BUILD.gn
[delete] https://crrev.com/26561f43c8598bbc921e17b8ef93f0c0183106d3/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/SupportLibraryInfoBoundaryInterface.java
[modify] https://crrev.com/b4a2e1e9c74cde018bbb232f48e56c3df6b66ff5/android_webview/support_library/java/src/org/chromium/support_lib_glue/SupportLibReflectionUtil.java
[modify] https://crrev.com/b4a2e1e9c74cde018bbb232f48e56c3df6b66ff5/android_webview/support_library/java/src/org/chromium/support_lib_glue/SupportLibWebViewChromiumFactory.java

re #14: I'm looking at the callback case now for ServiceWorkerClientCompat.shouldInterceptRequest. Just to clarify:

In the case of a 'normal call' (not a callback) we have the following setup:
1. WebViewFeature.isFeatureSupported checks whether the feature is supported by either the framework or webview
2. Features.FEATURE_NAME (in the boundary-interface folder) is declared in SupportLibWebViewChromiumFactory by WebView APKs that do support the feature. That feature is used to perform the isSupportedByWebView check from within WebViewFeature.isFeatureSupported.
https://cs.chromium.org/chromium/src/android_webview/support_library/java/src/org/chromium/support_lib_glue/SupportLibWebViewChromiumFactory.java

In the case of a callback the above two configurations still hold, but we also have a third configuration:
3. The callback-adapter on the support library side (e.g. ServiceWorkerClientAdapter) implements FeatureFlagHolderBoundaryInterface, to declare that it supports the feature Features.FEATURE_NAME (e.g. Features.SERVICE_WORKER_SHOULD_INTERCEPT_REQUEST), and this flag is checked in the support library glue (e.g. in SupportLibServiceWorkerClientAdapter) before calling across the boundary, to ensure the callback is future-proof (i.e. we can remove the callback and the corresponding feature-flag, and then the WebView APK won't call into that callback anymore).


In the future if we ever decide to remove/replace a callback we will have to 
WebView:
a. remove the feature from SupportLibWebViewChromiumFactory.

Support Library:
A. remove the feature from ServiceWorkerClientAdapter
B. somehow declare that the feature isn't supported from WebViewFeature.isFeatureSupported (currently that method just checks OS level and whether the feature is supported by the WebView APK, not whether it has been removed from the support library itself).



Does this sound correct / have I missed something?

Basically correct. Step "B" is a bit vague, but we would actually implement that like so:

Support Library:
...
B. Edit WebViewFeature.java & WebViewFeatureInternal.java, remove the feature name (apps shouldn't check for features which no longer exist in supp lib)

Right, we can remove the feature name on the support library side - we'd still wanna keep it in the boundary_interface folder (Features.java) to avoid accidentally adding a new feature with the same name (in the future).

I think this has been completely implemented by 71d3b4c3b22d383840673c9b0ec562c679fabb6a.

Bulk edit: marking stale 'fixed' bugs as 'verified' since they don't need verification at this point.