some features can never be ported back, so getSupportedSdkLevel doesn't seem to be general enough

so probably need both "can I use this feature" as well as throwing exceptions if an unsupported feature is called

Note that the WebView support library version will also have an impact on getSupportedSdkLevel. This probably doesn't matter much from the app perspective - they just won't be able to compile against APIs that are not there on earlier support library versions, but from the webview APK's point of view it matters because we might need to avoid calling from the support-library-glue into the support library from a method that doesn't exist in specific versions of the support library.

Bo, do you think it makes sense to just document the APIs that should be handled in a different way? (e.g. apis that won't be backported)

Maybe it does make more sense to have specific canIUseThisFeature() functionality since we might add some APIs in a support library version that doesn't represent the android version the original APIs were added in.. (e.g. if we introduce some new Android API in P, and then only add that API to the Q version of the support library).
An alternative could be to have a getSupportedLibraryLevel() or similar, that isn't directly connected to any SDK level.  

I think you just made a good case for canIUseThisFeature(), which I'd vote for.

Yeah, any ideas on how that API would look though?
Adding an extra method per existing method seems wrong, passing the signature of the method to canIUseThisFeature(MethodSignature) also feels awkward..?

method name should be good enough, right? we wouldn't want to overload methods in the support library, we'd just remove old versions?

Are there no cases where we might take different parameters? Similar to e.g loadUrl

If we're going to do canIUseThisFeature type stuff I'd suggest just using an enum of features, not basing it directly on methods.

@Torne that might be a bit annoying when we add single methods like shouldInterceptRequest, but otherwise it sounds reasonable.

I don't think it'd be a problem to have many of the "features" in that enum be individual methods - it's not really any different to using the method name (other than taking less space)..

Good point :)

Torne, are you suggesting we use these enums only publicly, or also internally?
I.e. would these enums only be used by a developer:

if (isFeatureAvailable(FooFeature)) {
    webview.foo();
}

or would we also use it internally to perform checks before calling between the support library and the webview apk?

I guess it makes sense to use this approach throughout rather than using API versions / library versions?



Separate concern: if we end up replacing one method signature with another (like we did with shouldOverrideUrlLoading), I assume we'll just add a new feature enum to check whether to use the new or the old method?

If we need to check things internally that way then we can use the same ones, yes.

It'd be cool if we could assign these enums to features in such a way that we could use them for API/feature removal as well (maybe that was part of your idea Torne, I just hadn't thought of that until now :)).

