Makes it so that you shouldn't be able to get a big fat bookmark

bar. This happened because when we changed from needing a
bookmarkbar/extension-shelf to not needing one we processed the layout
change asynchronously, but could paint immediately and painting always
checks the current state. I initially made painting/layout stay in
sync with regards to whether they thought the bookmark bar should be
shown, which also fixes this, but because we process the change async
there was still some noticable jank. Instead I've changed processing
of the transition from needing bars to not (or vice-versa) to be
synchronous.

BUG=22165
TEST=see bug

Review URL: http://codereview.chromium.org/219034

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

6 files changed, 63 insertions, 39 deletions

diff --git a/chrome/browser/tab_contents/navigation_controller.cc b/chrome/browser/tab_contents/navigation_controller.cc
index 82210ed..fddf3de 100644
--- a/chrome/browser/tab_contents/navigation_controller.cc
+++ b/chrome/browser/tab_contents/navigation_controller.cc
@@ -31,6 +31,10 @@
 
 namespace {
 
+const int kInvalidateAllButShelves =
+    0xFFFFFFFF & ~(TabContents::INVALIDATE_BOOKMARK_BAR |
+                   TabContents::INVALIDATE_EXTENSION_SHELF);
+
 // Invoked when entries have been pruned, or removed. For example, if the
 // current entries are [google, digg, yahoo], with the current entry google,
 // and the user types in cnet, then digg and yahoo are pruned.
@@ -376,8 +380,7 @@ void NavigationController::AddTransientEntry(NavigationEntry* entry) {
   DiscardTransientEntry();
   entries_.insert(entries_.begin() + index, linked_ptr<NavigationEntry>(entry));
   transient_entry_index_  = index;
-  tab_contents_->NotifyNavigationStateChanged(
-      TabContents::INVALIDATE_EVERYTHING);
+  tab_contents_->NotifyNavigationStateChanged(kInvalidateAllButShelves);
 }
 
 void NavigationController::LoadURL(const GURL& url, const GURL& referrer,
@@ -400,6 +403,7 @@ void NavigationController::OnUserGesture() {
 
 bool NavigationController::RendererDidNavigate(
     const ViewHostMsg_FrameNavigate_Params& params,
+    int extra_invalidate_flags,
     LoadCommittedDetails* details) {
   // Save the previous state before we clobber it.
   if (GetLastCommittedEntry()) {
@@ -477,7 +481,7 @@ bool NavigationController::RendererDidNavigate(
   details->serialized_security_info = params.security_info;
   details->is_content_filtered = params.is_content_filtered;
   details->http_status_code = params.http_status_code;
-  NotifyNavigationEntryCommitted(details);
+  NotifyNavigationEntryCommitted(details, extra_invalidate_flags);
 
   user_gesture_observed_ = false;
 
@@ -793,7 +797,7 @@ void NavigationController::CommitPendingEntry() {
   details.is_in_page = AreURLsInPageNavigation(details.previous_url,
                                                details.entry->url());
   details.is_main_frame = true;
-  NotifyNavigationEntryCommitted(&details);
+  NotifyNavigationEntryCommitted(&details, 0);
 }
 
 int NavigationController::GetIndexOfEntry(
@@ -835,8 +839,7 @@ void NavigationController::DiscardNonCommittedEntries() {
   // If there was a transient entry, invalidate everything so the new active
   // entry state is shown.
   if (transient) {
-    tab_contents_->NotifyNavigationStateChanged(
-        TabContents::INVALIDATE_EVERYTHING);
+    tab_contents_->NotifyNavigationStateChanged(kInvalidateAllButShelves);
   }
 }
 
@@ -901,7 +904,8 @@ void NavigationController::NavigateToPendingEntry(bool reload) {
 }
 
 void NavigationController::NotifyNavigationEntryCommitted(
-    LoadCommittedDetails* details) {
+    LoadCommittedDetails* details,
+    int extra_invalidate_flags) {
   details->entry = GetActiveEntry();
   NotificationDetails notification_details =
       Details<LoadCommittedDetails>(details);
@@ -915,7 +919,7 @@ void NavigationController::NotifyNavigationEntryCommitted(
   // should be removed, and interested parties should just listen for the
   // notification below instead.
   tab_contents_->NotifyNavigationStateChanged(
-      TabContents::INVALIDATE_EVERYTHING);
+      kInvalidateAllButShelves | extra_invalidate_flags);
 
   NotificationService::current()->Notify(
       NotificationType::NAV_ENTRY_COMMITTED,
diff --git a/chrome/browser/tab_contents/navigation_controller.h b/chrome/browser/tab_contents/navigation_controller.h
index 3d0d6cd..7374d38 100644
--- a/chrome/browser/tab_contents/navigation_controller.h
+++ b/chrome/browser/tab_contents/navigation_controller.h
@@ -314,7 +314,11 @@ class NavigationController {
   //
   // In the case that nothing has changed, the details structure is undefined
   // and it will return false.
+  //
+  // |extra_invalidate_flags| are an additional set of flags (InvalidateTypes)
+  // added to the flags sent to the delegate's NotifyNavigationStateChanged.
   bool RendererDidNavigate(const ViewHostMsg_FrameNavigate_Params& params,
+                           int extra_invalidate_flags,
                            LoadCommittedDetails* details);
 
   // Notifies us that we just became active. This is used by the TabContents
@@ -417,7 +421,11 @@ class NavigationController {
 
   // Allows the derived class to issue notifications that a load has been
   // committed. This will fill in the active entry to the details structure.
-  void NotifyNavigationEntryCommitted(LoadCommittedDetails* details);
+  //
+  // |extra_invalidate_flags| are an additional set of flags (InvalidateTypes)
+  // added to the flags sent to the delegate's NotifyNavigationStateChanged.
+  void NotifyNavigationEntryCommitted(LoadCommittedDetails* details,
+                                      int extra_invalidate_flags);
 
   // Sets the max restored page ID this NavigationController has seen, if it
   // was restored from a previous session.
diff --git a/chrome/browser/tab_contents/navigation_controller_unittest.cc b/chrome/browser/tab_contents/navigation_controller_unittest.cc
index deae8c0..285190e 100644
--- a/chrome/browser/tab_contents/navigation_controller_unittest.cc
+++ b/chrome/browser/tab_contents/navigation_controller_unittest.cc
@@ -751,7 +751,7 @@ TEST_F(NavigationControllerTest, Redirect) {
   NavigationController::LoadCommittedDetails details;
 
   EXPECT_EQ(0U, notifications.size());
-  EXPECT_TRUE(controller().RendererDidNavigate(params, &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
 
@@ -795,7 +795,7 @@ TEST_F(NavigationControllerTest, ImmediateRedirect) {
   NavigationController::LoadCommittedDetails details;
 
   EXPECT_EQ(0U, notifications.size());
-  EXPECT_TRUE(controller().RendererDidNavigate(params, &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
 
@@ -832,7 +832,7 @@ TEST_F(NavigationControllerTest, NewSubframe) {
   params.is_post = false;
 
   NavigationController::LoadCommittedDetails details;
-  EXPECT_TRUE(controller().RendererDidNavigate(params, &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
   EXPECT_EQ(url1, details.previous_url);
@@ -867,7 +867,7 @@ TEST_F(NavigationControllerTest, SubframeOnEmptyPage) {
   params.is_post = false;
 
   NavigationController::LoadCommittedDetails details;
-  EXPECT_FALSE(controller().RendererDidNavigate(params, &details));
+  EXPECT_FALSE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_EQ(0U, notifications.size());
 }
 
@@ -893,7 +893,7 @@ TEST_F(NavigationControllerTest, AutoSubframe) {
 
   // Navigating should do nothing.
   NavigationController::LoadCommittedDetails details;
-  EXPECT_FALSE(controller().RendererDidNavigate(params, &details));
+  EXPECT_FALSE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_EQ(0U, notifications.size());
 
   // There should still be only one entry.
@@ -923,7 +923,7 @@ TEST_F(NavigationControllerTest, BackSubframe) {
 
   // This should generate a new entry.
   NavigationController::LoadCommittedDetails details;
-  EXPECT_TRUE(controller().RendererDidNavigate(params, &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
   EXPECT_EQ(2, controller().entry_count());
@@ -932,7 +932,7 @@ TEST_F(NavigationControllerTest, BackSubframe) {
   const GURL url3("http://foo3");
   params.page_id = 2;
   params.url = url3;
-  EXPECT_TRUE(controller().RendererDidNavigate(params, &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
   EXPECT_EQ(3, controller().entry_count());
@@ -942,7 +942,7 @@ TEST_F(NavigationControllerTest, BackSubframe) {
   controller().GoBack();
   params.url = url2;
   params.page_id = 1;
-  EXPECT_TRUE(controller().RendererDidNavigate(params, &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
   EXPECT_EQ(3, controller().entry_count());
@@ -952,7 +952,7 @@ TEST_F(NavigationControllerTest, BackSubframe) {
   controller().GoBack();
   params.url = url1;
   params.page_id = 0;
-  EXPECT_TRUE(controller().RendererDidNavigate(params, &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
   EXPECT_EQ(3, controller().entry_count());
@@ -1007,7 +1007,7 @@ TEST_F(NavigationControllerTest, InPage) {
 
   // This should generate a new entry.
   NavigationController::LoadCommittedDetails details;
-  EXPECT_TRUE(controller().RendererDidNavigate(params, &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
   EXPECT_EQ(2, controller().entry_count());
@@ -1017,8 +1017,7 @@ TEST_F(NavigationControllerTest, InPage) {
   controller().GoBack();
   back_params.url = url1;
   back_params.page_id = 0;
-  EXPECT_TRUE(controller().RendererDidNavigate(back_params,
-                                                          &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(back_params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
   EXPECT_EQ(2, controller().entry_count());
@@ -1030,8 +1029,7 @@ TEST_F(NavigationControllerTest, InPage) {
   controller().GoForward();
   forward_params.url = url2;
   forward_params.page_id = 1;
-  EXPECT_TRUE(controller().RendererDidNavigate(forward_params,
-                                                          &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(forward_params, 0, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       NotificationType::NAV_ENTRY_COMMITTED));
   EXPECT_EQ(2, controller().entry_count());
@@ -1044,11 +1042,9 @@ TEST_F(NavigationControllerTest, InPage) {
   // one identified by an existing page ID. This would result in the second URL
   // losing the reference fragment when you navigate away from it and then back.
   controller().GoBack();
-  EXPECT_TRUE(controller().RendererDidNavigate(back_params,
-                                                          &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(back_params, 0, &details));
   controller().GoForward();
-  EXPECT_TRUE(controller().RendererDidNavigate(forward_params,
-                                                          &details));
+  EXPECT_TRUE(controller().RendererDidNavigate(forward_params, 0, &details));
   EXPECT_EQ(forward_params.url,
             controller().GetActiveEntry()->url());
 }
@@ -1168,7 +1164,7 @@ TEST_F(NavigationControllerTest, RestoreNavigate) {
   params.gesture = NavigationGestureUser;
   params.is_post = false;
   NavigationController::LoadCommittedDetails details;
-  our_controller.RendererDidNavigate(params, &details);
+  our_controller.RendererDidNavigate(params, 0, &details);
 
   // There should be no longer any pending entry and one committed one. This
   // means that we were able to locate the entry, assign its site instance, and
@@ -1432,7 +1428,7 @@ TEST_F(NavigationControllerTest, SameSubframe) {
   params.gesture = NavigationGestureAuto;
   params.is_post = false;
   NavigationController::LoadCommittedDetails details;
-  EXPECT_FALSE(controller().RendererDidNavigate(params, &details));
+  EXPECT_FALSE(controller().RendererDidNavigate(params, 0, &details));
 
   // Nothing should have changed.
   EXPECT_EQ(controller().entry_count(), 1);
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index a28ccf1..ba305b9 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -1958,9 +1958,20 @@ void TabContents::RenderViewDeleted(RenderViewHost* rvh) {
 
 void TabContents::DidNavigate(RenderViewHost* rvh,
                               const ViewHostMsg_FrameNavigate_Params& params) {
-  if (PageTransition::IsMainFrame(params.transition))
+  int extra_invalidate_flags = 0;
+
+  if (PageTransition::IsMainFrame(params.transition)) {
+    bool was_bookmark_bar_visible = IsBookmarkBarAlwaysVisible();
+    bool was_extension_shelf_visible = IsExtensionShelfAlwaysVisible();
+
     render_manager_.DidNavigateMainFrame(rvh);
 
+    if (was_bookmark_bar_visible != IsBookmarkBarAlwaysVisible())
+      extra_invalidate_flags |= INVALIDATE_BOOKMARK_BAR;
+    if (was_extension_shelf_visible != IsExtensionShelfAlwaysVisible())
+      extra_invalidate_flags |= INVALIDATE_EXTENSION_SHELF;
+  }
+
   // Update the site of the SiteInstance if it doesn't have one yet.
   if (!GetSiteInstance()->has_site())
     GetSiteInstance()->SetSite(params.url);
@@ -1977,7 +1988,8 @@ void TabContents::DidNavigate(RenderViewHost* rvh,
     contents_mime_type_ = params.contents_mime_type;
 
   NavigationController::LoadCommittedDetails details;
-  bool did_navigate = controller_.RendererDidNavigate(params, &details);
+  bool did_navigate = controller_.RendererDidNavigate(
+      params, extra_invalidate_flags, &details);
 
   // Update history. Note that this needs to happen after the entry is complete,
   // which WillNavigate[Main,Sub]Frame will do before this function is called.
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index 4faba02..70989ad 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -105,12 +105,16 @@ class TabContents : public PageNavigator,
   // Flags passed to the TabContentsDelegate.NavigationStateChanged to tell it
   // what has changed. Combine them to update more than one thing.
   enum InvalidateTypes {
-    INVALIDATE_URL = 1,           // The URL has changed.
-    INVALIDATE_TAB = 2,           // The tab (favicon, title, etc.) has changed
-    INVALIDATE_LOAD = 4,          // The loading state has changed.
-    INVALIDATE_PAGE_ACTIONS = 8,  // Page action icons have changed.
-    // Helper for forcing a refresh.
-    INVALIDATE_EVERYTHING = 0xFFFFFFFF
+    INVALIDATE_URL             = 1 << 0,  // The URL has changed.
+    INVALIDATE_TAB             = 1 << 1,  // The tab (favicon, title, etc.) has
+                                          // changed.
+    INVALIDATE_LOAD            = 1 << 2,  // The loading state has changed.
+    INVALIDATE_PAGE_ACTIONS    = 1 << 3,  // Page action icons have changed.
+    INVALIDATE_BOOKMARK_BAR    = 1 << 4,  // State of IsBookmarkBarAlwaysVisible
+                                          // changed.
+    INVALIDATE_EXTENSION_SHELF = 1 << 5,  // State of
+                                          // IsExtensionShelfAlwaysVisible
+                                          // changed.
   };
 
   // |base_tab_contents| is used if we want to size the new tab contents view
diff --git a/chrome/browser/tab_contents/web_contents_unittest.cc b/chrome/browser/tab_contents/web_contents_unittest.cc
index f7d021b..5231116 100644
--- a/chrome/browser/tab_contents/web_contents_unittest.cc
+++ b/chrome/browser/tab_contents/web_contents_unittest.cc
@@ -210,7 +210,7 @@ TEST_F(TabContentsTest, UpdateTitle) {
   InitNavigateParams(&params, 0, GURL(chrome::kAboutBlankURL));
 
   NavigationController::LoadCommittedDetails details;
-  controller().RendererDidNavigate(params, &details);
+  controller().RendererDidNavigate(params, 0, &details);
 
   contents()->UpdateTitle(rvh(), 0, L"    Lots O' Whitespace\n");
   EXPECT_EQ(ASCIIToUTF16("Lots O' Whitespace"), contents()->GetTitle());
@@ -231,7 +231,7 @@ TEST_F(TabContentsTest, NTPViewSource) {
   ViewHostMsg_FrameNavigate_Params params;
   InitNavigateParams(&params, 0, kGURL);
   NavigationController::LoadCommittedDetails details;
-  controller().RendererDidNavigate(params, &details);
+  controller().RendererDidNavigate(params, 0, &details);
   // Also check title and url.
   EXPECT_EQ(ASCIIToUTF16(kUrl), contents()->GetTitle());
   EXPECT_EQ(true, contents()->ShouldDisplayURL());