I don't think we're going with this approach, since I'm aiming to merge chrome and webview safe browsing as much as possible (see issue 748687).

I think it's sufficient to use PlatformServiceBridge#canUseGms() like we currently do.

Are you planning to get rid of SafeBrowsingApiBridge? IMO it's a bit silly to have that and also PlatformServiceBridge. Best to reduce the amount of hacky reflection stuff that can't be verified at compile time.

The plan is to move common logic upstream (for both chrome and webview) upstream. The goal is to minimize code duplication. AW-specific/chrome-specific logic can be done with upstream subclasses.

GMS APIs will be exposed in a bridge class. I can't reuse PlatformServiceBridge for this because I need something common to both Chrome and WebView.

What seems even sillier is using reflection to get the derived class, when in a downstream build, the derived class is right there at build time. What if instead of using reflection we just included one class or the other in the build?

E.g. we'd rename PlatformServiceBridgeGoogle to PlatformServiceBridge, so both upstream and downstream have a class called PlatformServiceBridge. The packages would also have to match (PlatformServiceBridge is currently under org.chromium.android_webview whereas PlatformServiceBridgeGoogle is under com.android.webview.chromium.)

We'd put a condition in GN to use only one or the other, and prefer the downstream one if it's present. (We could do that with a bridge_path GN variable that gets overridden downstream.)

Then when you import PlatformServiceBridge from any other Java file you'll get the right one, with no reflection.

A potential downside is the inability to share code between the 2 classes, by calling super. Neither PlatformServiceBridgeGoogle nor SafeBrowsingApiHandlerInternal currently do that, though. (And SafeBrowsingApiHandler is an interface anyway.)

That sounds like a plan to me.
I don't think the two different classes will ever need to share code - the upstream class isn't really doing anything anyway, right?

I'd double-check with someone with experience from maintaining upstream vs. downstream code though before coding this solution. Torne, WDYT about having different versions of the same class in upstream and downstream?

Talked with boliu@ & ntfschr@ and we decided to extend this approach to other reflection sites around WebView. (We could also extend it outside WebView, but lets not worry about that quite yet.) Passing to jamwalla@. Some more explanation, and my current plan:

We have several places in WebView where upstream code needs to call downstream code:

- PlatformServiceBridge fetches PlatformServiceBridgeGoogle:
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java

- AwSafeBrowsingConfigHelper fetches AwSafeBrowsingApiHandler:
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java

- AwBrowserContext also fetches AwSafeBrowsingApiHandler:
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java

- AwContentsStatics fetches SafeBrowsingWarmUpHelper:
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwContentsStatics.java

This code all needs to compile in a plain chromium checkout, when the downstream code isn't present, so they all use reflection. This is ugly because the compiler can't check our use of reflection, and because in a downstream checkout, the reflected classes are actually there at compile time, so conceptually reflection shouldn't be necessary.

We can improve this by making each downstream class implement an upstream base class. The base class will be full of no-ops, so chromium builds will just skip over the missing features. Then we'll have an upstream factory which provides the no-op objects, and an alternative, homonymous, downstream factory which provides the downstream implementations. We'll use GN to switch between the 2 factories.

Re-stated in more detail: we'll create 2 factory classes with the same name, 1 downstream and 1 upstream. Say we call it "PrivateFactory" (since it's a factory for closed-source implementations):

android_webview/java/src/org/chromium/android_webview/PrivateFactory.java

Since the upstream and downstream versions have the same name and package, they'd conflict if both included in the same build. So we'll only include one of them, using a GN variable to point to the included one. By default, the variable will point to the upstream PrivateFactory.java, but downstream it'll be overridden to point to the downstream file. (See the google_play_services_package variable for an example of overriding a value downstream.)

Now take PlatformServiceBridge for example, since it already has an upstream base class and a downstream implementation. The upstream PrivateFactory will have a method called e.g. createPlatformServiceBridge which instantiates the PlatformServiceBridge base class. The downstream PrivateFactory will have the same method, but it'll instead create a PlatformServiceBridgeGoogle. In PlatformServiceBridge.getInstance, instead of using reflection, it'll just call PrivateFactory.createPlatformServiceBridge, and it will get the correct object depending on the type of checkout.