In case anyone would like to think about this more (given that I'm OOO next week):

what I'm thinking here is that we pass a list of enums/integers across the boundary between the support library and the webview APK.

So both sides provide an API for fetching the features supported:
List<int> getSupportLibraryFeatureList()
List<int> getWebViewApkFeatureList()

and then either side uses the feature list from the 'opponent' to determine whether to perform a certain call/callback.


If the WebView APK on the device is too old to support anything support library related then the (Android) support library side should just interpret this as if getWebViewApkFeatureList() is empty - not providing any features at all.



Note that we have to deal with several types of feature detection here:
1. the features the (Android) support library supports at build-time
2. the features the current WebView APK supports (checked at run-time)
3. the features the current framework supports (checked at run-time)
4. the features the (Android) support library supports at run-time - this is essentially (1 & (2 | 3)), i.e. at run-time we support the features supported at build-time which either have a WebView APK implementation, or a framework implementation.


So the app-facing isFeatureSupported(Feature feature) API could be implemented as

public boolean isFeatureSupported(Feature feature) {
    return getAndroidVersion(feature) >= Build.VERSION.SDK_INT
            || getWebViewApkFeatureList().contains(feature);
}

where getAndroidVersion() is something like

public int getAndroidVersion(Feature feature) {
   switch(feature) {
      case VISUAL_STATE_CALLBACK:
         return Build.VERSION_CODES.MARSHMALLOW;
      ...
   }
}

Regardless of whether the rest of the support library will land in time for 66 we need to make sure 66 is compatible with future support libraries - e.g. by proclaiming that no features are supported.

Alright, I think the plan here should be to
1. add integers representing features, in the boundary interface directory (to be shared in both the support library and in chromium)
2. add an Enum corresponding to the integers from 1, to be presented to apps
3. pass the features supported by the support library to the webview apk through the SupportLibraryWebViewFactoryProvider ctor.
4. create a getter (e.g. 'List<Integer> getSupportedFeatures()') on SupportLibraryWebViewFactoryProvider to let the support library side know which features are implemented by the webview apk.
5. surround boundary calls with feature-integer-checks.
6. throw UnsupportedException if apps try to call APIs that are not currently supported


not sure if item nr.3 is completely necessary given that the webview apk will only need to know the features the support library supports in terms of app callbacks. But I guess we could just as well pass the entire list of supported features.

Any thoughts?

Sounds reasonable.

Reasonable plan. Some questions/comments:

1. Instead of exposing the full feature list to devs, what if we add a checked exception (something similar to UnsupportedOperationException) to each callable API?
  - I agree we need to expose a feature list for callback APIs--we need to keep the feature list for those APIs
  - If we ask devs to only check isFeatureSupported(foo), it's likely they'll miss edge cases (they need to downgrade WebView to properly test this)
  - If we use a checked exception, devs are strongly reminded to prepare for failure
  - It's obviously ridiculous to do both: isFeatureSupported doesn't give any additional information
2. Do we need a feature list for anything other than callable APIs and callback APIs? What about targetSdk-specific behavior? Anything I'm forgetting?
3. How about a Set instead of a List? We shouldn't have duplicates and order shouldn't matter.
4. Slight optimization: getSupportLibraryFeatureList() only needs to return the list of callback features; getWebViewApkFeatureList() only needs to return the list of callable features (I think you mentioned half of this in comment 17)
  - Even more optimization: we don't need to pass a list, we can remember version numbers
    - Support lib remembers the 1st version the WebView APK implements a callable API
    - WebView APK remembers the 1st version of the support lib to implement a callback
    - Downside: we can't remove APIs once we publish a version of the support library (I guess we might do this down the road)
  - A different optimization: we don't need a list of callable features
    - When we try to invoke a boundary interface, we should get an exception if the method isn't implemented
    - We can catch this and re-throw
    - We might be able to do the same for callback features (but then we have to catch exceptions and plumb back to the webkit-glue callback, which is a pain)
    - Downside: it's harder to expose a list of callable features (isFeatureSupported has to use reflection to check if methods exist)

In general, I like isFeatureSupported(foo) better than getEffectiveApiLevel()/getSupportedSdkLevel().

Thanks for the response, Nate!
re #19:

(numbers corresponding to your bullet points)
1: ideally we would have a lint check for the webview support library, to ensure devs are calling Feature.isFeatureSupported to guard a support library calls (see b/67245365). In general I'm not super thrilled by throwing an exception whenever the device is using an old webview - IMO exceptions should be thrown in exceptional cases, not as a way to deliver information to the caller. 
2: I believe targetSdk-specific behaviour is orthogonal to the work implemented in the support library - it'll just work as if using the framework webkit APIs.
3: I don't mind either way, a Set sounds more appropriate.
4:
re: version numbers: I think this is complex enough as it is, IMO a list/set of features is easier to debug and argue about than 'first version number feature X was introduced in', and yeah, it'd be nice to have the option to remove features down the line.
re: catching an exception thrown when a method is missing; IMO it's better to declare up-front which features should be supported, if the list is wrong this should manifest itself as an exception being thrown - which we can then catch/discover during testing. Detecting features using reflection seems error-prone / difficult to keep track of?

I don't think throwing a checked exception is a good idea; that's not how platform APIs work and it prevents you from just having all the "modern" references wrapped up in an entire class (and just checking which entire class to use at creation time) by forcing you to wrap each individual call even if that's not necessary.

Mostly on board with #20.

I'm still nervous about throwing all our weight behind lint. Do we have evidence that lint is widespread? Or do we simply not care about devs who don't use lint?

> that's not how platform APIs work

I'm curious, torne@ do you know the rationale for why framework APIs don't use exceptions?

I'm not sure how well-spread lint is. Given that it's used in Android Studio I assume most people use it.

Framework APIs that are missing can't throw exceptions "on purpose" - you would just get NoSuchMethodError from the VM, which isn't a checked exception (since it's a runtime binary compat issue). So, devs have to check API level manually and then die at runtime with NoSuchMethodError if they mess it up; that's analogous to what Gustav is proposing here (check feature flags, get an unchecked exception if you mess it up).

Right, but WebViewCompat#foo() isn't "missing," it's trying to invoke a method which very likely may not exist. I think this is analogous to reflection, which of course has the family of checked ReflectiveOperationExceptions. I thought you meant "framework APIs seldom use checked exceptions to signal errors of any kind."

I think `Build.VERSION.SDK_INT >= Build.VERSION_CODES.X` is reasonable because I would expect serious apps to test manually across all API levels they support. Devs often omit checks like this, but they catch it with automated testing or QA.

On the other hand, isFeatureAvailable(FOO) is roughly equivalent to isWebViewVersionAtLeast(X.0.Y.Z). Testing that fully requires access to all WebView stable releases, which isn't reasonable. I think we should do everything we can to make sure devs get it right the first time.

Well, the framework is much less likely to use checked exceptions for anything than java in general; a lot of things get caught and rethrown wrapped in AndroidRuntimeException specifically to make them unchecked.

In theory the point of checked exceptions is "this is a normal error condition that can occur as the result of things outside the programmer's control, so we're forcing you to check it instead of making it an ignorable return code", whereas unchecked exceptions are "your code is wrong, fix it". I would argue that not following the requirements to check if a function is available falls into the latter.

I also kinda hate checked exceptions as a general principle and wish they didn't exist in Java at all, though, so hey..

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

commit ba016de1efcd69651c043f5958e9c4530c7d36a2
Author: Gustav Sennton <gsennton@google.com>
Date: Fri Mar 09 18:29:59 2018

[WebView Support Library] Pass and fetch list of supported features.

We need some way for either side (android support library + chromium) of
the webview support library to tell what features the other side of the
support library supports. We do this by passing a list of integers
across the boundary. These integers will each represent a feature (which
will be defined in a class in the boundary interface package).

Add a single feature to the list of features to support (visual state
callback).

Bug:  740082 
Change-Id: I791af590cb846d536d01d63649ab6cfffe558ee9
Reviewed-on: https://chromium-review.googlesource.com/941805
Commit-Queue: Gustav Sennton <gsennton@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542169}
[modify] https://crrev.com/ba016de1efcd69651c043f5958e9c4530c7d36a2/android_webview/support_library/boundary_interfaces/BUILD.gn
[add] https://crrev.com/ba016de1efcd69651c043f5958e9c4530c7d36a2/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/SupportLibraryInfoBoundaryInterface.java
[modify] https://crrev.com/ba016de1efcd69651c043f5958e9c4530c7d36a2/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/WebViewProviderFactoryBoundaryInterface.java
[add] https://crrev.com/ba016de1efcd69651c043f5958e9c4530c7d36a2/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/util/Features.java
[modify] https://crrev.com/ba016de1efcd69651c043f5958e9c4530c7d36a2/android_webview/support_library/java/src/org/chromium/support_lib_glue/SupportLibReflectionUtil.java
[modify] https://crrev.com/ba016de1efcd69651c043f5958e9c4530c7d36a2/android_webview/support_library/java/src/org/chromium/support_lib_glue/SupportLibWebViewChromiumFactory.java

I think the chromium-side of this is fixed with the CL in #27.
I think we can move further discussions on forward-compatibility to  crbug.com/777832  ?

We should probably follow-up with support library folks about Nate's thoughts on using checked exceptions and the problem of devs not being able to test feature-detection for the webview support library (because the feature detection depends on which WebView version you're using rather than which Android version).