PlzNavigate: Chrome UI changes for new methods of WebContentsObserver

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.tab;
 
 import android.os.SystemClock;
 import android.support.annotation.IntDef;
 import android.view.View;
 
@@ skipping 138 matching lines @@
     @Override
     public void didFinishLoad(long frameId, String validatedUrl, boolean isMainFrame) {
         if (isMainFrame) mTab.didFinishPageLoad();
         PolicyAuditor auditor =
                 ((ChromeApplication) mTab.getApplicationContext()).getPolicyAuditor();
         auditor.notifyAuditEvent(
                 mTab.getApplicationContext(), AuditEvent.OPEN_URL_SUCCESS, validatedUrl, "");
     }
 
     @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,
                 failingUrl, description);
         if (errorCode == BLOCKED_BY_ADMINISTRATOR) {
             auditor.notifyAuditEvent(
                     mTab.getApplicationContext(), AuditEvent.OPEN_URL_BLOCKED, failingUrl, "");
         }
     }
 
     @Override
     public void titleWasSet(String title) {
         mTab.updateTitle(title);
     }
 
     @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(
                     "Startup.FirstCommitNavigationTime2.ZoomedIn",
                     SystemClock.uptimeMillis() - UmaUtils.getForegroundStartTime(),
                     200, 1000, TimeUnit.MILLISECONDS, 100);
             // For ZoomedOut very rarely is it under 50ms and this range matches
             // CustomTabs.IntentToFirstCommitNavigationTime2.ZoomedOut.
             RecordHistogram.recordCustomTimesHistogram(
                     "Startup.FirstCommitNavigationTime2.ZoomedOut",
                     SystemClock.uptimeMillis() - UmaUtils.getForegroundStartTime(),
                     50, TimeUnit.MINUTES.toMillis(10), TimeUnit.MILLISECONDS, 50);
             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);
-    }
-
-    @Override
-    public void didNavigateMainFrame(String url, String baseUrl,
-            boolean isNavigationToDifferentPage, boolean isFragmentNavigation, int statusCode) {
-        FullscreenManager fullscreenManager = mTab.getFullscreenManager();
-        if (isNavigationToDifferentPage && fullscreenManager != null) {
-            fullscreenManager.setPersistentFullscreenMode(false);
+        if (hasCommitted) {
+            observers.rewind();
+            while (observers.hasNext()) {
+                observers.next().onUrlUpdated(mTab);
+            }
         }
 
-        RewindableIterator<TabObserver> observers = mTab.getTabObservers();
-        while (observers.hasNext()) {
-            observers.next().onDidNavigateMainFrame(
-                    mTab, url, baseUrl, isNavigationToDifferentPage,
-                    isFragmentNavigation, statusCode);
+        FullscreenManager fullscreenManager = mTab.getFullscreenManager();
+        if (isInMainFrame && hasCommitted && !isSamePage && fullscreenManager != null) {
+            fullscreenManager.setPersistentFullscreenMode(false);
         }
 
         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()) {
             observers.next().didFirstVisuallyNonEmptyPaint(mTab);
         }
     }
 
     @Override
     public void didChangeThemeColor(int color) {
         mTab.updateThemeColorIfNeeded(true);
@@ skipping 32 matching lines @@
         mTab.notifyLoadProgress(mTab.getProgress());
 
         mTab.updateFullscreenEnabledState();
 
         if (!mTab.maybeShowNativePage(mTab.getUrl(), false)) {
             mTab.showRenderedPage();
         }
     }
 
     @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());
         super.destroy();
     }
 }