We'll do similar refactorings for the Safe Browsing classes.

Awesome, thanks for fixing this!

We've discovered a dependency cycle in my proposed solution.

1. The downstream PrivateFactory must import downstream classes in order to instantiate them.
2. Downstream classes already depend on upstream classes. 
3. Upstream classes must import PrivateFactory.

To make this work, everything (upstream Java, downstream Java, and PrivateFactory) must be compiled at the same time. (This is only an issue in Java. C separates compiling and linking, so you can compile mutually dependent pieces separately.)

I have some ideas.

1. We could compile the upstream and downstream Java together in the same targets.

We'd remove all of Webview's downstream android_library targets, and instead put their source and dependency lists in variables. E.g. downstream android_webview/java:java could become android_webview_private_java and android_webview_private_java_deps.

We'd import these variables in the downstream config.gni, while assigning them to empty lists by default in upstream build/config/android/config.gni.

Then we'd append these variables to the source and dependency lists in their corresponding upstream targets. E.g. android_webview_private_java and android_webview_private_java_deps would be appended in //android_webview:android_webview_java.

Thus in a chromium checkout, the variables would be empty lists, and have no effect on the upstream targets. In a full checkout, the downstream Java files would be included into the upstream targets, allowing the dependency cycle.

This gets kind of messy, but it still might be better than reflection. The main problem is that it disallows us from having any build logic downstream; we would only be allowed to list sources and dependencies. This would be a problem for next/, which has some ijar-related logic. There could be more problems in the future if we need more logic.

2. We could always build upstream Java with the upstream PrivateFactory (which doesn't import downstream Java), then swap in the downstream PrivateFactory as a separate build step.

The upstream PrivateFactory would be included unconditionally in //android_webview:android_webview_java. The downstream PrivateFactory would have its own target downstream which would depend on both the upstream and downstream WebView Java targets.

We'd write a custom build action downstream to remove the upstream PrivateFactory from the upstream library, and insert the downstream PrivateFactory.

This allows us to keep upstream and downstream targets separate, despite the dependency cycle. The downsides are that it's a non-obvious thing to do, which may surprise people in the future, and that the compiler wouldn't be able to catch errors in how upstream code calls the downstream PrivateFactory. (Though we could cover that with a simple test which just calls every PrivateFactory method.)

3. We could give up on completely removing reflection, and instead limit reflection to a single use in PrivateFactory.

So in a full checkout, both PrivateFactories would be present, and the upstream one would fetch the downstream one via reflection, just like PlatformServiceBridge currently does.

Thoughts?

I'm probably missing something but it feels like we should be able to break the circular dependency somehow:

Do our downstream classes depend on upstream implementations, or just upstream interfaces? If they only depend on interfaces, can't we just put those in a separate target (and have both the upstream + downstream classes implement those interfaces)?


I thought Java separated compiling and linking as well? (as in, compiling happens during build-time, linking happens during run-time?). But I'm probably just confused :)

Upstream interface should work. Please refer to AppHooks and AppHooksImpl implementations for Clank.

Yeah, that's a better idea. Downstream AwSafeBrowsingApiHandler calls upstream AwContentsLifecycleNotifier.addObserver and AwSafeBrowsingConfigHelper.getSafeBrowsingUserOptIn (both static), so those would need to be refactored into abstract base classes containing singleton references to their implementations. And then we'd have to install those implementations, probably in WebViewChromiumFactoryProvider. But otherwise I think that would work.

Right, AwContentsLifecycleNotifier.addObserver is static.. if it wasn't we could have just passed an upstream implementation of AwContentsLifecycleNotifier to AwSafeBrowsingApiHandler (in general it seems to me classes containing static methods cause design issues like this one, but there's probably a good reason for this particular method to be static..?).

