I would like to scope this out more: if we want to tell users to perform these kinds of conversions as often as possible, does it make sense to create a new support-library-glue adapter every time app asks for a conversion? Example:

// Somewhere in the support library glue layer
public SupLibWebSettingsAdapter convertWebSettings(WebSettings webSettings) {
    return new SupLibWebSettingsAdapter(
            ((ContentSettingsAdapter)webSettings).getAwSettings());
}

Note that every time we make a call between the support library and the webview apk code we invocationhandlers and proxies, so maybe creating a new adapter object isn't such a big deal?

Torne/Toby/Bo, WDYT?

The alternative to creating new adapters with each conversion would be to use a (weak) hashmap either in the support library itself, or in the support library glue layer to convert webkit objects to compat-objects.

> does it make sense to create a new support-library-glue adapter every time app asks for a conversion?

WebSettingsCompat.from(WebSettings) should always return the same object given the same object. I don't think we should allow apps to mess this up in complex ways.

> The alternative to creating new adapters with each conversion would be to use a (weak) hashmap either in the support library itself

Just have AwSettings or SettingsAdapter hold onto SettingsCompat. No need to worry about weak reference and things.

Ok this will be a bit funny.
Using the WebSettings example:
the first time we convert a WebSettings object to a WebSettingsCompat we will create a new
'SupportLibWebSettingsAdapter extends WebSettingsCompat'
in the support library glue layer, and pass an InvocationHandler to the support library side (from the webview apk code) - on the support library side (i.e. in the app classloader) we will then use the class java.lang.reflect.Proxy to create an implementation of WebSettingsBoundaryInterface and finally create a WebSettingsCompat by using another adapter (defined in the app classloader).
So to the app we will return an adapter created from the app-classloader. It's not obvious how we would then hold on to that adapter from the webview apk code (unless we pass that adapater back into the webview apk, and store it as an 'Object').

What if SupportLibWebSettingsAdapter were a class had to call back to the app? Can you just pretend it had to do that, except in this case it doesn't actually need to. I don't know if Object makes more sense, or if support lib glue should declare an interface that's shared with the support lib. Object isn't all that bad here imo..

Just to clarify comment #4 from Bo, to hold on to an Object in the support library glue / normal glue we would do this:


FooCompat.of(Foo foo) {
    FooCompat fooCompat = createFooCompatFromInvocationHandler(callIntoWebViewApkToGetFooCompat(foo));
    callIntoWebViewApkToAttachFooCompatToFoo(foo, fooCompat);
    return fooCompat;
}


where the only new part really is
    callIntoWebViewApkToAttachFooCompatToFoo(foo, fooCompat);
which will call into the webview apk to store an Object fooCompat in the FooAdapter from the old glue.

you should also first check if if there's a FooCompat attached already, and just return that instead of creating a new one. and the whole thing needs to be guarded with a lock

Yeah, I don't think we want to be recreating these objects, as Bo says; we should be returning the same one every time. Storing them in the corresponding Aw object is a nice way to avoid worrying about their lifetimes which I didn't think of when looking at this before; having to lock around this operation is mildly annoying but not a huge deal (there's no IO or blocking operations going on here, just a bunch of heap object juggling).

Cool, thanks for quick responses guys, this is super helpful :)


Related to this, there's also the question of falling back to using the framework when the device in on an up-to-date Android version. E.g. if the support library targets P and the device it runs on is on P then WebViewCompat.of(WebView) should return an implementation using WebView APIs rather than hooking into the support library glue:

// Support Library for P
WebViewCompat.of(WebView webview) {
    if (BUILD.SDK_INT >= BUILD.VERSION_CODES.P) {
        return new WebViewCompatFrameworkImpl(webview);
    } else {
        return callIntoSupLibEtc(webview); // as described in comments 5 and 6 above
    }
}

@TargetApi(28)
class WebViewCompatFrameworkImpl {
    private WebView mWebview;

    WebViewCompatFrameworkImpl(WebView webview) { mWebView = webview; }

