PlzNavigate: Chrome UI changes for new methods of WebContentsObserver

chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java

Index: chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java b/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
index 73e32d0886a2d261229d15f61ef49449db4edbce..21a54425154edc3d699da76889a968abbfc1ee1b 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
@@ -308,15 +308,6 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
             }
 
             @Override
-            public void onDidNavigateMainFrame(Tab tab, String url, String baseUrl,
-                    boolean isNavigationToDifferentPage, boolean isFragmentNavigation,
-                    int statusCode) {
-                if (isNavigationToDifferentPage) {
-                    mToolbar.onNavigatedToDifferentPage();
-                }
-            }
-
-            @Override
             public void onTitleUpdated(Tab tab) {
                 mLocationBar.setTitleToPageTitle();
             }
@@ -393,21 +384,6 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
             }
 
             @Override
-            public void onDidStartNavigationToPendingEntry(Tab tab, String url) {
-                // Update URL as soon as it becomes available when it's a new tab.
-                // But we want to update only when it's a new tab. So we check whether the current
-                // navigation entry is initial, meaning whether it has the same target URL as the
-                // initial URL of the tab.
-                WebContents webContents = tab.getWebContents();
-                if (webContents == null) return;
-                NavigationController navigationController = webContents.getNavigationController();
-                if (navigationController == null) return;
-                if (navigationController.isInitialNavigation()) {
-                    mLocationBar.setUrlToPageUrl();
-                }
-            }
-
-            @Override
             public void onLoadUrl(Tab tab, LoadUrlParams params, int loadType) {
                 NewTabPage ntp = mToolbarModel.getNewTabPageForCurrentTab();
                 if (ntp == null) return;
@@ -432,21 +408,6 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
             }
 
             @Override
-            public void onDidFailLoad(Tab tab, boolean isProvisionalLoad, boolean isMainFrame,
-                    int errorCode, String description, String failingUrl) {
-                NewTabPage ntp = mToolbarModel.getNewTabPageForCurrentTab();
-                if (ntp == null) return;
-
-                // If the load failed due to a different navigation, there is no need to reset the
-                // location bar animations.
-                if (isProvisionalLoad && isMainFrame && !hasPendingNonNtpNavigation(tab)) {
-                    ntp.setUrlFocusAnimationsDisabled(false);
-                    mToolbar.onTabOrModelChanged();
-                    if (mToolbar.getProgressBar() != null) mToolbar.getProgressBar().finish(false);
-                }
-            }
-
-            @Override
             public void onContextualActionBarVisibilityChanged(Tab tab, boolean visible) {
                 if (visible) RecordUserAction.record("MobileActionBarShown");
                 ActionBar actionBar = mActionBarDelegate.getSupportActionBar();
@@ -461,16 +422,26 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
             }
 
             @Override
-            public void onDidStartProvisionalLoadForFrame(
-                    Tab tab, boolean isMainFrame, String validatedUrl) {
+            public void onDidStartNavigation(
+                    Tab tab, String url, boolean isInMainFrame, boolean isErrorPage) {
+                // Update URL as soon as it becomes available when it's a new tab.

[jam, 2017/02/07 05:10:32]

the old code was triggered by WCO::DidStartNavigationToPendingEntry which is
only called for main frame navigations. so this should be moved after the
!isInMainFrame check?

[shaktisahu, 2017/02/07 07:35:51]

Oh, I thought it is called for all frames. If you look into
NavigatorImpl::NavigateToEntry, it seems so since it has some checks for
IsMainFrame, but I might be incorrect.
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp...

[jam, 2017/02/07 17:12:38]

for current behavior (i.e. without site isolation/out of process iframes), it's
only called for main frame navigations.

[shaktisahu, 2017/02/08 01:00:46]

Done.

+                // But we want to update only when it's a new tab. So we check whether the current
+                // navigation entry is initial, meaning whether it has the same target URL as the
+                // initial URL of the tab.
+                if (tab.getWebContents() != null
+                        && tab.getWebContents().getNavigationController() != null
+                        && tab.getWebContents().getNavigationController().isInitialNavigation()) {
+                    mLocationBar.setUrlToPageUrl();
+                }
+
                 // This event is used as the primary trigger for the progress bar because it

[jam, 2017/02/07 05:10:32]

to maintain same behavior as before, we need to early out if isSamePage() is
true (since WCO::DidStartProvisionalLoadForFrame isn't called for same page
navigations)

[shaktisahu, 2017/02/07 07:35:51]

Okay, didn't know that, again not obvious from the code :). I will add
isSamePage() here and other call sites.

[jam, 2017/02/07 17:12:38]

yeah it's definitely not obvious or consistent from the other methods! that's
another reason why the new callbacks are better :)

[shaktisahu, 2017/02/08 01:00:46]

Done.

                 // is the earliest indication that a load has started for a particular frame. In
                 // the case of the progress bar, it should only traverse the screen a single time
                 // per page load. So if this event states the main frame has started loading the
                 // progress bar is started.
-                if (!isMainFrame) return;
+                if (!isInMainFrame) return;
 
-                if (NativePageFactory.isNativePageUrl(validatedUrl, tab.isIncognito())) {
+                if (NativePageFactory.isNativePageUrl(url, tab.isIncognito())) {
                     finishLoadProgress(false);
                     return;
                 }
@@ -479,6 +450,27 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
                 startLoadProgress();
                 updateLoadProgress(tab.getProgress());
             }
+
+            @Override
+            public void onDidFinishNavigation(Tab tab, String url, boolean isInMainFrame,
+                    boolean isErrorPage, boolean hasCommitted, boolean isSamePage,
+                    boolean isFragmentNavigation, Integer pageTransition, int errorCode,
+                    int httpStatusCode) {
+                if (hasCommitted && isInMainFrame && !isSamePage) {
+                    mToolbar.onNavigatedToDifferentPage();
+                }
+
+                // If the load failed due to a different navigation, there is no need to reset the
+                // location bar animations.
+                if (!hasCommitted && isInMainFrame && !hasPendingNonNtpNavigation(tab)) {
+                    NewTabPage ntp = mToolbarModel.getNewTabPageForCurrentTab();
+                    if (ntp == null) return;
+
+                    ntp.setUrlFocusAnimationsDisabled(false);
+                    mToolbar.onTabOrModelChanged();
+                    if (mToolbar.getProgressBar() != null) mToolbar.getProgressBar().finish(false);
+                }
+            }
         };
 
         mBookmarksObserver = new BookmarkBridge.BookmarkModelObserver() {