Actually, to me these kind of classes (with mostly static behaviour) seem like utility classes, that we should be able to factor out into their own component fairly easily. Paul, WDYT about putting AwContentsLifecycleNotifier and AwSafeBrowsingConfigHelper into their own target (something like android_webview_utility_java, or android_webview_statics_java) instead of just factoring out their interfaces/base classes. Would that make sense, or do you think it would make matter more complicated? Then we could again make both upstream and downstream android_webview depend on those utility/statics classes without having to initialize anything in WebViewChromiumFactoryProvider. 


An alternative could be to just factor out the entire classes (AwContentsLifecycleNotifier and AwSafeBrowsingConfigHelper) into the separate target, instead of factoring out interfaces for them. In that way you don't need to initialize anything from WebViewChromiumFactoryProvider. Or does the jni/native dependency in AwContentsLifecycleNotifier prevent us from doing that?

Woops, that's a lot of random thoughts ;). I meant to only write the last paragraph, so it is meant to be read "an alternative to comment #14 could be to..."

Just talked to Paul - I think we're not able to pull AwSafeBrowsingConfigHelper into a separate target because it's one of the classes that uses reflection and so it's going to depend on PrivateFactory (and create a new, shorter cycle). Let me know if that sounds wrong or if there's a way around it, and in the meantime I'm going to try the approach in #14.

You could just create an interface
AwSafeBrowsingOptInPreferenceFetcher {
    boolean getSafeBrowsingUserOptIn();
}

have AwSafeBrowsingConfigHelper (upstream) implement that, and pass the AwSafeBrowsingConfigHelper to the downstream AwSafeBrowsingApiHandler.

Would that make sense? I don't know why all of this is static in the first place so I might be misleading you :)  

Thanks, Gustav, that works! I refactored AwSafeBrowsingConfigHelper into a singleton class.

Can you help me figure out how to pass the AwSafeBrowsingConfigHelper to the downstream AwSafeBrowsingApiHandler? Should that happen in WebViewChromiumFactoryProvider (by adding a static setter downstream)?

