Nate, did you have any thoughts on this?

I think the most straight-forward approach is approach B from the document linked above. I.e. to only include new APIs (since L) in the support library WebViewClient, and then have the support library client take precedence for those APIs.
The only issue with that approach is potential bugs for the app developer - they might forget/miss that a certain callback is considered new, and thus forget to put their implementation of that callback on the support library client instead of the android.webkit client.
If we could solve that by some kind of lint check that would be great, but I don't know if that's feasible.

Actually, Nate, do you feel like looking into this? Otherwise I'll do it.
It would be great to get this resolved asap so that we can implement the new glue layer.

Yeah, I can handle this.

Here's how I envision this:

 - when the app calls setWebViewClientCompat(), we'll set the WebViewClientCompat in glue2, and set a variable in glue-common (or AW-layer)
 (hasWebViewClientCompat) to true
 - when we invoke a callback like onSafeBrowsingHit:

   if (hasWebViewClientCompat)
     // call onSafeBrowsingHit in glue
   else
     // call onSafeBrowsingHit in glue2 (support lib)

---

Each time the app re-sets the WebViewClientCompat, we need to do a null-check and possibly modify hasWebViewClientCompat.

---

This is a pretty simple design, but I think we can enhance it later if needed. A possible later enhancement is as you've suggested in the doc, to call the callback whether it's on WebViewClient or WebViewClientCompat.

I think that sounds fine (with the exception that your if/else statement in comment #4 is reversed ;)).

I think the addition of calling the callback on different clients depending on whether it is defined, is a bit weird / complex, I would prefer just warning developers if they happen to implement a method on the old client which exists on the new client as well (if that's feasible).

As you say I think we can start with the approach you describe, and potentially add any developer-help later if it turns out this is actually causing bugs.

> with the exception that your if/else statement in comment #4 is reversed

Whoops!

> As you say I think we can start with the approach you describe, and potentially add any developer-help later if it turns out this is actually causing bugs.

Sounds good. I see that interfaces are in-progress, but we still have no implementation (so there's currently nothing to pass to setWebViewClientCompat, for example). I think we can still get meaningful development by using a flag which sets `hasWebViewClientCompat = true`, and logging instead of invoking the real WebViewClientCompat methods.

That should let us finish 90% of the code while we wait on implementations.

Sounds like a plan :)

I think the only other app-callback API that is new since L is ServiceWorkerClient - we should probably use a similar approach for that.

A note: there are some decisions still to be made about the actual implementation of the support library <-> glue communication, so it's nice that we can go ahead with the WebViewClient stuff without depending on that implementation.


Do you think it would make sense to add a couple of tests in the style of:
1. setting an old client, 2. setting a new client, 3. invoking a new callback, 4. assertThat(the callback goes to the new client).

> Do you think it would make sense to add a couple of tests in the style of:

Yeah, that sounds ok. I'll take a look at what we can do here.

Doc (internal only for now) is up at http://go/wv-support-library-callbacks

I'll get a prototype up ASAP.

A functional prototype is at: https://chromium-review.googlesource.com/c/chromium/src/+/965883

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

commit 2636fba313a00683509bc25cf4e79ad1f917ee49
Author: Nate Fischer <ntfschr@chromium.org>
Date: Fri Mar 23 20:53:53 2018

AW: add WebViewClient boundary interface

No change to logic.

This adds WebViewClientBoundaryInterface, which will be implemented by
WebViewClientCompat on the support library side.

These methods will be invoked from the support_library side in later
CLs, as we implement each callback.

Bug:  781764 
Test: N/A
Change-Id: I2ba052ff02076cec1c9d4e360aed322a63b511ef
Reviewed-on: https://chromium-review.googlesource.com/974898
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Gustav Sennton <gsennton@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545575}
[modify] https://crrev.com/2636fba313a00683509bc25cf4e79ad1f917ee49/android_webview/support_library/boundary_interfaces/BUILD.gn
[add] https://crrev.com/2636fba313a00683509bc25cf4e79ad1f917ee49/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/WebViewClientBoundaryInterface.java

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

commit b73c062f19bf41d9f50c405302b9bdfe419b554d
Author: Nate Fischer <ntfschr@chromium.org>
Date: Wed Mar 28 20:45:49 2018

AW: add boundary interfaces for new classes used in callbacks

This adds boundary interfaces for SafeBrowsingResponse and
WebResourceError, both of which are post-L classes added in
WebViewClient callbacks (onSafeBrowsingHit() and onReceivedError()).

Design doc: http://go/wv-support-library-callbacks

Bug:  781764 
Test: N/A
Change-Id: Idcf1b4af21b197f951192b5a262f63ec0500debe
Reviewed-on: https://chromium-review.googlesource.com/982651
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546590}
[modify] https://crrev.com/b73c062f19bf41d9f50c405302b9bdfe419b554d/android_webview/support_library/boundary_interfaces/BUILD.gn
[add] https://crrev.com/b73c062f19bf41d9f50c405302b9bdfe419b554d/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/SafeBrowsingResponseBoundaryInterface.java
[add] https://crrev.com/b73c062f19bf41d9f50c405302b9bdfe419b554d/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/WebResourceErrorBoundaryInterface.java

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

