New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 816506 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 781754



Sign in to add a comment

Add support in chromium for static WebView methods

Project Member Reported by gsennton@chromium.org, Feb 26 2018

Issue description

Buganizer part: b/73151403
 
Cc: ntfschr@chromium.org
I just tried to upload a patch for this and got a presubmit error for trying to use android.webkit.ValueCallback outside of the webkit glue :)
We added that presubmit error here:
https://chromium.googlesource.com/chromium/src/+/535972b3b509dd28b58982b4a150a44435a87956

Given that ValueCallback is a very simple class that shouldn't be changed in the future, maybe we should allow using ValueCallback in the support library? (to avoid having to use an InvocationHandler).
Nate, WDYT?


Project Member

Comment 2 by bugdroid1@chromium.org, Feb 27 2018

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

commit 4cadbe42f5180aa81a32d4c6c3e416b6f3042b1b
Author: Gustav Sennton <gsennton@google.com>
Date: Tue Feb 27 16:31:47 2018

[WebView] Add support library support for WebViewFactoryProvider.Statics

Static WebView methods are implemented using the class
WebViewFactoryProvider.Statics, in this CL we implement the
corresponding class for the WebView support library.

Bug:  816506 
Change-Id: I3909a8f1b78a79ed06895da0611ca643231e1716
Reviewed-on: https://chromium-review.googlesource.com/939391
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Gustav Sennton <gsennton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539453}
[modify] https://crrev.com/4cadbe42f5180aa81a32d4c6c3e416b6f3042b1b/android_webview/glue/java/src/com/android/webview/chromium/CallbackConverter.java
[modify] https://crrev.com/4cadbe42f5180aa81a32d4c6c3e416b6f3042b1b/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumAwInit.java
[modify] https://crrev.com/4cadbe42f5180aa81a32d4c6c3e416b6f3042b1b/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
[modify] https://crrev.com/4cadbe42f5180aa81a32d4c6c3e416b6f3042b1b/android_webview/glue/java/src/com/android/webview/chromium/WebkitToSharedGlueConverter.java
[modify] https://crrev.com/4cadbe42f5180aa81a32d4c6c3e416b6f3042b1b/android_webview/support_library/boundary_interfaces/BUILD.gn
[add] https://crrev.com/4cadbe42f5180aa81a32d4c6c3e416b6f3042b1b/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/StaticsBoundaryInterface.java
[modify] https://crrev.com/4cadbe42f5180aa81a32d4c6c3e416b6f3042b1b/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/WebViewProviderFactoryBoundaryInterface.java
[modify] https://crrev.com/4cadbe42f5180aa81a32d4c6c3e416b6f3042b1b/android_webview/support_library/java/src/org/chromium/support_lib_glue/SupportLibWebViewChromiumFactory.java

Comment 3 by torne@chromium.org, Feb 27 2018

I think ValueCallback is fine since we know it's always there. Maybe the presubmit needs to have a whitelist.
Oh crap, now I committed a CL that breaks presubmit... will this break presubmit for others who try to upload a CL?

Comment 5 by torne@chromium.org, Feb 27 2018

Depends how the presubmit is implemented - probably only if they touch the same file, or possibly even only the same line.
Looks like it will just whine if we touch the same file:

+  sources = lambda affected_file: input_api.FilterSourceFile(
+      affected_file,
+      black_list=(_EXCLUDED_PATHS +
+                  _TEST_CODE_EXCLUDED_PATHS +
+                  input_api.DEFAULT_BLACK_LIST +
+                  (r'^android_webview[\\\/]glue[\\\/].*',)),
+      white_list=(r'.*\.java$',))
+
+  for f in input_api.AffectedSourceFiles(sources):
+    for line_num, line in f.ChangedContents():
+      if valuecallback_import_pattern.search(line):
+        errors.append("%s:%d" % (f.LocalPath(), line_num))
+
+  results = []
I added the presubmit before we decided on an augmenting support lib. The original plan was "disallow all android.webkit.* inside the chromium layer, and the support lib will have compat-equivalents of every class." Because ValueCallback is always there, it's safe to remove the presubmit.

Alternative plans of attack:

 - whitelist the support lib glue and boundary interfaces (as you suggested)
 - disallow android_webview/java, android_webview/javatests/, content/, and some other folders explicitly
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 6 2018

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

commit cc82b2cb5c9e356f3a5502de77b17a2133a64d9f
Author: Gustav Sennton <gsennton@google.com>
Date: Fri Apr 06 17:13:53 2018

[android webview] Add feature flags for lots of APIs

We need to guard support library method calls with feature flags to
ensure the methods are only called on webview / support library version
where they're supported.
In this CL we add feature flags for various APIs related to
WebSettings, SafeBrowsing, and ServiceWorkers.

Bug:  828612 
Bug:  819595 
Bug:  816506 
Change-Id: Ice5f5645040ab7316ef90322812ca6319e8a2351
Reviewed-on: https://chromium-review.googlesource.com/995933
Commit-Queue: Gustav Sennton <gsennton@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548828}
[modify] https://crrev.com/cc82b2cb5c9e356f3a5502de77b17a2133a64d9f/android_webview/support_library/boundary_interfaces/src/org/chromium/support_lib_boundary/util/Features.java
[modify] https://crrev.com/cc82b2cb5c9e356f3a5502de77b17a2133a64d9f/android_webview/support_library/java/src/org/chromium/support_lib_glue/SupportLibWebViewChromiumFactory.java

Status: Fixed (was: Assigned)

Sign in to add a comment