(Paul, correct me if I'm wrong) I believe the way we set out to do this is:

Define 2 different classes called PrivateFactory - on Upstream and one DownStream. PrivateFactory will instantiate classes that depend on GmsCore (with a no-op implementation upstream and a gms-core implementation downstream) like AwSafeBrowsingApiHandler.
In our case AwSafeBrowsingApiHandler depends on some APIs from AwSafeBrowsingConfigHelper, so AwSafeBrowsingConfigHelper should implement an interface which is defined in another target than AwSafeBrowsingConfigHelper (to avoid the dependency cycle AwSafeBrowsingConfigHelper -> PrivateFactory -> downstream AwSafeBrowsingApiHandler -> AwSafeBrowsingConfigHelper).

// Upstream
interface AwSafeBrowsingConfigHelperInterface { // This should probably be named just AwSafeBrowsingConfigHelper
    Boolean getSafeBrowsingUserOptIn();
}

class AwSafeBrowsingConfigHelper implements AwSafeBrowsingConfigHelperInterface { // Should probably be named 
AwSafeBrowsingConfigHelperImpl
   // existing implementation..
}

// Downstream
class PrivateFactory {
    AwSafeBrowsingApiHandler createSafeBrowsingApiHandler(AwSafeBrowsingConfigHelperInterface configHelper) {
        return new AwSafeBrowsingApiHandler(configHelper, ...);
    }
}


To be honest I'm a bit confused here - why are we using reflection in AwSafeBrowsingConfigHelper to fetch a static method (getUserOptInPreference) from AwSafeBrowsingApiHandler? Can't we just make that method non-static and call that method through SafeBrowsingApiBridge instead?

I think, instead of interfaces, they'll need to be base classes with singleton references. That way, downstream can get a reference to the derived object. So:

// upstream, in a GN target that downstream can depend on
class AwSafeBrowsingConfigHelper {
    static AwSafeBrowsingConfigHelper sInstance;
    static AwSafeBrowsingConfigHelper setInstance(AwSafeBrowsingConfigHelper instance) { sInstance = instance; }
    static AwSafeBrowsingConfigHelper getInstance() { if (sInstance == nul) { oh noes abandon ship } return sInstance; }
    boolean getSafeBrowsingUserOptIn();
}

// upstream, in a GN target that downstream can't depend on, because it depends on PrivateFactory
class AwSafeBrowsingConfigHelperImpl extends AwSafeBrowsingConfigHelper {
    @Override
    boolean getSafeBrowsingUserOptIn() { ... }
}

// upstream, probably in WebViewChromiumFactoryProvider
AwSafeBrowsingConfigHelper.setInstance(new AwSafeBrowsingConfigHelperImpl());

// downstream
AwSafeBrowsingConfigHelper.getInstance().getSafeBrowsingUserOptIn()

If we don't use singletons, then I'm not sure how downstream would get references to the upstream *Impl objects.

Thanks! I used different class names but I can change them later. I used WebViewChromiumFactoryProvider to pass AwSafeBrowsingConfigHelper to AwSafeBrowsingApiHandler[Google] (the new name for the downstream class).

I'm working on PlatformServiceBridge/Google, now, which is part of the library android_webview_platform_services_java, which has the following comment:
# Separate target to allow for a dependency on GmsCore without pulling in all of
# android_webview_java.

I don't think this will work any more: we want PrivateFactory to create PlatformServiceBridges, and we want PlatformServiceBridge to call PrivateFactory to get the right class (upstream or downstream), so android_webview_java and android_webview_platform_services_java depend on each other. Can I move PlatformServiceBridge back into android_webview_java?

Do we need a mini design doc for this? So far we have 22 messages of back-and-forth for what we expected to be a noogler-level refactor. At this point, I don't know how I should expect this code to change.

I pulled out the reflection for everything except platformservicebridge, so let me make a doc explaining what it looks like.

Here's a doc describing how to pull reflection out of everything except PlatformServiceBridge:
https://docs.google.com/a/google.com/document/d/1bOBB0Z8BE4uQzAtbr1vDYoohf4zvQRb_HeRAqqjDYu4/edit?usp=sharing

Here's the CL for the upstream changes https://chromium-review.googlesource.com/c/chromium/src/+/754164/

I'm having some difficulty uploading the downstream changes, but everything works locally.

(This is the accompanying downstream CL: https://chrome-internal-review.googlesource.com/c/clank/internal/apps/+/498998 - some discussion has moved to doc, tho)

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/d9f81956262f1620aa62a0e1217759f453e6c325

commit d9f81956262f1620aa62a0e1217759f453e6c325
Author: James Wallace-Lee <jamwalla@google.com>
Date: Wed Nov 15 23:20:21 2017

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

commit 879fb124ce96a93d0ea96947b14eeebbe3cec6c6
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Mon Nov 27 22:40:39 2017

WebView: move statics into PlatformServiceBridge

(Part 2 of a 3-sided patch.)
Depends on crrev.com/i/502732

Downstream, we are moving static SafeBrowsing methods into
PlatformServiceBridge. This CL replaces upstream calls to SafeBrowsing
methods, which previously used reflections, with calls to new instance
methods in PlatformServiceBridge.

BUG= 682070 

Change-Id: Id457a6065aa1d16b2acb8404a2fea4070b0879be
Reviewed-on: https://chromium-review.googlesource.com/765092
Reviewed-by: Paul Miller <paulmiller@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519432}
[modify] https://crrev.com/879fb124ce96a93d0ea96947b14eeebbe3cec6c6/android_webview/BUILD.gn
[modify] https://crrev.com/879fb124ce96a93d0ea96947b14eeebbe3cec6c6/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java
[modify] https://crrev.com/879fb124ce96a93d0ea96947b14eeebbe3cec6c6/android_webview/java/src/org/chromium/android_webview/AwContentsStatics.java
[modify] https://crrev.com/879fb124ce96a93d0ea96947b14eeebbe3cec6c6/android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java
[modify] https://crrev.com/879fb124ce96a93d0ea96947b14eeebbe3cec6c6/android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/b15ad33450f9c0432a495f3b57b1ed9d88974e2e

commit b15ad33450f9c0432a495f3b57b1ed9d88974e2e
Author: James Wallace-Lee <jamwalla@google.com>
Date: Wed Nov 29 00:25:16 2017

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

commit 23faa7b09e9ab053d0eac9154f40a0c7238ab2fc
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Tue Feb 13 20:38:10 2018

create empty PlatformServiceBridgeImpl

Add an empty PlatformServiceBridgeImpl to subclass PlatformServiceBridge,
plus a new target for this class. This doesn't do anything yet, but later
CLs need this class.

Bug:  682070 
Change-Id: I07079e9089bfd36ebc818762b0676064e39d4247
Reviewed-on: https://chromium-review.googlesource.com/912461
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536456}
[modify] https://crrev.com/23faa7b09e9ab053d0eac9154f40a0c7238ab2fc/android_webview/BUILD.gn
[add] https://crrev.com/23faa7b09e9ab053d0eac9154f40a0c7238ab2fc/android_webview/java/src/org/chromium/android_webview/PlatformServiceBridgeImpl.java

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/b8da7ae5083051cf463b27e8e37b2c5946844ca2