commit f3b8b74e8f237e377d65145879790dc5a222fb9b
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Apr 05 20:18:15 2018

AW: implement support lib callbacks

This implements support library callbacks (WebViewclientCompat). This
creates a new layer (support_library/callback) which the glue layer
depends on. This dependency lets us instantiate a
SupportLibWebViewContentsClientAdapter inside setWebViewClient(), and
benefit from the glue layer's parameter cleanup code (e.g., in
onReceivedError2()).

The support_library/callback glue must be a separate layer from
support_library/ glue, as that already depends on the webkit glue to
initiate state. Implementing callbacks as a separate target avoids the
circular dependency.

This refactors the (post-L) glue layer callback methods to take the
following precedence:

 1. SupportLibWebViewContentsClientAdapter (if it supports the callback)
 2. WebViewClient (if on the appropriate platform level)
 3. Default behavior (implementation provided by the glue layer)

This implements both category 1 and 2 APIs.

Design doc: http://go/wv-support-library-callbacks

Bug:  781764 
Test: manual - built test application with latest support-lib changes
Change-Id: I21e28493873e670cfd428c7eeef12b0e212aeec4
Reviewed-on: https://chromium-review.googlesource.com/989015
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Gustav Sennton <gsennton@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548538}
[modify] https://crrev.com/f3b8b74e8f237e377d65145879790dc5a222fb9b/android_webview/glue/glue.gni
[modify] https://crrev.com/f3b8b74e8f237e377d65145879790dc5a222fb9b/android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java
[modify] https://crrev.com/f3b8b74e8f237e377d65145879790dc5a222fb9b/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/util/Features.java
[add] https://crrev.com/f3b8b74e8f237e377d65145879790dc5a222fb9b/android_webview/support_library/callback/BUILD.gn
[add] https://crrev.com/f3b8b74e8f237e377d65145879790dc5a222fb9b/android_webview/support_library/callback/java/src/org/chromium/support_lib_callback_glue/SupportLibWebViewContentsClientAdapter.java

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

commit bc64fe05b894458c95224232155fdda50d097a53
Author: Nate Fischer <ntfschr@chromium.org>
Date: Wed Apr 11 16:35:39 2018

AW: rename WEB_RESOURCE_ERROR feature name

No change to logic. This simply renames a feature name to be more
explicit about what it stands for.

WEB_RESOURCE_ERROR -> RECEIVE_WEB_RESOURCE_ERROR

Bug:  781764 
Test: N/A
Change-Id: I1bda04b8fb8d80d9ac28e633b9d94b6428b6801d
Reviewed-on: https://chromium-review.googlesource.com/1006178
Reviewed-by: Gustav Sennton <gsennton@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549908}
[modify] https://crrev.com/bc64fe05b894458c95224232155fdda50d097a53/android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java
[modify] https://crrev.com/bc64fe05b894458c95224232155fdda50d097a53/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/util/Features.java
[modify] https://crrev.com/bc64fe05b894458c95224232155fdda50d097a53/android_webview/support_library/callback/java/src/org/chromium/support_lib_callback_glue/SupportLibWebViewContentsClientAdapter.java

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

commit 71d3b4c3b22d383840673c9b0ec562c679fabb6a
Author: Nate Fischer <ntfschr@chromium.org>
Date: Wed Apr 11 22:36:23 2018

AW: add feature detection for WebViewClientCompat

With this CL, we only plumb WebViewClient callbacks to the
support_library glue code if the WebViewClientCompat supports the
relevant Feature.

