Better distinguish didFinishLoad and didStopLoading

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 34b00f9f4cdc7b4be6bfdd251ab2c78e1bab70de..c1dede6224720dd2a3cd933519b47496954fbb00 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
@@ -311,35 +311,14 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
             @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();
             }
@@ -363,6 +342,25 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
             }
 
             @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);