commit b8da7ae5083051cf463b27e8e37b2c5946844ca2
Author: James Wallace-Lee <jamwalla@google.com>
Date: Fri Mar 02 21:55:56 2018

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

commit 088e6832cc34214563bc64c9a33a034c0de3b58d
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Sat Mar 03 14:42:36 2018

Make PlatformServiceBridge instantiate PlatformServiceBridgeImpl

PlatformServiceBridge, instead of using reflection, should load
an instance of PlatformServiceBridgeImpl that is determined by GN.
A downstream cl, which should land first, provides an alternate
implementation of PlatformServiceBridgeImpl.

Bug:  682070 
Change-Id: I642603ccb4e4dd4a602803772acd343bfe6cf0dd
Reviewed-on: https://chromium-review.googlesource.com/897994
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Paul Miller <paulmiller@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540752}
[modify] https://crrev.com/088e6832cc34214563bc64c9a33a034c0de3b58d/android_webview/BUILD.gn
[modify] https://crrev.com/088e6832cc34214563bc64c9a33a034c0de3b58d/android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java
[modify] https://crrev.com/088e6832cc34214563bc64c9a33a034c0de3b58d/android_webview/test/BUILD.gn
[modify] https://crrev.com/088e6832cc34214563bc64c9a33a034c0de3b58d/chrome/android/BUILD.gn

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/fb8cb31052a5d600da75516bd85ae70cfb895282

commit fb8cb31052a5d600da75516bd85ae70cfb895282
Author: James Wallace-Lee <jamwalla@google.com>
Date: Tue Mar 13 21:07:59 2018

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

commit a5c3304e0007f5c492139b2ad152462427d3515b
Author: Nate Fischer <ntfschr@chromium.org>
Date: Tue Apr 03 17:11:07 2018

AW: remove unnecessary annotations

No change to behavior.

This removes no-longer-needed annotations for Safe Browsing methods. For
initSafeBrowsing(), the SuppressWarnings and TargetApi annotations are
leftover from when this method relied on reflection. For other,
maybeEnableSafeBrowsingFromManifest, the SuppressWarnings annotation was
similarly leftover from reflection.

Bug:  682070 
Test: N/A
Change-Id: I828dc570217742c4d052e3fb5ef16c799d7c9715
Reviewed-on: https://chromium-review.googlesource.com/991162
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547739}
[modify] https://crrev.com/a5c3304e0007f5c492139b2ad152462427d3515b/android_webview/java/src/org/chromium/android_webview/AwContentsStatics.java
[modify] https://crrev.com/a5c3304e0007f5c492139b2ad152462427d3515b/android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java