This lets WebViewClientBoundaryInterface extend
FeatureFlagHolderBoundaryInterface, because WebViewClientCompat is only
instantiated on the support-lib side.

The corresponding Android change is http://ag/3877212.

Bug:  781764 
Test: Manual
Change-Id: I66f23e18fabd013c5b4872f3310819608a78394c
Reviewed-on: https://chromium-review.googlesource.com/1006051
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Gustav Sennton <gsennton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549959}
[modify] https://crrev.com/71d3b4c3b22d383840673c9b0ec562c679fabb6a/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/WebViewClientBoundaryInterface.java
[modify] https://crrev.com/71d3b4c3b22d383840673c9b0ec562c679fabb6a/android_webview/support_library/callback/java/src/org/chromium/support_lib_callback_glue/SupportLibWebViewContentsClientAdapter.java
[modify] https://crrev.com/71d3b4c3b22d383840673c9b0ec562c679fabb6a/android_webview/support_library/java/src/org/chromium/support_lib_glue/SupportLibWebViewChromiumFactory.java

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

commit abfcc4e6dffdeed209ac45f4010ab8fba2cb9546
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Apr 12 21:21:01 2018

AW: better support resetting WebViewClientCompat

This modifies SupportLibWebViewContentsClientAdapter to better support
resetting WebViewClient, without re-instantiating the adapter object,
and lets SupportLibWebViewContentsClientAdapter be 1:1 with
WebViewContentsClientAdapter.

Previously, we would re-instantiate
SupportLibWebViewContentsClientAdapter each time we re-set
WebViewClient. However, this isn't necessary, since we need only
associate the new WebViewClient with the same
SupportLibWebViewContentsClientAdapter.

This will be helpful if we ever add WebChromeClientCompat, since we can
re-set either WebViewClient/WebChromeClient without re-instantiating the
entire object.

Bug:  781764 
Test: Manual
Change-Id: Id4a099a38fda2ecc4aeed1f68f83c158f85efef2
Reviewed-on: https://chromium-review.googlesource.com/1006184
Reviewed-by: Gustav Sennton <gsennton@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550362}
[modify] https://crrev.com/abfcc4e6dffdeed209ac45f4010ab8fba2cb9546/android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java
[modify] https://crrev.com/abfcc4e6dffdeed209ac45f4010ab8fba2cb9546/android_webview/support_library/callback/java/src/org/chromium/support_lib_callback_glue/SupportLibWebViewContentsClientAdapter.java

I think the majority of WebViewClientCompat work is done, so I'll close this.

gsennton@ and I will provide manual verification steps as part of an overall test plan for the Support Library.

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

commit abfcc4e6dffdeed209ac45f4010ab8fba2cb9546
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Apr 12 21:21:01 2018

AW: better support resetting WebViewClientCompat

This modifies SupportLibWebViewContentsClientAdapter to better support
resetting WebViewClient, without re-instantiating the adapter object,
and lets SupportLibWebViewContentsClientAdapter be 1:1 with
WebViewContentsClientAdapter.

Previously, we would re-instantiate
SupportLibWebViewContentsClientAdapter each time we re-set
WebViewClient. However, this isn't necessary, since we need only
associate the new WebViewClient with the same
SupportLibWebViewContentsClientAdapter.

This will be helpful if we ever add WebChromeClientCompat, since we can
re-set either WebViewClient/WebChromeClient without re-instantiating the
entire object.

Bug:  781764 
Test: Manual
Change-Id: Id4a099a38fda2ecc4aeed1f68f83c158f85efef2
Reviewed-on: https://chromium-review.googlesource.com/1006184
Reviewed-by: Gustav Sennton <gsennton@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550362}
[modify] https://crrev.com/abfcc4e6dffdeed209ac45f4010ab8fba2cb9546/android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java
[modify] https://crrev.com/abfcc4e6dffdeed209ac45f4010ab8fba2cb9546/android_webview/support_library/callback/java/src/org/chromium/support_lib_callback_glue/SupportLibWebViewContentsClientAdapter.java

Please add the manual verification steps to verify the fix.Thanks

gsennton@ and I will provide manual verification steps as part of an overall test plan for the Support Library. Manual steps are not ready yet.

Crosslinking: the android bug is http://b/73151460

Bulk edit: marking stale 'fixed' bugs as 'verified' since they don't need verification at this point.