I don't understand how adding another layer helps here? Do you mean to factor out all the common things into a separate layer? How much common code will there be, if we can't use the same types..

Yeah, this is purely for factoring out common code (avoiding duplication).
I think most of the code we can share is initialization code - e.g. WebViewChromium.initForReal() and stuff inside WebViewChromiumFactoryProvider.ensureChromiumStartedLocked/startChromiumLocked(). It would be great if such code-sharing could simplify the existing glue-layer a little bit to become a more 'pure' conversion layer.

So practically it's not really 'helping' to solve any design problems we have in the support library, it's just to avoid code duplication, and to clear up the glue layer a bit.

Do we have any kind of restriction/rule regarding whether to put startup-code in WebViewChromiumFactoryProvider.initialize() (called from the WebViewChromiumFactoryProvider ctor), and WebViewChromiumFactoryProvider.startChromiumLocked (called when almost any APIs are accessed) ?

It matters for CookieManager, which doesn't fully start chromium. Can't remember if there's anything else like that.

Alright, thanks Bo :).

Another thing I realized when prototyping: if we don't want the shared state between the two glue-layers to be static, we probably have to add some getter returning the shared state (including e.g. a reference to AwBrowserContext) on WebViewChromiumFactoryProvider, which we can then call using reflection (to avoid having to add another Android API) and pass to the support_library_glue.WebViewChromiumFactoryProvider. Am I missing something that would allow for an easier solution?

I think we probably *do* want the shared state to be shared, and it's fine for it to stay static.

Yeah, I don't think we want to e.g. create different AwBrowserContext/AwBrowserProcess for the different glue-layers, but I'm not yet convinced we want static state (e.g. static state is a bit annoying for instrumentation tests, though we don't have any of those for the current glue-layer anyway).

Note that we probably want to share WebViewDelegateFactory between the two glue layers as well.

Ideally we would move as much code as possible from WebViewChromiumFactoryProvider into this new layer, so that WebViewChromiumFactoryProvider itself would contain a minimal implementation of WebViewFactoryProvider.

This means it would be nice to move the following stuff to the new layer
AwBrowserContext
AwBrowserProcess
ResourceRewriter
Application context stuff?
CommandLine
WebViewChromiumRunQueue?

We will also have to determine whether the different static adapter classes (or rather the classes they reference) should be shared between the two glue-layers, e.g. CookieManagerAdapter that holds a reference to AwCookieManager.

it it not obvious to me whether we can create/use several instances of the classes referenced by the static adapters - e.g. AwCookieManager. Given that there is at most WebViewFactoryProvider per app process we probably haven't actually tried creating several of these instances at the same time.

hmm, probably not.

Ideally we'd just have one FactoryProvider, but I forgot which version of android switched from calling FactoryProvider constructor to FactoryProvider.create, and it might not be early enough. Of course this won't work if android calls the constructor directly

So I guess might need to have two FactoryProvider instances, but they'll return the same objects for things like AwCookieManager

Except the FactoryProvider and all the adapters (e.g. CookieManagerAdapter) are built against android.webkit, while we want the support-library glue built against support library classes. So the glue layers have to use different classes (we could create singleton classes for things like AwCookieManager though, to ensure the different glue-layers are using the same thing).

oh yeah, true

Summary of what I think we need to do here:


Given that we are going with an approach where we will augment an existing WebView instance rather than creating a new Support library WebView, we don't really need to share any initialization code between the two glue layers.

All we need to do is ensure we can fetch the relevant existing Chromium objects (like AwContents/mAppTargetSdkVersion in WebViewChromium, and e.g. AwBrowserContext/AwCookieManager in WebViewChromiumFactoryProvider) from the new glue layer. So we really just need WebViewChromium, and WebViewChromiumFactoryProvider to implement some interface in the Aw-layer with lots of getters for the new glue-layer to use. See https://chromium-review.googlesource.com/c/chromium/src/+/677392 for an example - there WebViewChromium implements SharedWebViewProviderState to share its AwContents instance with the support library glue version of WebViewChromium (GlueWebViewChromiumProvider).

Another question here is how to handle callback-clients like WebViewClient - we will have to decide how these will work when shared between two glue-layers. See
https://docs.google.com/a/google.com/document/d/1al0NKFp9CZXEnmgYeybrmv5LK-16Ver228uaXDfpthA/edit?usp=sharing#heading=h.afh71y9wy3gq

Since some of the state in the existing glue-layer is lazily initialized (startChromiumLocked) we should ensure that we don't end up in a state where try to call some support-library api without initializing Chromium correctly.

Looking into this again now.

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

commit a31f7cff03165f61b142c1258fd362ee8e711537
Author: Gustav Sennton <gsennton@google.com>
Date: Tue Feb 13 12:38:20 2018

Add SharedStatics class to share between glue layers.

The WebView support library and the existing android.webkit package
will have some statics methods in common - by sharing a SharedStatics
class between the support library glue and the old glue we ensure these
methods are implemented in the same way in both glue layers.

For now SharedStatics is implemented in the glue layer. Ideally, it
would be implemented in a lower layer to avoid complex dependencies.

We also add an 'adapter-lock' in the WebViewChromiumFactoryProvider to
guard initialization of webkit <-> aw-layer adapters. The idea is for
aw-objects to be initialized in WebViewChromiumAwInit while the adapters
wrapping those aw-objects are created in WebViewChromiumFactoryProvider.

Bug:  735943 , 807332

Change-Id: Ie33407a5d4286df692ba975a7cc7bbfd3f1d22e2
Reviewed-on: https://chromium-review.googlesource.com/908328
Commit-Queue: Gustav Sennton <gsennton@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536343}
[modify] https://crrev.com/a31f7cff03165f61b142c1258fd362ee8e711537/android_webview/glue/BUILD.gn
[add] https://crrev.com/a31f7cff03165f61b142c1258fd362ee8e711537/android_webview/glue/java/src/com/android/webview/chromium/SharedStatics.java
[modify] https://crrev.com/a31f7cff03165f61b142c1258fd362ee8e711537/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumAwInit.java
[modify] https://crrev.com/a31f7cff03165f61b142c1258fd362ee8e711537/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java

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

