Better distinguish didFinishLoad and didStopLoading

chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.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.toolbar;
 
 import android.content.Context;
 import android.graphics.drawable.Drawable;
 import android.os.Handler;
 import android.os.Looper;
@@ skipping 292 matching lines @@
             public void onDidNavigateMainFrame(Tab tab, String url, String baseUrl,
                     boolean isNavigationToDifferentPage, boolean isFragmentNavigation,
                     int statusCode) {
                 if (isNavigationToDifferentPage) {
                     mToolbar.onNavigatedToDifferentPage();
                 }
             }
 
             @Override
             public void onPageLoadStarted(Tab tab, String url) {
                 updateButtonStatus();

[Ted C, 2015/10/27 23:07:20]

Based on your comments on the change, I think all of this should
go into onLoadStarted.

Also, reading the description in TabObserver:

"""
Called when the WebContents starts loading. Different from
{@link #onPageLoadStarted(Tab, String)}, if the user is navigated to a different
url while
staying in the same html document, {@link #onLoadStarted(Tab)} will be called,
while
{@link #onPageLoadStarted(Tab, String)} will not.
@param tab The notifying {@link Tab}.
"""

So "if" that comment is still valid, then onLoadStarted will be
called for in page navigations while onPageLoadStarted will not.
So that will likely result in undesired flashing of the progress bar.

The progress bar logic is different from the throbber on desktop as
we want it shown at most one time per page load.

For clarity (and my own sanity), the current tab observer methods that
are being listened to for showing the progress bar:

TabObserver#onPageLoadStarted == DidStartProvisionalLoadForFrame
    (where isMainFrame == true)

TabObserver#onPageLoadFinished == DidFinishLoad
    (where isMainFrame == true)

TabObserver#onPageLoadFailed == DidFailProvisionalLoad || DidFailLoad
    (where isMainFrame == true)

So, I'm curious how onLoadStarted and onLoadStopped correspond, as they
both seem to be driven off of WebContentsDelegate::LoadingStateChanged.
But it looks like we do not propagate the to_different_document bool up
to java which might get the behaviors closer together.

[Nate Chapin, 2015/10/28 20:57:39]

Done, toDifferentDocument propagated.

-                updateTabLoadingState(true);
                 mLoadProgressSimulator.cancel();
 
                 if (NativePageFactory.isNativePageUrl(url, tab.isIncognito())) {
                     finishLoadProgress(false);
-                } else {
-                    mToolbar.startLoadProgress();
-                    setLoadProgress(0.0f);
                 }
             }
 
             @Override
-            public void onPageLoadFinished(Tab tab) {
-                Tab currentTab = mToolbarModel.getTab();
-                updateTabLoadingState(true);
-
-                // If we made some progress, fast-forward to complete, otherwise just dismiss any
-                // MINIMUM_LOAD_PROGRESS that had been set.
-                if (currentTab.getProgress() > MINIMUM_LOAD_PROGRESS) setLoadProgress(1.0f);
-                finishLoadProgress(true);
-            }
-
-            @Override
-            public void onPageLoadFailed(Tab tab, int errorCode) {
-                updateTabLoadingState(true);
-                finishLoadProgress(false);
-            }
-
-            @Override
             public void onTitleUpdated(Tab tab) {
                 mLocationBar.setTitleToPageTitle();
             }
 
             @Override
             public void onUrlUpdated(Tab tab) {
                 // Update the SSL security state as a result of this notification as it will
                 // sometimes be the only update we receive.
                 updateTabLoadingState(true);
 
                 // A URL update is a decent enough indicator that the toolbar widget is in
                 // a stable state to capture its bitmap for use in fullscreen.
                 mControlContainer.setReadyForBitmapCapture(true);
             }
 
             @Override
             public void onCrash(Tab tab, boolean sadTabShown) {
                 updateTabLoadingState(false);
                 updateButtonStatus();
                 finishLoadProgress(false);
             }
 
             @Override
+            public void onLoadStarted(Tab tab) {
+                updateTabLoadingState(true);
+                mToolbar.startLoadProgress();
+                setLoadProgress(0.0f);
+            }
+
+            @Override
+            public void onLoadStopped(Tab tab) {
+                updateTabLoadingState(true);
+
+                // If we made some progress, fast-forward to complete, otherwise just dismiss any
+                // MINIMUM_LOAD_PROGRESS that had been set.
+                if (tab.getProgress() > MINIMUM_LOAD_PROGRESS && tab.getProgress() < 1.0f) {
+                    setLoadProgress(1.0f);
+                }
+                finishLoadProgress(true);
+            }
+
+            @Override
             public void onLoadProgressChanged(Tab tab, int progress) {
                 // TODO(kkimlabs): Investigate using float progress all the way up to Blink.
                 setLoadProgress(progress / 100.0f);
             }
 
             @Override
             public void onContentChanged(Tab tab) {
                 mToolbar.onTabContentViewChanged();
             }
 
@@ skipping 707 matching lines @@
         }
 
         /**
          * Cancels simulating load progress.
          */
         public void cancel() {
             mHandler.removeMessages(MSG_ID_UPDATE_PROGRESS);
         }
     }
 }