Ensure the WebView Support Library is forwards compatible. |
||||
Issue descriptionWe might end up deprecating and removing WebView APIs in the future, so we should make sure the WebView Support library is forwards compatible, i.e. it doesn't crash if one of its APIs is unavailable. Basically, we have to make sure that, both in the support library, and in the glue-layer in the WebView APK, any calls into the opposing side (glue resp. support library) are checked so they don't cause a crash if we remove support for an API. It would be cool if we could add a test to ensure this mechanism works as well - if we mess this up in any version of the support library or WebView APK we will cause crashes for that version when removing some WebView API in the future.
,
Dec 18 2017
I'm not entirely convinced which way to go here: Ideally (for testing/avoiding accidental method removals) we would throw a NoSuchMethodError whenever the WebView Support library tries to call a method that doesn't exist in the WebView APK. However, since the support library should be forwards-compatible, i.e. removing methods shouldn't cause crashes (according to the Android Support Library team, and to me this makes sense as well to avoid having to support super old methods). If we think accidental method removals are a problem this calls for a way to explicitly let the support library know that a certain method has been removed - so that we can avoid calling that method, and still propagate NoSuchMethodError exceptions for cases where methods were accidentally removed. I'm not sure whether it's worth having an explicit way of knowing that certain methods have been removed..
,
Mar 9 2018
So the way we're solving this is by using feature flags to declare whether a feature is supported in the support library vs. in the webview APK. One question that Nate raised on https://chromium-review.googlesource.com/c/chromium/src/+/941805 is how we replace / remove / add APIs: "what if we introduce onPageCommitVisible2 (similar purpose, but a different signature for some reason). We can't put it in the VISUAL_STATE_CALLBACK feature because old APKs will say they implement this feature, but won't know about the new API. Do we just create a new VISUAL_STATE_CALLBACK_2 feature?" response: Yeah, for new APIs we'd need to add a new feature string/flag. The same thing holds when we want to replace APIs - we'd just have to add a new feature flag for the new API. Deleting an API just means we remove the feature flag from our list of supported features. I think the more important/tricky question is: what do we do if we have included too many APIs in one of our feature flags? E.g. if the VISUAL_STATE_CALLBACK feature represents both postVisualStateCallback and onPageCommitVisible, and we either remove or replace one (but not both) of these APIs. VISUAL_STATE_CALLBACK will always have to represent both of those APIs (if it did at some point) so we would essentially have to split that feature flag in two (or introduce 'negative' internal flags, e.g. POST_VISUAL_STATE_CALLBACK_UNSUPPORTED), which means app devs would have to change their code between support library updates. This kinda suggests that we should be using one feature flag per public method (unless the methods protected under the same flag are so inter-connected that to change one you'd have to change the other as well). The only thing that bothers me here is that this will accumulate into a lot of feature flags.. WDYT?
,
Mar 9 2018
I'm not sure I really understand what case you're referring to. When you say "removing apis" - from where?
,
Mar 9 2018
removing apis from the webview support library
,
Mar 9 2018
(and from the support library glue-layer in the WebView APK)
,
Mar 9 2018
It seems like we shouldn't be removing anything from the glue in the APK, because that'd remove functionality from existing apps even if they feature-detect it properly to avoid crashing. Removing stuff from the support library side doesn't seem like it needs to interact with feature detection generally, because apps just won't compile against the new version if they use the removed things..
,
Mar 9 2018
> It seems like we shouldn't be removing anything from the glue in the APK Agreed. > if the VISUAL_STATE_CALLBACK feature represents both postVisualStateCallback and onPageCommitVisible, and we either remove or replace one (but not both) of these APIs. One idea: never remove a fraction of a Feature, removing is either all or nothing. If we introduce a replacement API later (e.g., onPageCommitVisible2 is a better version of onPageCommitVisible), then we allow the app to use both features. OTOH, if we introduce both postVisualStateCallback2 and onPageCommitVisible2, then we may remove the original APIs in favor of the new-and-improved ones (which live under a new, different Feature).
,
Mar 12 2018
Yeah, that sounds good Nate, if we decide to merge several APIs in the same feature, we should be prepared to remove/replace all of those APIs at the same time.
,
Apr 3 2018
Alright, cool, the forward compatibility should be fixed by our feature flags. In the future we might want to do just like Android and create API files for our boundary interface to declare which APIs are supported depending on the Support Library version - as an extra check to ensure we don't break the API surface. Though, the boundary interfaces already only contain APIs/interfaces, so I'm not sure extra API files will make any difference? |
||||
►
Sign in to add a comment |
||||
Comment 1 by gsennton@chromium.org
, Nov 14 2017