Remove code specific to pre-PlzNavigate code path |
|||
Issue descriptionI 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
,
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?
,
Mar 15 2018
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!
,
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
,
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
,
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
,
Mar 17 2018
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?
,
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.
,
Mar 17 2018
Ok thanks, I'll send out a CL and test it against the repro steps from that bug.
,
Mar 19 2018
Please add the manual verification steps to verify the commit changes.
,
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
,
Mar 20 2018
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 |
|||
Comment 1 by ntfschr@chromium.org
, Mar 15 2018