PlzNavigate: Chrome UI changes for new methods of WebContentsObserver

chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java

Index: chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java b/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java
index bf4d756e1fa0d9738c2b3f0016c1bb4985a87122..48128e1a0fe2e6c20c3f2e88aec8ec0f85d18bc9 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java
@@ -156,17 +156,20 @@ public class TabWebContentsObserver extends WebContentsObserver {
     }
 
     @Override
-    public void didFailLoad(boolean isProvisionalLoad, boolean isMainFrame, int errorCode,
-            String description, String failingUrl, boolean wasIgnoredByHandler) {
+    public void didFailLoad(
+            boolean isMainFrame, int errorCode, String description, String failingUrl) {
         mTab.updateThemeColorIfNeeded(true);
         RewindableIterator<TabObserver> observers = mTab.getTabObservers();
         while (observers.hasNext()) {
-            observers.next().onDidFailLoad(mTab, isProvisionalLoad, isMainFrame, errorCode,
-                    description, failingUrl);
+            observers.next().onDidFailLoad(mTab, isMainFrame, errorCode, description, failingUrl);
         }
 
         if (isMainFrame) mTab.didFailPageLoad(errorCode);
 
+        recordErrorInPolicyAuditor(failingUrl, description, errorCode);
+    }
+
+    private void recordErrorInPolicyAuditor(String failingUrl, String description, int errorCode) {
         PolicyAuditor auditor =
                 ((ChromeApplication) mTab.getApplicationContext()).getPolicyAuditor();
         auditor.notifyAuditEvent(mTab.getApplicationContext(), AuditEvent.OPEN_URL_FAILURE,
@@ -183,17 +186,31 @@ public class TabWebContentsObserver extends WebContentsObserver {
     }
 
     @Override
-    public void didStartProvisionalLoadForFrame(long frameId, long parentFrameId,
-            boolean isMainFrame, String validatedUrl, boolean isErrorPage) {
-        if (isMainFrame) mTab.didStartPageLoad(validatedUrl, isErrorPage);
+    public void didStartNavigation(String url, boolean isInMainFrame, boolean isErrorPage) {
+        if (isInMainFrame) {
+            mTab.didStartPageLoad(url, isErrorPage);
+        }
 
-        mTab.handleDidStartProvisionalLoadForFrame(isMainFrame, validatedUrl);
+        RewindableIterator<TabObserver> observers = mTab.getTabObservers();
+        while (observers.hasNext()) {
+            observers.next().onDidStartNavigation(mTab, url, isInMainFrame, isErrorPage);
+        }
     }
 
     @Override
-    public void didCommitProvisionalLoadForFrame(long frameId, boolean isMainFrame, String url,
-            int transitionType) {
-        if (isMainFrame && UmaUtils.isRunningApplicationStart()) {
+    public void didFinishNavigation(String url, boolean isInMainFrame, boolean isErrorPage,
+            boolean hasCommitted, boolean isSamePage, boolean isFragmentNavigation,
+            Integer pageTransition, int errorCode, int httpStatusCode) {
+        RewindableIterator<TabObserver> observers = mTab.getTabObservers();
+        if (errorCode != 0) {
+            mTab.updateThemeColorIfNeeded(true);
+            if (isInMainFrame) mTab.didFailPageLoad(errorCode);
+
+            // TODO(shaktisahu): Find out how to get the description for this error.
+            recordErrorInPolicyAuditor(url, "", errorCode);

[shaktisahu, 2017/01/31 01:57:53]

I didn't find a way to retrieve the error description. Mostly it is generated
from ContentRendererClient::GetNavigationErrorStrings that requires a
WebURLError to pass in which has more details like error domain etc.

[Ted C, 2017/01/31 18:57:48]

I wonder if we could just do net::ErrorToShortString here.  I believe bauerb@
added this though, so they'll likely know what should be logged here.

[Bernhard Bauer, 2017/02/07 13:19:16]

I think error_page::LocalizedError::GetErrorDetails() with a domain of
net::kErrorDomain should work. 

[shaktisahu, 2017/02/07 18:08:22]

Hmm... but error_page is in components and we may not be able to use it here.

[Ted C, 2017/02/08 05:31:15]

Why wouldn't we able to use it here?  We're in chrome/ so we have access to
all of the components?  Is it just because we haven't expose GetErrorDetails
to java?

If that is the case, we could do a few different things.
1.) we could add a new param to didFinishNavigation that is errorDetails that is
populated when errorCode != 0.   This is somewhat wasteful as only one call
site needs this, but it goes through the observer proxy so we're only paying
the jni cost once.  But no place in webview needs it, so it is a relatively
chrome
specific need in terms of embedder.
2.) Put a android utility class in components/error_page that allows this to
call
into it.
3.) Put a native method "somewhere" in the tab directory that allows you do
get this (easiest solution but probably least clean as you have an odd dangling
method somewhere...but I'd rather do this now with a TODO to find a better
home than to have a TODO/ship a build to stable where this isn't working
anymore...unless bauerb@ says it is ok to have broken for a while).

[shaktisahu, 2017/02/08 06:32:10]

Oh, I was trying to add this to web_contents_observer_proxy.cc which lives in
contents/, so components/error_page cannot be used there. This would rule out
option 1.

Since chrome is the only place where this is being used, I can go with option 3
for now with a TODO to file a better place later.

[Bernhard Bauer, 2017/02/08 10:26:38]

That sounds good to me, thanks!

[shaktisahu, 2017/02/10 08:07:41]

Seems like error_page::LocalizedError::GetErrorDetails is not  able to find the
string resource for many net error codes. Filed crbug/690784 to investigate
separately and left the error description empty for now.

+        }

[Ted C, 2017/01/31 18:57:47]

Should we return in the errorCode case?

[shaktisahu, 2017/02/03 06:56:16]

Done.

Yes. In that case I would move the block having
observers.next().onDidFinishNavigation() before this block as tab observers
still need to be notified.

+
+        if (isInMainFrame && hasCommitted && UmaUtils.isRunningApplicationStart()) {
             // Current median is 550ms, and long tail is very long. ZoomedIn gives good view of the
             // median and ZoomedOut gives a good overview.
             RecordHistogram.recordCustomTimesHistogram(
@@ -209,65 +226,36 @@ public class TabWebContentsObserver extends WebContentsObserver {
             UmaUtils.setRunningApplicationStart(false);
         }
 
-        if (isMainFrame) {
+        if (isInMainFrame && hasCommitted) {
             mTab.setIsTabStateDirty(true);
             mTab.updateTitle();
-        }
-
-        RewindableIterator<TabObserver> observers = mTab.getTabObservers();
-        while (observers.hasNext()) {
-            observers.next().onDidCommitProvisionalLoadForFrame(
-                    mTab, frameId, isMainFrame, url, transitionType);
+            mTab.handleDidFinishNavigation(url, pageTransition);
+            mTab.setIsShowingErrorPage(isErrorPage);
         }
 
         observers.rewind();
         while (observers.hasNext()) {
-            observers.next().onUrlUpdated(mTab);
+            observers.next().onDidFinishNavigation(mTab, url, isInMainFrame, isErrorPage,
+                    hasCommitted, isSamePage, isFragmentNavigation, pageTransition, errorCode,
+                    httpStatusCode);
         }
 
-        if (!isMainFrame) return;
-        mTab.handleDidCommitProvisonalLoadForFrame(url, transitionType);
-    }
+        if (hasCommitted) {
+            observers.rewind();
+            while (observers.hasNext()) {
+                observers.next().onUrlUpdated(mTab);
+            }
+        }
 
-    @Override
-    public void didNavigateMainFrame(String url, String baseUrl,
-            boolean isNavigationToDifferentPage, boolean isFragmentNavigation, int statusCode) {
         FullscreenManager fullscreenManager = mTab.getFullscreenManager();
-        if (isNavigationToDifferentPage && fullscreenManager != null) {
+        if (isInMainFrame && hasCommitted && !isSamePage && fullscreenManager != null) {
             fullscreenManager.setPersistentFullscreenMode(false);
         }
 
-        RewindableIterator<TabObserver> observers = mTab.getTabObservers();
-        while (observers.hasNext()) {
-            observers.next().onDidNavigateMainFrame(
-                    mTab, url, baseUrl, isNavigationToDifferentPage,
-                    isFragmentNavigation, statusCode);
-        }
-
         mTab.stopSwipeRefreshHandler();
     }
 
     @Override
-    public void didStartNavigation(String url, boolean isInMainFrame, boolean isErrorPage) {
-        RewindableIterator<TabObserver> observers = mTab.getTabObservers();
-        while (observers.hasNext()) {
-            observers.next().onDidStartNavigation(mTab, url, isInMainFrame, isErrorPage);
-        }
-    }
-
-    @Override
-    public void didFinishNavigation(String url, boolean isInMainFrame, boolean isErrorPage,
-            boolean hasCommitted, boolean isSamePage, Integer pageTransition, int errorCode) {
-        if (isInMainFrame && hasCommitted) mTab.setIsShowingErrorPage(isErrorPage);
-
-        RewindableIterator<TabObserver> observers = mTab.getTabObservers();
-        while (observers.hasNext()) {
-            observers.next().onDidFinishNavigation(mTab, url, isInMainFrame, isErrorPage,
-                    hasCommitted, isSamePage, pageTransition, errorCode);
-        }
-    }
-
-    @Override
     public void didFirstVisuallyNonEmptyPaint() {
         RewindableIterator<TabObserver> observers = mTab.getTabObservers();
         while (observers.hasNext()) {
@@ -320,14 +308,6 @@ public class TabWebContentsObserver extends WebContentsObserver {
     }
 
     @Override
-    public void didStartNavigationToPendingEntry(String url) {
-        RewindableIterator<TabObserver> observers = mTab.getTabObservers();
-        while (observers.hasNext()) {
-            observers.next().onDidStartNavigationToPendingEntry(mTab, url);
-        }
-    }
-
-    @Override
     public void destroy() {
         MediaCaptureNotificationService.updateMediaNotificationForTab(
                 mTab.getApplicationContext(), mTab.getId(), 0, mTab.getUrl());