Upstream ShouldOverrideUrlLoading changes

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

Index: android_webview/java/src/org/chromium/android_webview/AwContents.java
diff --git a/android_webview/java/src/org/chromium/android_webview/AwContents.java b/android_webview/java/src/org/chromium/android_webview/AwContents.java
index bb4222c87c273f6e8a5a8a32fc91e7805fdd5e50..a2892becabe15f213b59be8dff361b2a102c3d47 100644
--- a/android_webview/java/src/org/chromium/android_webview/AwContents.java
+++ b/android_webview/java/src/org/chromium/android_webview/AwContents.java
@@ -195,6 +195,9 @@ public class AwContents {
 
     private AwPdfExporter mAwPdfExporter;
 
+    // This flag is to ShouldOverrideUrlNavigation through the resourcethrottle.

[boliu, 2013/12/06 20:28:23]

The var name is awesome, this comment, not so much...how about something like

Indicates shouldOverrideUrlLoading has not been called on the pending main frame
navigation of a pop up.

Also should really have a test for this path if there isn't already one.

[sgurun-gerrit only, 2013/12/07 00:11:07]

The longest variable name I have ever written. you are right the comment does
not make sense.
Have a CTS test for this path.

+    private boolean mDeferredShouldOverrideUrlLoadingIsPendingForPopup;
+
     private static final class DestroyRunnable implements Runnable {
         private final long mNativeAwContents;
         private DestroyRunnable(long nativeAwContents) {
@@ -284,62 +287,36 @@ public class AwContents {
     }
 
     //--------------------------------------------------------------------------------------------
+    // When the navigation is for a newly created WebView (i.e. a popup), intercept the navigation
+    // here for implementing shouldOverrideUrlLoading. This is to send the shouldOverrideUrlLoading
+    // callback to the correct WebViewClient that is associated with the WebView.
+    // Otherwise, use this delegate only to post onPageStarted messages. The navigations are

[boliu, 2013/12/06 20:28:23]

Otherwise, this delegate is used to post onPageStarted messages only.

And I think the rest of the paragraph after this can be deleted.

[sgurun-gerrit only, 2013/12/07 00:11:07]

Done.

+    // intercepted via handleNavigation API of ContentRendererClient and communicated via a SYNC
+    // IPC. This is to prevent cancelling XHRs that may have already started.
+    //
+    // As for posting the onPageStarted, we could have used WebContentsObserver. However using

[boliu, 2013/12/06 20:28:23]

Not using WebContentsObserverAndroid.didStartLoading because...

I think that path can be deleted after this since we were the only ones using
it. Asap so no one else adds an dependency.

[sgurun-gerrit only, 2013/12/07 00:11:07]

Done.
Let's take a look at this after this CL.

+    // it causes these problems at the time: stale URLs, out of order onPageStarted's and double
+    // onPageStarted's.
+    //
     private class InterceptNavigationDelegateImpl implements InterceptNavigationDelegate {
-        private String mLastLoadUrlAddress;
-
-        public void onUrlLoadRequested(String url) {
-            mLastLoadUrlAddress = url;
-        }
-
         @Override
         public boolean shouldIgnoreNavigation(NavigationParams navigationParams) {
             final String url = navigationParams.url;
-            final int transitionType = navigationParams.pageTransitionType;
-            final boolean isLoadUrl =
-                    (transitionType & PageTransitionTypes.PAGE_TRANSITION_FROM_API) != 0;
-            final boolean isBackForward =
-                    (transitionType & PageTransitionTypes.PAGE_TRANSITION_FORWARD_BACK) != 0;
-            final boolean isReload =
-                    (transitionType & PageTransitionTypes.PAGE_TRANSITION_CORE_MASK) ==
-                    PageTransitionTypes.PAGE_TRANSITION_RELOAD;
-            final boolean isRedirect = navigationParams.isRedirect;
-
             boolean ignoreNavigation = false;
-
-            // Any navigation from loadUrl, goBack/Forward, or reload, are considered application
-            // initiated and hence will not yield a shouldOverrideUrlLoading() callback.
-            // TODO(joth): Using PageTransitionTypes should be sufficient to determine all app
-            // initiated navigations, and so mLastLoadUrlAddress should be removed.
-            if ((isLoadUrl && !isRedirect) || isBackForward || isReload ||
-                    mLastLoadUrlAddress != null && mLastLoadUrlAddress.equals(url)) {
-                // Support the case where the user clicks on a link that takes them back to the
-                // same page.
-                mLastLoadUrlAddress = null;
-
-                // If the embedder requested the load of a certain URL via the loadUrl API, then we
-                // do not offer it to AwContentsClient.shouldOverrideUrlLoading.
-                // The embedder is also not allowed to intercept POST requests because of
-                // crbug.com/155250.
-            } else if (!navigationParams.isPost) {
-                ignoreNavigation = mContentsClient.shouldOverrideUrlLoading(url);
+            if (mDeferredShouldOverrideUrlLoadingIsPendingForPopup) {
+                mDeferredShouldOverrideUrlLoadingIsPendingForPopup = false;
+                // If this is used for all navigations in future, cases for application initiated
+                // load, redirect and backforward should also be filtered out.
+                if (!navigationParams.isPost) {
+                    ignoreNavigation = mContentsClient.shouldOverrideUrlLoading(url);
+                }
             }
-
-            // The existing contract is that shouldOverrideUrlLoading callbacks are delivered before
-            // onPageStarted callbacks; third party apps depend on this behavior.
-            // Using a ResouceThrottle to implement the navigation interception feature results in
-            // the WebContentsObserver.didStartLoading callback happening before the
-            // ResourceThrottle has a chance to run.
-            // To preserve the ordering the onPageStarted callback is synthesized from the
-            // shouldOverrideUrlLoading, and only if the navigation was not ignored (this
-            // balances out with the onPageFinished callback, which is suppressed in the
-            // AwContentsClient if the navigation was ignored).
+            // 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 (!ignoreNavigation) {
-                // 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.
                 mContentsClient.getCallbackHelper().postOnPageStarted(url);
             }
-
             return ignoreNavigation;
         }
     }
@@ -618,6 +595,7 @@ public class AwContents {
     // Recap: supplyContentsForPopup() is called on the parent window's content, this method is
     // called on the popup window's content.
     private void receivePopupContents(int popupNativeAwContents) {
+        mDeferredShouldOverrideUrlLoadingIsPendingForPopup = true;
         // Save existing view state.
         final boolean wasAttached = mIsAttachedToWindow;
         final boolean wasViewVisible = mIsViewVisible;
@@ -912,8 +890,6 @@ public class AwContents {
 
         mContentViewCore.loadUrl(params);
 
-        suppressInterceptionForThisNavigation();
-
         // The behavior of WebViewClassic uses the populateVisitedLinks callback in WebKit.
         // Chromium does not use this use code path and the best emulation of this behavior to call
         // request visited links once on the first URL load of the WebView.
@@ -923,14 +899,6 @@ public class AwContents {
         }
     }
 
-    private void suppressInterceptionForThisNavigation() {
-        if (mInterceptNavigationDelegate != null) {
-            // getUrl returns a sanitized address in the same format that will be used for
-            // callbacks, so it's safe to use string comparison as an equality check later on.
-            mInterceptNavigationDelegate.onUrlLoadRequested(mContentViewCore.getUrl());
-        }
-    }
-
     /**
      * Get the URL of the current page.
      *
@@ -1146,8 +1114,6 @@ public class AwContents {
      */
     public void goBack() {
         mContentViewCore.goBack();
-
-        suppressInterceptionForThisNavigation();
     }
 
     /**
@@ -1162,8 +1128,6 @@ public class AwContents {
      */
     public void goForward() {
         mContentViewCore.goForward();
-
-        suppressInterceptionForThisNavigation();
     }
 
     /**
@@ -1178,8 +1142,6 @@ public class AwContents {
      */
     public void goBackOrForward(int steps) {
         mContentViewCore.goToOffset(steps);
-
-        suppressInterceptionForThisNavigation();
     }
 
     /**