commit fa5378a9c28aacc095174d4d8a982e494de0f13d
Author: Gustav Sennton <gsennton@google.com>
Date: Tue Feb 13 20:00:39 2018

Split parts of WebViewChromium into a class shared between glue layers.

To share code between the existing webkit glue layer and the new support
library glue layer we here break out some functionality out of
WebViewChromium into a SharedWebViewChromium class that will be used by
both glue layers.
The idea is that new functionality should be added in that shared class
to avoid code duplication bugs.

For now only the insertVisualStateCallback method is put in
SharedWebViewChromium. Long-term, all WebViewProvider methods should be
put in SharedWebViewChromium to avoid confusion regarding where a method
should be implemented.

Bug:  735943 
Change-Id: I91b6dd4b5f092e1580aa754981974c386c6f8ff0
Reviewed-on: https://chromium-review.googlesource.com/916181
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Gustav Sennton <gsennton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536431}
[modify] https://crrev.com/fa5378a9c28aacc095174d4d8a982e494de0f13d/android_webview/glue/BUILD.gn
[add] https://crrev.com/fa5378a9c28aacc095174d4d8a982e494de0f13d/android_webview/glue/java/src/com/android/webview/chromium/SharedWebViewChromium.java
[modify] https://crrev.com/fa5378a9c28aacc095174d4d8a982e494de0f13d/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
[modify] https://crrev.com/fa5378a9c28aacc095174d4d8a982e494de0f13d/android_webview/java/src/org/chromium/android_webview/WebViewChromiumRunQueue.java

Marking this as fixed - I still think we should move as much logic as possible (i.e. logic that can be shared between the two glue layers) out of the existing webkit glue layer though.