    public void postVisualStateCallback(long requestId, VisualStateCallbackCompat callback) {
        mWebView.postVisualStateCallback(requestId, new VisualStateCallback() {
            public void onComplete(long requestId) {
                callback.onComplete(requestId);
            }
        });
    }
}

And then again we have the problem of recreating WebViewCompatFrameworkImpl over and over again (and in this case it's not obvious what object we would attach this to while at the same time avoiding to use the support-library reflection).


For this case in particular I think it would make more sense to have static support library APIs taking the used object as a parameter, like in the support library ViewCompat
 [1]:

// WebViewCompat class
public static postVisualStateCallback(WebView webview, long requestId, VisualStateCallbackCompat callback) {
    if (BUILD.SDK_INT >= BUILD.VERSION_CODES.P) {
        IMPL.postVisualStateCallback(webview, requestId, callback);
    } else {
        return callIntoSupLibEtc(webview); // as described in comments 5 and 6 above
    }
}

where IMPL is set like this:

static final WebViewCompatBaseImpl IMPL;
static {
    if (Build.VERSION.SDK_INT >= 30) {
        IMPL = new ViewCompatApi30Impl();
    } else {
        IMPL = new ViewCompatBaseImpl();
    }
}


Any thoughts?



[1]
https://cs.corp.google.com/android/frameworks/support/compat/src/main/java/android/support/v4/view/ViewCompat.java

> And then again we have the problem of recreating WebViewCompatFrameworkImpl over and over again (and in this case it's not obvious what object we would attach this to while at the same time avoiding to use the support-library reflection).

It's obvious which object to attach to. It's the reflection that's the problem.

> For this case in particular I think it would make more sense to have static support library APIs taking the used object as a parameter, like in the support library ViewCompat

You probably have given this more thought than me?

For classes that have always existed for all versions of android the support library supports, this way does feel easier, and I don't really see any down sides.

> For classes that have always existed for all versions of android the support library supports, this way does feel easier, and I don't really see any down sides.

"this way" as in the ViewCompat way? (static WebViewCompat.foo(WebView, Bar);)

> "this way" as in the ViewCompat way? (static WebViewCompat.foo(WebView, Bar);)

yes

We'll have to think through this a bit more - I just realized classes introduced post-L CANNOT be implemented using the 'static FooCompat.method(Foo, Bar)' approach since for some Android versions (android.webkit.)Foo itself won't exist.

Note: none of our "don't return different objects each time" concerns matter for objects which are
1. new since L, and
2. passed to app callbacks
example: WebResourceError
The reason for this is that there are no situations for such classes where the app expects to receive the same object twice (it's just passed through a callback - e.g. WebViewClient.onReceivedError(..)).

This covers the following classes:
WebResourceError
SafeBrowsingResponse
RenderProcessGoneDetail (probably not relevant anyway)

Right, you are going to need support lib equivalents for those cases. Wasn't that always the plan?

That was indeed the plan, I just want to make sure it works in terms of not returning different objects each time we fetch an object like that (or determine that it's a non-issue :)). 

I think objects that are static - i.e. ones where we have one single global instance - are not really a problem either, since we can fetch the object once through reflection, and then store it in a static variable on the support library side.
This covers e.g. ServiceWorkerController, and thus also ServiceWorkerWebSettings (since there's just one ServiceWorkerWebSettings per ServiceWorkerController).
Would it make sense to have a static variable to store e.g. ServiceWorkerWebSettings in the support library, or would that encode an assumption we might want to change going forward?


Assuming the solution in the above paragraph is OK the only classes for which this is really an issue are ones with the following attributes
1. introduced post-L (for pre-L classes we can use static-ViewCompat APIs)
2. there exists some API that should return the same Compat-object when called several times (e.g. WebViewCompat.of(WebView)).

If we want to implement FooCompat.of(Foo) we would just attach FooCompat to the old-glue adapter for Foo, so that's no problem.


I don't think there are any other classes that fall under categories 1 and 2 at the same time:
The only post-L class that isn't created/overridden by apps, and isn't static, is WebMessagePort, which we only have one API for fetching:
class WebView {
    public WebMessagePort[] createWebMessageChannel();
}
and that API should return new WebMessagePorts with each call anyway (AFAICT).



A related question:
w.r.t. to attaching Compat objects to adapters in chromium: is there any reason for attaching an object except to make sure we always return the same compat object from the support library? Otherwise the whole 'static ViewCompat' methods API makes the attaching unnecessary (for pre-L classes) since by using such APIs we never return compat-objects to the app.

Thinking about this a bit more, I don't think we will ever have a problem with introducing a new class, and introducing a method for returning the same instance of that class over and over again:
1. FooCompat.of(Foo) is only an issue when we DON'T want to use reflection - with reflection we can just attach the compat object to the glue-layer object implementing Foo. 
Otherwise I only see a few ways we might introduce such a class/method pair:
2. a getter, like WebView.getSettings (should return the same WebSettings over and over again), in this case we can just store the object we return from the getter (WebSettingsCompat) in the owning object (WebViewCompat).
3. getters for static objects - we can just store the returned object statically.
This is not an exhaustive list, but I'm having a hard time seeing a case which is actually a problem..


So the only case which isn't optimal is the implementation of 'FooCompat.of(Foo)' for post-L classes. And that method isn't really necessary anyway for post-L classes - because for those classes we will have to provide APIs to get a FooCompat without owning a Foo anyway (since Foo might not be defined).

So to summarize:
A)
for pre-L classes we will use the 'static ViewCompat' approach because it means we don't need a 'FooCompat.of(Foo)' method and thus don't need to use reflection in the case when the device we're running on is up-to-date enough to all methods in Foo.
B)
for post-L classes the FooCompats will be actual classes with non-static methods, and we will store these Compat-classes in the most convenient way - e.g. in the owning Compat-class, or as a static variable if the instance is static.

Does this make sense? :)

> Would it make sense to have a static variable to store e.g. ServiceWorkerWebSettings in the support library, or would that encode an assumption we might want to change going forward?

ServiceWorkerWebSettings is a bad example, because the API doesn't actually imply it's a global singleton (even if it's implemented as one). ServiceWorkerController is better, since ServiceWorkerController.getInstance implies it is a singleton.

You can't do ServiceWorkerControllerCompat.from(ServiceWorkerController) pattern here, since app may not have access to ServiceWorkerController either. So you'd have to do ServiceWorkerControllerCompat.getInstance as well, which means the support lib API pretty much specifies it's a singleton as well, in which case keeping it in a static variable in the support lib is fine.

Then going back to ServiceWorkerWebSettingsCompat, you probably want to just keep it as a member of ServiceWorkerControllerCompat.

> w.r.t. to attaching Compat objects to adapters in chromium: is there any reason for attaching an object except to make sure we always return the same compat object from the support library?

No, don't think so

> Does this make sense? :)

Yes

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

commit a193b7d8016a25799f2a821f229caa2e28f0d5be
Author: Gustav Sennton <gsennton@google.com>
Date: Wed Feb 14 22:58:23 2018

Unify WebResourceRequestImpl classes into WebResourceRequestAdapter.

When implementing the WebView Support Library we will need to cast
webkit classes to their adapter/implementation classes from the webkit
glue (to fetch the underlying Aw-object).
To allow casting a WebResourceRequest to its implementation we here
unify all implementations of WebResourceRequest into one single class.

Bug:  807333 
Change-Id: Iea473d9d044e11be4da4d1383867a3050b79ba16
Reviewed-on: https://chromium-review.googlesource.com/919361
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Gustav Sennton <gsennton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536860}
[modify] https://crrev.com/a193b7d8016a25799f2a821f229caa2e28f0d5be/android_webview/glue/BUILD.gn
[modify] https://crrev.com/a193b7d8016a25799f2a821f229caa2e28f0d5be/android_webview/glue/java/src/com/android/webview/chromium/ServiceWorkerClientAdapter.java
[add] https://crrev.com/a193b7d8016a25799f2a821f229caa2e28f0d5be/android_webview/glue/java/src/com/android/webview/chromium/WebResourceRequestAdapter.java
[modify] https://crrev.com/a193b7d8016a25799f2a821f229caa2e28f0d5be/android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java

Marking as fixed - the plan is in comments #17/18.