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

Issue 822124 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Remove code specific to pre-PlzNavigate code path

Project Member Reported by ntfschr@chromium.org, Mar 15 2018

Issue description

I believe we no longer support the old code path. We can simplify the code base by removing some PlzNavigate-specific code.

Things that come to mind:

 * Java's isBrowserSideNavigationEnabled() [1]
 * extra code for shouldOverrideUrlLoading and loadDataWithBaseURL's onPageStarted [2]
 * reenable AwContentsClientShouldInterceptRequestTest#testLoadDataWithBaseUrlTriggersShouldInterceptRequest

[1] https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content_public/common/BrowserSideNavigationPolicy.java?q=lang:java+isbrowsersidenav&sq=package:chromium&l=14
[2] https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwContents.java?q=onpagestarted+f:awcontents.java&sq=package:chromium&l=612
 
Cc: clamy@chromium.org jam@chromium.org nasko@chromium.org
Hmm maybe that's not quite true, this tester appears to still be active: https://ci.chromium.org/buildbot/chromium.android/Marshmallow%2064%20bit%20Tester/

PlzNavigate team, when should we expect this to become unsupported?

Comment 2 by jam@chromium.org, Mar 15 2018

a lot of non-plznavigate codepath has been removed already, so it's non functional.

I'm not sure what you mean by that link, that bot was one where we added the plznavigate path but I don't see the old one running on it?
Ah, I see. We previously tried to remove code for the old codepath and saw  issue 794117 . I misread that bug: the problem was not the bot, but the "renderer_side_navigation_webview_instrumentation_test_apk" target. It sounds like that target has since been removed. Thanks!
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 15 2018

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

commit 6ba50de74bbbed1ccda6e850b0698cfec172690a
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Mar 15 17:48:49 2018

AW: enable PlzNavigate-specific test

This was originally disabled because we still ran tests against the old
code path. That's no longer supported, so we can safely reenable the
test.

This also turns a comment into a proper TODO, since it shouldn't be left
too long.

Bug:  822124 
Bug:  769126 
Test: run_webview_instrumentation_test_apk -f AwContentsClientShouldInterceptRequestTest#testLoadDataWithBaseUrlTriggersShouldInterceptRequest
Change-Id: I0947560b44c565edb3be2966e121d916b003ed01
Reviewed-on: https://chromium-review.googlesource.com/963647
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543434}
[modify] https://crrev.com/6ba50de74bbbed1ccda6e850b0698cfec172690a/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 16 2018

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

commit 6ba97d0b9c69cee7cc85a0bcd05277c5a9b99c64
Author: Nate Fischer <ntfschr@chromium.org>
Date: Fri Mar 16 01:26:00 2018

AW: remove legacy non-PlzNavigate code for WebView

No change to behavior, this removes no-longer used code paths.

The android_webview/ code only existed to support the old code path.

This removes BrowserSideNavigationPolicy now that
isBrowserSideNavigationEnabled() is guaranteed to return true.

Bug:  822124 
Test: N/A
Change-Id: I8d1fb406f572b1fefaac70853140dad64bc5883f
Reviewed-on: https://chromium-review.googlesource.com/963649
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543590}
[modify] https://crrev.com/6ba97d0b9c69cee7cc85a0bcd05277c5a9b99c64/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/6ba97d0b9c69cee7cc85a0bcd05277c5a9b99c64/content/common/BUILD.gn
[delete] https://crrev.com/fd3cd0f93df8338bd143536cfdd122e56a497640/content/common/android/browser_side_navigation_policy_android.cc
[modify] https://crrev.com/6ba97d0b9c69cee7cc85a0bcd05277c5a9b99c64/content/public/android/BUILD.gn
[delete] https://crrev.com/fd3cd0f93df8338bd143536cfdd122e56a497640/content/public/android/java/src/org/chromium/content_public/common/BrowserSideNavigationPolicy.java

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 16 2018

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

commit f87cca5c9ceae85dbd39c92d51d55a674209d22d
Author: Nate Fischer <ntfschr@chromium.org>
Date: Fri Mar 16 20:56:29 2018

AW: modify test for PlzNavigate

No change to production code.

This test was previously modified to anticipate the behavior change for
loadDataWithBaseURL (namely, that we now call shouldInterceptRequest)
caused by PlzNavigate, with backwards compatibility for the old code
path.

This removes the support for the old code path, and asserts the new
behavior. As with http://crrev/c/820717, this leaves a TODO to actually
assert the URL intercepted for the main resource, since this value is
not yet finalized.

Bug:  822124 
Test: run_webview_instrumentation_test_apk -f AwContentsClientShouldInterceptRequestTest#testBaseUrlOfLoadDataSentInRefererHeader
Change-Id: I315e1c0c310cda116603ee4026e15d89947760df
Reviewed-on: https://chromium-review.googlesource.com/967082
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543827}
[modify] https://crrev.com/f87cca5c9ceae85dbd39c92d51d55a674209d22d/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java

Cc: mea...@chromium.org
The last bit should be https://cs.chromium.org/chromium/src/android_webview/browser/aw_browser_main_parts.cc?l=136&rcl=0b03fd176af9977ec83548ab8b87b0456a028fe6

meacer@ is it appropriate to remove that line (and the accompanying code in content layer) now?

Comment 8 by mea...@chromium.org, Mar 17 2018

It's something I've been meaning to remove, but I haven't had a chance to verify if it'll still work. If the PoC in  bug 732976  works after removing it, then yes it should be fine.
Ok thanks, I'll send out a CL and test it against the repro steps from that bug.
Please add the manual verification steps to verify the commit changes.
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 20 2018

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

commit c5fab0fb6d5c175faa887762864a14e39a76df44
Author: Nate Fischer <ntfschr@chromium.org>
Date: Tue Mar 20 21:52:23 2018

AW: remove AllowDataUrlNavigationForAndroidWebView

No change in behavior.

This function was only necessary when PlzNavigate was disabled. Now that
it's on by default, it's no longer necessary. This removes the method
and the corresponding logic.

Bug:  822124 
Test: manual - see steps reported in  issue 732976 
Change-Id: I2527602022f3c7fc4b33518a7a04a8742b3b0a6d
Reviewed-on: https://chromium-review.googlesource.com/967547
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544541}
[modify] https://crrev.com/c5fab0fb6d5c175faa887762864a14e39a76df44/android_webview/browser/aw_browser_main_parts.cc
[modify] https://crrev.com/c5fab0fb6d5c175faa887762864a14e39a76df44/content/browser/frame_host/data_url_navigation_throttle.cc
[modify] https://crrev.com/c5fab0fb6d5c175faa887762864a14e39a76df44/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/c5fab0fb6d5c175faa887762864a14e39a76df44/content/public/browser/render_frame_host.h

Status: Verified (was: Assigned)
No manual verification necessary. None of these changes affect live code paths. I locally confirmed the steps in  issue 732976  just as an extra precaution, but the code I touched has been out-of-use since PlzNavigate was enabled.

Sign in to add a comment