Make the throbber throb sooner after you navigate. This fixes the new tab page,

which would not start throbbing until the load committed. I think this was always
broken, but switching the tab contents types covered it up.

Now I have a flag that goes along with the tab updating that indicates if it's
a load update or a full update. This is necessary to avoid updating the title
to the page's URL until it does actually commit.

BUG=9310
Review URL: http://codereview.chromium.org/60066

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@13131 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 60 insertions, 34 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 3a69158..26ade9e 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1657,10 +1657,10 @@ void Browser::OpenURLFromTab(TabContents* source,
     if (GetStatusBubble())
       GetStatusBubble()->Hide();
 
-    // Synchronously update the location bar. This allows us to immediately
-    // have the URL bar update when the user types something, rather than
-    // going through the normal system of ScheduleUIUpdate which has a delay.
-    UpdateToolbar(false);
+    // Update the location bar and load state. These are both synchronous
+    // updates inside of ScheduleUIUpdate.
+    ScheduleUIUpdate(source, TabContents::INVALIDATE_URL |
+                             TabContents::INVALIDATE_LOAD);
   } else if (disposition == OFF_THE_RECORD) {
     OpenURLOffTheRecord(profile_, url);
     return;
@@ -1868,7 +1868,7 @@ void Browser::ConvertContentsToApplication(TabContents* contents) {
 void Browser::ContentsStateChanged(TabContents* source) {
   int index = tabstrip_model_.GetIndexOfTabContents(source);
   if (index != TabStripModel::kNoTab)
-    tabstrip_model_.UpdateTabContentsStateAt(index);
+    tabstrip_model_.UpdateTabContentsStateAt(index, true);
 }
 
 bool Browser::ShouldDisplayURLField() {
@@ -2199,17 +2199,27 @@ void Browser::UpdateToolbar(bool should_restore_state) {
 
 void Browser::ScheduleUIUpdate(const TabContents* source,
                                unsigned changed_flags) {
-  // Synchronously update the URL.
+  // Do some synchronous updates.
   if (changed_flags & TabContents::INVALIDATE_URL &&
       source == GetSelectedTabContents()) {
     // Only update the URL for the current tab. Note that we do not update
     // the navigation commands since those would have already been updated
     // synchronously by NavigationStateChanged.
     UpdateToolbar(false);
-
-    if (changed_flags == TabContents::INVALIDATE_URL)
-      return;  // Just had an update URL and nothing else.
   }
+  if (changed_flags & TabContents::INVALIDATE_LOAD && source) {
+    // Update the loading state synchronously. This is so the throbber will
+    // immediately start/stop, which gives a more snappy feel. We want to do
+    // this for any tab so they start & stop quickly, but the source can be
+    // NULL, so we have to check for that.
+    tabstrip_model_.UpdateTabContentsStateAt(
+        tabstrip_model_.GetIndexOfController(source->controller()), true);
+  }
+
+  // If the only updates were synchronously handled above, we're done.
+  if (changed_flags ==
+      (TabContents::INVALIDATE_URL | TabContents::INVALIDATE_LOAD))
+    return;
 
   // Save the dirty bits.
   scheduled_updates_.push_back(UIUpdate(source, changed_flags));
@@ -2289,7 +2299,7 @@ void Browser::ProcessPendingUIUpdates() {
 
     if (invalidate_tab) {  // INVALIDATE_TITLE or INVALIDATE_FAVICON.
       tabstrip_model_.UpdateTabContentsStateAt(
-          tabstrip_model_.GetIndexOfController(contents->controller()));
+          tabstrip_model_.GetIndexOfController(contents->controller()), false);
       window_->UpdateTitleBar();
 
       if (contents == GetSelectedTabContents()) {
diff --git a/chrome/browser/dom_ui/dom_ui_unittest.cc b/chrome/browser/dom_ui/dom_ui_unittest.cc
index 63eb2e7..3371acf 100644
--- a/chrome/browser/dom_ui/dom_ui_unittest.cc
+++ b/chrome/browser/dom_ui/dom_ui_unittest.cc
@@ -137,7 +137,7 @@ TEST_F(DOMUITest, StandardToDOMUI) {
   GURL new_tab_url(chrome::kChromeUINewTabURL);
   controller()->LoadURL(new_tab_url, GURL(), PageTransition::LINK);
   EXPECT_FALSE(contents()->ShouldDisplayURL());
-  EXPECT_FALSE(contents()->ShouldDisplayFavIcon());
+  EXPECT_TRUE(contents()->ShouldDisplayFavIcon());
   EXPECT_FALSE(contents()->IsBookmarkBarAlwaysVisible());
   EXPECT_TRUE(contents()->FocusLocationBarByDefault());
 
diff --git a/chrome/browser/tab_contents/web_contents.cc b/chrome/browser/tab_contents/web_contents.cc
index c4746a1..3ac939a 100644
--- a/chrome/browser/tab_contents/web_contents.cc
+++ b/chrome/browser/tab_contents/web_contents.cc
@@ -372,6 +372,10 @@ bool WebContents::ShouldDisplayURL() {
 }
 
 bool WebContents::ShouldDisplayFavIcon() {
+  // Always display a throbber during pending loads.
+  if (controller()->GetLastCommittedEntry() && controller()->GetPendingEntry())
+    return true;
+
   DOMUI* dom_ui = GetDOMUIForCurrentState();
   if (dom_ui)
     return !dom_ui->hide_favicon();
@@ -1501,7 +1505,7 @@ void WebContents::LoadStateChanged(const GURL& url,
   if (load_state_ == net::LOAD_STATE_READING_RESPONSE)
     SetNotWaitingForResponse();
   if (is_loading())
-    NotifyNavigationStateChanged(INVALIDATE_LOAD);
+    NotifyNavigationStateChanged(INVALIDATE_LOAD | INVALIDATE_FAVICON);
 }
 
 void WebContents::OnDidGetApplicationInfo(
diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index 821c80d..e5ee652 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -159,7 +159,7 @@ void TabStripModel::ReplaceTabContentsAt(int index,
   contents_data_[index]->contents = replacement_contents;
 
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-      TabChangedAt(replacement_contents, index));
+      TabChangedAt(replacement_contents, index, false));
 
   // Re-use the logic for selecting tabs to ensure the replacement contents is
   // shown and sized appropriately.
@@ -215,11 +215,11 @@ int TabStripModel::GetIndexOfController(
   return kNoTab;
 }
 
-void TabStripModel::UpdateTabContentsStateAt(int index) {
+void TabStripModel::UpdateTabContentsStateAt(int index, bool loading_only) {
   DCHECK(ContainsIndex(index));
 
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
-      TabChangedAt(GetContentsAt(index), index));
+      TabChangedAt(GetContentsAt(index), index, loading_only));
 }
 
 void TabStripModel::CloseAllTabs() {
diff --git a/chrome/browser/tabs/tab_strip_model.h b/chrome/browser/tabs/tab_strip_model.h
index 8220f4d..697e59c 100644
--- a/chrome/browser/tabs/tab_strip_model.h
+++ b/chrome/browser/tabs/tab_strip_model.h
@@ -70,7 +70,8 @@ class TabStripModelObserver {
   // The specified TabContents at |index| changed in some way. |contents| may
   // be an entirely different object and the old value is no longer available
   // by the time this message is delivered.
-  virtual void TabChangedAt(TabContents* contents, int index) { }
+  virtual void TabChangedAt(TabContents* contents, int index,
+                            bool loading_only) { }
   // The TabStripModel now no longer has any "significant" (user created or
   // user manipulated) tabs. The implementer may use this as a trigger to try
   // and close the window containing the TabStripModel, for example...
@@ -286,8 +287,9 @@ class TabStripModel : public NotificationObserver {
   int GetIndexOfController(const NavigationController* controller) const;
 
   // Notify any observers that the TabContents at the specified index has
-  // changed in some way.
-  void UpdateTabContentsStateAt(int index);
+  // changed in some way. Loading only specifies whether only the loading state
+  // has changed.
+  void UpdateTabContentsStateAt(int index, bool loading_only);
 
   // Make sure there is an auto-generated New Tab tab in the TabStripModel.
   // If |force_create| is true, the New Tab will be created even if the
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index c9006ce..d44fdbc 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -274,7 +274,8 @@ class MockTabStripModelObserver : public TabStripModelObserver {
   virtual void TabDetachedAt(TabContents* contents, int index) {
     states_.push_back(new State(contents, index, DETACH));
   }
-  virtual void TabChangedAt(TabContents* contents, int index) {
+  virtual void TabChangedAt(TabContents* contents, int index,
+                            bool loading_only) {
     states_.push_back(new State(contents, index, CHANGE));
   }
   virtual void TabStripEmpty() {
@@ -465,7 +466,7 @@ TEST_F(TabStripModelTest, TestBasicAPI) {
 
   // Test UpdateTabContentsStateAt
   {
-    tabstrip.UpdateTabContentsStateAt(0);
+    tabstrip.UpdateTabContentsStateAt(0, false);
     EXPECT_EQ(1, observer.GetStateCount());
     State s1(replacement_contents2, 0, MockTabStripModelObserver::CHANGE);
     EXPECT_TRUE(observer.StateEquals(0, s1));
diff --git a/chrome/browser/views/tabs/dragged_tab_view.cc b/chrome/browser/views/tabs/dragged_tab_view.cc
index fa45678..4f233d2 100644
--- a/chrome/browser/views/tabs/dragged_tab_view.cc
+++ b/chrome/browser/views/tabs/dragged_tab_view.cc
@@ -36,7 +36,7 @@ DraggedTabView::DraggedTabView(TabContents* datasource,
       close_animation_(this) {
   SetParentOwned(false);
 
-  renderer_->UpdateData(datasource);
+  renderer_->UpdateData(datasource, false);
 
   container_.reset(new views::WidgetWin);
   container_->set_delete_on_destroy(false);
diff --git a/chrome/browser/views/tabs/tab_renderer.cc b/chrome/browser/views/tabs/tab_renderer.cc
index 4ee10cf..e2d6381 100644
--- a/chrome/browser/views/tabs/tab_renderer.cc
+++ b/chrome/browser/views/tabs/tab_renderer.cc
@@ -236,15 +236,20 @@ TabRenderer::~TabRenderer() {
   delete crash_animation_;
 }
 
-void TabRenderer::UpdateData(TabContents* contents) {
+void TabRenderer::UpdateData(TabContents* contents, bool loading_only) {
   DCHECK(contents);
-  data_.favicon = contents->GetFavIcon();
-  data_.title = UTF16ToWideHack(contents->GetTitle());
+  if (!loading_only) {
+    data_.title = UTF16ToWideHack(contents->GetTitle());
+    data_.off_the_record = contents->profile()->IsOffTheRecord();
+    data_.show_download_icon = contents->IsDownloadShelfVisible();
+    data_.crashed = contents->is_crashed();
+    data_.favicon = contents->GetFavIcon();
+  }
+
+  // Loading state also involves whether we show the favicon, since that's where
+  // we display the throbber.
   data_.loading = contents->is_loading();
-  data_.off_the_record = contents->profile()->IsOffTheRecord();
   data_.show_icon = contents->ShouldDisplayFavIcon();
-  data_.show_download_icon = contents->IsDownloadShelfVisible();
-  data_.crashed = contents->is_crashed();
 }
 
 void TabRenderer::UpdateFromModel() {
diff --git a/chrome/browser/views/tabs/tab_renderer.h b/chrome/browser/views/tabs/tab_renderer.h
index 1a7a5fc..f652843 100644
--- a/chrome/browser/views/tabs/tab_renderer.h
+++ b/chrome/browser/views/tabs/tab_renderer.h
@@ -37,8 +37,10 @@ class TabRenderer : public views::View,
   virtual ~TabRenderer();
 
   // Updates the data the Tab uses to render itself from the specified
-  // TabContents.
-  void UpdateData(TabContents* contents);
+  // TabContents. If only the loading state was updated, the loading_only flag
+  // should be specified. If other things change, set this flag to false to 
+  // update everything.
+  void UpdateData(TabContents* contents, bool loading_only);
 
   // Updates the display to reflect the contents of this TabRenderer's model.
   void UpdateFromModel();
diff --git a/chrome/browser/views/tabs/tab_strip.cc b/chrome/browser/views/tabs/tab_strip.cc
index 01be61a..d377e0b 100644
--- a/chrome/browser/views/tabs/tab_strip.cc
+++ b/chrome/browser/views/tabs/tab_strip.cc
@@ -806,11 +806,11 @@ void TabStrip::TabInsertedAt(TabContents* contents,
     if (index == TabStripModel::kNoTab) {
       TabData d = { tab, gfx::Rect() };
       tab_data_.push_back(d);
-      tab->UpdateData(contents);
+      tab->UpdateData(contents, false);
     } else {
       TabData d = { tab, gfx::Rect() };
       tab_data_.insert(tab_data_.begin() + index, d);
-      tab->UpdateData(contents);
+      tab->UpdateData(contents, false);
     }
   }
 
@@ -867,11 +867,12 @@ void TabStrip::TabMoved(TabContents* contents, int from_index, int to_index) {
   StartMoveTabAnimation(from_index, to_index);
 }
 
-void TabStrip::TabChangedAt(TabContents* contents, int index) {
+void TabStrip::TabChangedAt(TabContents* contents, int index,
+                            bool loading_only) {
   // Index is in terms of the model. Need to make sure we adjust that index in
   // case we have an animation going.
   Tab* tab = GetTabAtAdjustForAnimation(index);
-  tab->UpdateData(contents);
+  tab->UpdateData(contents, loading_only);
   tab->UpdateFromModel();
 }
 
diff --git a/chrome/browser/views/tabs/tab_strip.h b/chrome/browser/views/tabs/tab_strip.h
index 38a212b..ea819a8 100644
--- a/chrome/browser/views/tabs/tab_strip.h
+++ b/chrome/browser/views/tabs/tab_strip.h
@@ -118,7 +118,8 @@ class TabStrip : public views::View,
                              int index,
                              bool user_gesture);
   virtual void TabMoved(TabContents* contents, int from_index, int to_index);
-  virtual void TabChangedAt(TabContents* contents, int index);
+  virtual void TabChangedAt(TabContents* contents, int index,
+                            bool loading_only);
 
   // Tab::Delegate implementation:
   virtual bool IsTabSelected(const Tab* tab) const;