New issue
Advanced search Search tips

Issue 782525 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocked on:
issue 782848



Sign in to add a comment

Upstream WebView's Android-OMR1-specific code

Project Member Reported by ntfschr@chromium.org, Nov 8 2017

Issue description

See also  issue 756653  (the bug for O)

The SDK is public, so we can upstream our OMR1-specific code as soon as upstream targets OMR1 by default (glue layer can't import OMR1-specific classes).

John, do we use OMR1 upstream yet? If not, is there a bug I can follow?

---

This is orthogonal from Torne's tarball suggestion in  issue 756653 , this is to track the actual upstreaming work (although we can also consider following his suggestion).
 

Comment 1 by torne@chromium.org, Nov 8 2017

Labels: -Pri-2 -M-65 M-64 Pri-1
We should be targeting M64 for this, and getting this done before the branch point near the end of november.
Blockedon: 782848
Talked to torne@ about this offline this morning, but no, we don't use OMR1 upstream yet. Just filed a bug for it over in  issue 782848 .
Are we allowed to land this after feature freeze? If so, then I agree M64 is the right target. Torne, are we allowed to upstream the OMR1 system SDK?

Comment 4 by torne@chromium.org, Nov 8 2017

We are already shipping this code, so it's not in any way a new feature - this is just making public the exact code our shipping binaries already contain.

Yes, we can publish the system SDK (though we should make sure that we do so from the same build that the public O MR1 SDK is built from, which may or may not be the current SDK we actually have checked in downstream)
Labels: ReleaseBlock-Beta
Owner: ntfschr@chromium.org
Status: Assigned (was: Available)
I'll take this for now (most of the OMR1 code is mine). I'll wait on the public SDK 27 roll before starting.
Please note that M64 Beta coming in 2 weeks. Any update on this ?

We're still blocked, but the CLs are starting to land. I expect to be unblocked in time to land CLs next week before branch.
Status: Started (was: Assigned)
Cc: changwan@chromium.org
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 30 2017

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

commit 704813b0f826f3d7d977cae9000b72443e3748e5
Author: Nate Fischer <ntfschr@chromium.org>
Date: Wed Nov 29 23:59:15 2017

AW: upstream onSafeBrowsingHit (OMR1 API)

This copies the implementation of
WebViewContentsClientAdapter#onSafeBrowsingHit from
WebViewContentsClientAdapterForOMR1. The new implementation is still
unused until WebViewChromiumFactoryProviderForOMR1 moves upstream (in a
separate CL).

Bug:  782525 
Test: ninja system_webview_google_apk (it compiles with the new SDK)
Change-Id: I8993474e38bc982c5f34a9b1a3143c19578bc077
Reviewed-on: https://chromium-review.googlesource.com/797492
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520310}
[modify] https://crrev.com/704813b0f826f3d7d977cae9000b72443e3748e5/android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 30 2017

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

commit 232de28411d92dfc320c01b64d088eea4b8f32e6
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Nov 30 00:32:30 2017

AW: upstream WebViewChromiumFactoryProviderForOMR1

See downstream counterpart: http://crrev/i/517972

This adds WebViewChromiumFactoryProviderForOMR1 upstream (it inherits
directly from WebViewChromiumFactoryProvider, since *ForO is empty).

Bug:  782525 
Test: install system_webview_apk & monochrome_public_apk on a custom AOSP build as WebView providers
Change-Id: I3a93bb479ce403e134bd32d3cfe084f0a86c1c92
Reviewed-on: https://chromium-review.googlesource.com/797417
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520338}
[modify] https://crrev.com/232de28411d92dfc320c01b64d088eea4b8f32e6/android_webview/glue/BUILD.gn
[add] https://crrev.com/232de28411d92dfc320c01b64d088eea4b8f32e6/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProviderForOMR1.java

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 30 2017

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

commit d2aeddb09746b3507f757ca8e5a0062965b0b60e
Author: Nate Fischer <ntfschr@google.com>
Date: Thu Nov 30 00:44:23 2017

Update: the only change left should be to bump the target SDK (see http://crrev/c/797946). I'll land that shortly after double-checking all WebView/Monochrome APKs.
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 30 2017

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

commit 44012e21ff6710e9c812a4c8805ae16568d60d02
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Nov 30 01:32:59 2017

AW: bump target SDK for AOSP WebView

This is another requirement to be an OMR1 WebView provider.

Bug:  782525 
Test: build AOSP WebView, install, and set as WebView provider (no longer rejected)
Change-Id: Idb43fa38cbefa134315d2b7e57249f8b30ff9d4f
Reviewed-on: https://chromium-review.googlesource.com/797946
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520376}
[modify] https://crrev.com/44012e21ff6710e9c812a4c8805ae16568d60d02/android_webview/apk/java/AndroidManifest.xml

Status: Fixed (was: Started)
Alright, should be fixed!
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-64; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-64 label, otherwise remove Merge-TBD label. Thanks.
Labels: -Merge-TBD
I don't think I missed the branch. I double-checked each of my chromium CLs, they all made M64. I assume the downstream one did as well (I don't have a branch number to check with).
Branch is 3282 and your CLs did make it (you should wait until it's officially announced to check your CLs are in there). :)
Status: Verified (was: Fixed)
Bulk edit: marking stale 'fixed' bugs as 'verified' since they don't need verification at this point.

Sign in to add a comment