aw: Delay onPageStarted until navigation commit part 2

This CL moves onPageStarted to page commit for browser navigations.
However the old code path is not removed since we still do not want to
enable this for old versions of GMSCore. Add a kill switch feature
that's disabled by default and a test finch config to enable it for
testing.

Bug: 896022
Change-Id: I7cd0777687ca4be096209a3a0cfed8dba4e5bd13
Reviewed-on: https://chromium-review.googlesource.com/c/1327170Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607466}

android_webview/browser/aw_feature_list.cc

@@ -24,6 +24,7 @@ namespace {
 // in other locations in the code base (e.g. content/, components/, etc).
 const base::Feature* kFeaturesExposedToJava[] = {
     &features::kWebViewConnectionlessSafeBrowsing,
+    &features::kWebViewPageStartedOnCommit,
 };
 
 const base::Feature* FindFeatureExposedToJava(const std::string& feature_name) {
@@ -47,6 +48,11 @@ namespace features {
 const base::Feature kWebViewConnectionlessSafeBrowsing{
     "WebViewConnectionlessSafeBrowsing", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Kill switch for feature to call onPageFinished for browser-initiated
+// navigations when the navigation commits.
+const base::Feature kWebViewPageStartedOnCommit{
+    "WebViewPageStartedOnCommit", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Whether the application package name is logged in UMA.
 const base::Feature kWebViewUmaLogAppPackageName{
     "WebViewUmaLogAppPackageName", base::FEATURE_DISABLED_BY_DEFAULT};

android_webview/browser/aw_feature_list.h

@@ -15,6 +15,7 @@ namespace features {
 
 // Alphabetical:
 extern const base::Feature kWebViewConnectionlessSafeBrowsing;
+extern const base::Feature kWebViewPageStartedOnCommit;
 extern const base::Feature kWebViewUmaLogAppPackageName;
 
 }  // namespace features

android_webview/java/src/org/chromium/android_webview/AwContents.java

@@ -640,7 +640,7 @@ public class AwContents implements SmartClipProvider {
             // The shouldOverrideUrlLoading call might have resulted in posting messages to the
             // UI thread. Using sendMessage here (instead of calling onPageStarted directly)
             // will allow those to run in order.
-            if (!navigationParams.isRendererInitiated) {
+            if (!AwFeatureList.pageStartedOnCommitEnabled(navigationParams.isRendererInitiated)) {
                 mContentsClient.getCallbackHelper().postOnPageStarted(navigationParams.url);
             }
             return false;

android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java

@@ -301,7 +301,8 @@ public class AwContentsClientBridge {
             } else {
                 error.errorCode = ErrorCodeConversionHelper.convertErrorCode(error.errorCode);
             }
-            if (request.isMainFrame && isRendererInitiated) {
+            if (request.isMainFrame
+                    && AwFeatureList.pageStartedOnCommitEnabled(isRendererInitiated)) {
                 mClient.getCallbackHelper().postOnPageStarted(request.url);
             }
             mClient.getCallbackHelper().postOnReceivedError(request, error);

android_webview/java/src/org/chromium/android_webview/AwFeatureList.java

@@ -4,6 +4,10 @@
 
 package org.chromium.android_webview;
 
+import android.content.pm.PackageInfo;
+import android.content.pm.PackageManager;
+
+import org.chromium.base.ContextUtils;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.MainDex;
 
@@ -16,6 +20,36 @@ final public class AwFeatureList {
     // Do not instantiate this class.
     private AwFeatureList() {}
 
+    private static final String GMS_PACKAGE = "com.google.android.gms";
+
+    private static Boolean sPageStartedOnCommitForBrowserNavigations;
+
+    private static boolean computePageStartedOnCommitForBrowserNavigations() {
+        if (!nativeIsEnabled(WEBVIEW_PAGE_STARTED_ON_COMMIT)) return false;
+        if (GMS_PACKAGE.equals(ContextUtils.getApplicationContext().getPackageName())) {
+            try {
+                PackageInfo gmsPackage =
+                        ContextUtils.getApplicationContext().getPackageManager().getPackageInfo(
+                                GMS_PACKAGE, 0);
+                return gmsPackage.versionCode >= 15000000;
+            } catch (PackageManager.NameNotFoundException e) {
+            }
+            return false;
+        }
+        return true;
+    }
+
+    public static boolean pageStartedOnCommitEnabled(boolean isRendererInitiated) {
+        // Always enable for renderer-initiated navigations.
+        if (isRendererInitiated) return true;
+        if (sPageStartedOnCommitForBrowserNavigations != null) {
+            return sPageStartedOnCommitForBrowserNavigations;
+        }
+        sPageStartedOnCommitForBrowserNavigations =
+                computePageStartedOnCommitForBrowserNavigations();
+        return sPageStartedOnCommitForBrowserNavigations;
+    }
+
     /**
      * Returns whether the specified feature is enabled or not.
      *
@@ -32,6 +66,7 @@ final public class AwFeatureList {
     // Alphabetical:
     public static final String WEBVIEW_CONNECTIONLESS_SAFE_BROWSING =
             "WebViewConnectionlessSafeBrowsing";
+    public static final String WEBVIEW_PAGE_STARTED_ON_COMMIT = "WebViewPageStartedOnCommit";
 
     private static native boolean nativeIsEnabled(String featureName);
 }

android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java

@@ -107,7 +107,8 @@ public class AwWebContentsObserver extends WebContentsObserver {
         if (client != null) {
             // OnPageStarted is not called for fragment navigations.
             // Error page is handled by AwContentsClientBridge.onReceivedError.
-            if (!isFragmentNavigation && !isErrorPage && isRendererInitiated) {
+            if (!isFragmentNavigation && !isErrorPage
+                    && AwFeatureList.pageStartedOnCommitEnabled(isRendererInitiated)) {
                 client.getCallbackHelper().postOnPageStarted(url);
             }
 

android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageStartedTest.java

@@ -285,19 +285,21 @@ public class ClientOnPageStartedTest {
         int hangingRequestCount = mHangingRequestCallbackHelper.getCallCount();
         mActivityTestRule.loadUrlAsync(mAwContents, mHangingUrl);
 
-        // onPageStarted should be called, but not onPageFinished.
+        // onPageStarted and onPageFinished should not be called yet.
         mContentsClient.getOnLoadResourceHelper().waitForCallback(onLoadResourceCount);
         mHangingRequestCallbackHelper.waitForCallback(hangingRequestCount);
-        mContentsClient.getOnPageStartedHelper().waitForCallback(pageStartedCount);
         Assert.assertEquals(
                 mHangingUrl, mContentsClient.getOnLoadResourceHelper().getLastLoadedResource());
-        Assert.assertEquals(mHangingUrl, mContentsClient.getOnPageStartedHelper().getUrl());
+        Assert.assertEquals(
+                pageStartedCount, mContentsClient.getOnPageStartedHelper().getCallCount());
         Assert.assertEquals(
                 pageFinishedCount, mContentsClient.getOnPageFinishedHelper().getCallCount());
 
         // Release request on server. Should get onPageStarted/Finished after.
         mHangingRequestSemaphore.release();
+        mContentsClient.getOnPageStartedHelper().waitForCallback(pageStartedCount);
         mContentsClient.getOnPageFinishedHelper().waitForCallback(pageFinishedCount);
+        Assert.assertEquals(mHangingUrl, mContentsClient.getOnPageStartedHelper().getUrl());
         Assert.assertEquals(mHangingUrl, mContentsClient.getOnPageFinishedHelper().getUrl());
     }
 
@@ -356,24 +358,26 @@ public class ClientOnPageStartedTest {
         int hangingRequestCount = mHangingRequestCallbackHelper.getCallCount();
         mActivityTestRule.loadUrlAsync(mAwContents, mRedirectToHangingUrl);
 
-        // onPageStarted should be called, but not onPageFinished.
+        // onPageStarted and onPageFinished should not be called yet.
         mContentsClient.getShouldOverrideUrlLoadingHelper().waitForCallback(
                 shouldOverrideUrlLoadingCount);
         mContentsClient.getOnLoadResourceHelper().waitForCallback(onLoadResourceCount);
         mHangingRequestCallbackHelper.waitForCallback(hangingRequestCount);
-        mContentsClient.getOnPageStartedHelper().waitForCallback(pageStartedCount);
         Assert.assertEquals(mHangingUrl,
                 mContentsClient.getShouldOverrideUrlLoadingHelper()
                         .getShouldOverrideUrlLoadingUrl());
         Assert.assertEquals(mRedirectToHangingUrl,
                 mContentsClient.getOnLoadResourceHelper().getLastLoadedResource());
-        Assert.assertEquals(mHangingUrl, mContentsClient.getOnPageStartedHelper().getUrl());
+        Assert.assertEquals(
+                pageStartedCount, mContentsClient.getOnPageStartedHelper().getCallCount());
         Assert.assertEquals(
                 pageFinishedCount, mContentsClient.getOnPageFinishedHelper().getCallCount());
 
         // Release request on server. Should get onPageStarted/Finished after.
         mHangingRequestSemaphore.release();
+        mContentsClient.getOnPageStartedHelper().waitForCallback(pageStartedCount);
         mContentsClient.getOnPageFinishedHelper().waitForCallback(pageFinishedCount);
+        Assert.assertEquals(mHangingUrl, mContentsClient.getOnPageStartedHelper().getUrl());
         Assert.assertEquals(mHangingUrl, mContentsClient.getOnPageFinishedHelper().getUrl());
     }
 }

android_webview/tools/cts_config/webview_cts_gcs_path.json

@@ -37,10 +37,51 @@
       {
         "apk": "android-cts/repository/testcases/CtsWebkitTestCases.apk",
         "excludes": [
-        {
-          "match": "android.webkit.cts.WebViewClientTest#testShouldOverrideUrlLoadingOnCreateWindow",
-          "_bug_id": "crbug.com/896857"
-        }]
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testDoUpdateVisitedHistory",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testLoadPage",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnFormResubmission",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedError",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedErrorForSubresource",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedHttpAuthRequest",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedHttpError",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedLoginRequest",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnScaleChanged",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testShouldOverrideUrlLoading",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testShouldOverrideUrlLoadingOnCreateWindow",
+            "_bug_id": "crbug.com/896022"
+          }
+        ]
       },
       {
         "apk": "android-cts/repository/testcases/CtsWidgetTestCases.apk",
@@ -58,10 +99,51 @@
       {
         "apk": "android-cts/repository/testcases/CtsWebkitTestCases.apk",
         "excludes": [
-        {
-          "match": "android.webkit.cts.WebViewClientTest#testShouldOverrideUrlLoadingOnCreateWindow",
-          "_bug_id": "crbug.com/896857"
-        }]
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testDoUpdateVisitedHistory",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testLoadPage",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnFormResubmission",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedError",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedErrorForSubresource",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedHttpAuthRequest",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedHttpError",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnReceivedLoginRequest",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testOnScaleChanged",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testShouldOverrideUrlLoading",
+            "_bug_id": "crbug.com/896022"
+          },
+          {
+            "match": "android.webkit.cts.WebViewClientTest#testShouldOverrideUrlLoadingOnCreateWindow",
+            "_bug_id": "crbug.com/896022"
+          }
+        ]
       },
       {
         "apk": "android-cts/repository/testcases/CtsWidgetTestCases.apk",

testing/variations/fieldtrial_testing_config.json

@@ -477,6 +477,21 @@
             ]
         }
     ],
+    "AndroidWebViewPageStartedOnCommit": [
+        {
+            "platforms": [
+                "android_webview"
+            ],
+            "experiments": [
+                {
+                    "name": "Enabled",
+                    "enable_features": [
+                        "WebViewPageStartedOnCommit"
+                    ]
+                }
+            ]
+        }
+    ],
     "AppLauncherRefresh": [
         {
             "platforms": [