Make the bookmarks bar disappear when the load after the new tab page commits

rather than when it is pending. This makes it change at the same time the
page changes.

To support this, we now have to keep track of both a pending and a committed
DOMUI object. This is tracked by the RenderManager, which does a similar
swapping between pending and committed RenderViewHosts.

BUG=8963
Review URL: http://codereview.chromium.org/42512

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

8 files changed, 291 insertions, 80 deletions

diff --git a/chrome/browser/dom_ui/dom_ui_unittest.cc b/chrome/browser/dom_ui/dom_ui_unittest.cc
new file mode 100644
index 0000000..40a5617
--- /dev/null
+++ b/chrome/browser/dom_ui/dom_ui_unittest.cc
@@ -0,0 +1,116 @@
+// Copyright (c) 2009 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.

+

+#include "chrome/browser/dom_ui/new_tab_ui.h"

+#include "chrome/browser/renderer_host/test_render_view_host.h"

+#include "chrome/common/url_constants.h"

+#include "testing/gtest/include/gtest/gtest.h"

+

+class DOMUITest : public RenderViewHostTestHarness {

+ public:

+  DOMUITest() {}

+

+ private:

+  DISALLOW_COPY_AND_ASSIGN(DOMUITest);

+};

+

+// Tests that the New Tab Page flags are correctly set and propogated by

+// WebContents when we first navigate to a DOM UI page, then to a standard

+// non-DOM-UI page.

+TEST_F(DOMUITest, DOMUIToStandard) {

+  // Start a pending load.

+  GURL new_tab_url(chrome::kChromeUINewTabURL);

+  controller()->LoadURL(new_tab_url, GURL(), PageTransition::LINK);

+

+  // The navigation entry should be pending with no committed entry.

+  ASSERT_TRUE(controller()->GetPendingEntry());

+  ASSERT_FALSE(controller()->GetLastCommittedEntry());

+

+  // Check the things the pending DOM UI should have set.

+  EXPECT_FALSE(contents()->ShouldDisplayURL());

+  EXPECT_FALSE(contents()->ShouldDisplayFavIcon());

+  EXPECT_TRUE(contents()->IsBookmarkBarAlwaysVisible());

+  EXPECT_TRUE(contents()->FocusLocationBarByDefault());

+

+  // Now commit the load.

+  rvh()->SendNavigate(1, new_tab_url);

+

+  // The same flags should be set as before now that the load has committed.

+  // Note that the location bar isn't focused now. Once the load commits, we

+  // don't care about this flag, so this value is OK.

+  EXPECT_FALSE(contents()->ShouldDisplayURL());

+  EXPECT_FALSE(contents()->ShouldDisplayFavIcon());

+  EXPECT_TRUE(contents()->IsBookmarkBarAlwaysVisible());

+  EXPECT_FALSE(contents()->FocusLocationBarByDefault());

+

+  // Start a pending navigation to a regular page.

+  GURL next_url("http://google.com/");

+  controller()->LoadURL(next_url, GURL(), PageTransition::LINK);

+

+  // Check the flags. Some should reflect the new page (URL, title), some should

+  // reflect the old one (bookmark bar) until it has committed.

+  EXPECT_TRUE(contents()->ShouldDisplayURL());

+  EXPECT_TRUE(contents()->ShouldDisplayFavIcon());

+  EXPECT_TRUE(contents()->IsBookmarkBarAlwaysVisible());

+  EXPECT_FALSE(contents()->FocusLocationBarByDefault());

+

+  // Commit the regular page load. Note that we must send it to the "pending"

+  // RenderViewHost, since this transition will also cause a process transition,

+  // and our RVH pointer will be the "committed" one.

+  static_cast<TestRenderViewHost*>(

+      contents()->render_manager()->pending_render_view_host())->SendNavigate(

+          2, next_url);

+

+  // The state should now reflect a regular page.

+  EXPECT_TRUE(contents()->ShouldDisplayURL());

+  EXPECT_TRUE(contents()->ShouldDisplayFavIcon());

+  EXPECT_FALSE(contents()->IsBookmarkBarAlwaysVisible());

+  EXPECT_FALSE(contents()->FocusLocationBarByDefault());

+}

+

+TEST_F(DOMUITest, DOMUIToDOMUI) {

+  // Do a load (this state is tested above).

+  GURL new_tab_url(chrome::kChromeUINewTabURL);

+  controller()->LoadURL(new_tab_url, GURL(), PageTransition::LINK);

+  rvh()->SendNavigate(1, new_tab_url);

+

+  // Start another pending load of the new tab page.

+  controller()->LoadURL(new_tab_url, GURL(), PageTransition::LINK);

+  rvh()->SendNavigate(2, new_tab_url);

+

+  // The flags should be the same as the non-pending state.

+  EXPECT_FALSE(contents()->ShouldDisplayURL());

+  EXPECT_FALSE(contents()->ShouldDisplayFavIcon());

+  EXPECT_TRUE(contents()->IsBookmarkBarAlwaysVisible());

+  EXPECT_FALSE(contents()->FocusLocationBarByDefault());

+}

+

+TEST_F(DOMUITest, StandardToDOMUI) {

+  // Start a pending navigation to a regular page.

+  GURL std_url("http://google.com/");

+  controller()->LoadURL(std_url, GURL(), PageTransition::LINK);

+

+  // The state should now reflect the default.

+  EXPECT_TRUE(contents()->ShouldDisplayURL());

+  EXPECT_TRUE(contents()->ShouldDisplayFavIcon());

+  EXPECT_FALSE(contents()->IsBookmarkBarAlwaysVisible());

+  EXPECT_FALSE(contents()->FocusLocationBarByDefault());

+

+  // Commit the load, the state should be the same.

+  rvh()->SendNavigate(1, std_url);

+  EXPECT_TRUE(contents()->ShouldDisplayURL());

+  EXPECT_TRUE(contents()->ShouldDisplayFavIcon());

+  EXPECT_FALSE(contents()->IsBookmarkBarAlwaysVisible());

+  EXPECT_FALSE(contents()->FocusLocationBarByDefault());

+

+  // Start a pending load for a DOMUI.

+  GURL new_tab_url(chrome::kChromeUINewTabURL);

+  controller()->LoadURL(new_tab_url, GURL(), PageTransition::LINK);

+  EXPECT_FALSE(contents()->ShouldDisplayURL());

+  EXPECT_FALSE(contents()->ShouldDisplayFavIcon());

+  EXPECT_FALSE(contents()->IsBookmarkBarAlwaysVisible());

+  EXPECT_TRUE(contents()->FocusLocationBarByDefault());

+

+  // Committing DOM UI is tested above.

+}

diff --git a/chrome/browser/dom_ui/new_tab_ui.cc b/chrome/browser/dom_ui/new_tab_ui.cc
index 199e5db..8318f15 100644
--- a/chrome/browser/dom_ui/new_tab_ui.cc
+++ b/chrome/browser/dom_ui/new_tab_ui.cc
@@ -382,16 +382,19 @@ MostVisitedHandler::MostVisitedHandler(DOMUI* dom_ui)
   dom_ui_->RegisterMessageCallback("getMostVisited",
       NewCallback(this, &MostVisitedHandler::HandleGetMostVisited));
 
-  // Set up our sources for thumbnail and favicon data.
-  // Ownership is passed to the ChromeURLDataManager.
-  g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
-      NewRunnableMethod(&chrome_url_data_manager,
-                        &ChromeURLDataManager::AddDataSource,
-                        new DOMUIThumbnailSource(dom_ui->GetProfile())));
-  g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
-      NewRunnableMethod(&chrome_url_data_manager,
-                        &ChromeURLDataManager::AddDataSource,
-                        new DOMUIFavIconSource(dom_ui->GetProfile())));
+  // Set up our sources for thumbnail and favicon data. Since we may be in
+  // testing mode with no I/O thread, only add our handler when an I/O thread
+  // exists. Ownership is passed to the ChromeURLDataManager.
+  if (g_browser_process->io_thread()) {
+    g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
+        NewRunnableMethod(&chrome_url_data_manager,
+                          &ChromeURLDataManager::AddDataSource,
+                          new DOMUIThumbnailSource(dom_ui->GetProfile())));
+    g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
+        NewRunnableMethod(&chrome_url_data_manager,
+                          &ChromeURLDataManager::AddDataSource,
+                          new DOMUIFavIconSource(dom_ui->GetProfile())));
+  }
 
   // Get notifications when history is cleared.
   NotificationService* service = NotificationService::current();
@@ -1052,9 +1055,12 @@ NewTabUI::NewTabUI(WebContents* contents)
 
     NewTabHTMLSource* html_source = new NewTabHTMLSource();
 
-    g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(&chrome_url_data_manager,
-            &ChromeURLDataManager::AddDataSource,
-            html_source));
+    // In testing mode there may not be an I/O thread.
+    if (g_browser_process->io_thread()) {
+      g_browser_process->io_thread()->message_loop()->PostTask(FROM_HERE,
+          NewRunnableMethod(&chrome_url_data_manager,
+              &ChromeURLDataManager::AddDataSource,
+              html_source));
+    }
   }
 }
diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc
index d1f8259..d22b052 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager.cc
@@ -6,6 +6,7 @@
 
 #include "base/command_line.h"
 #include "base/logging.h"
+#include "chrome/browser/dom_ui/dom_ui.h"
 #include "chrome/browser/dom_ui/dom_ui_factory.h"
 #include "chrome/browser/renderer_host/render_view_host.h"
 #include "chrome/browser/renderer_host/render_view_host_delegate.h"
@@ -57,7 +58,7 @@ void RenderViewHostManager::Init(Profile* profile,
 
 void RenderViewHostManager::Shutdown() {
   if (pending_render_view_host_)
-    CancelPendingRenderView();
+    CancelPending();
 
   // We should always have a main RenderViewHost.
   RenderViewHost* render_view_host = render_view_host_;
@@ -66,10 +67,15 @@ void RenderViewHostManager::Shutdown() {
 }
 
 RenderViewHost* RenderViewHostManager::Navigate(const NavigationEntry& entry) {
-  RenderViewHost* dest_render_view_host = UpdateRendererStateNavigate(entry);
+  // Create a pending RenderViewHost. It will give us the one we should use
+  RenderViewHost* dest_render_view_host = UpdateRendererStateForNavigate(entry);
   if (!dest_render_view_host)
     return NULL;  // We weren't able to create a pending render view host.
 
+  // This will possibly create (set to NULL) a DOM UI object for the pending
+  // page. We'll use this later to give the page special access.
+  pending_dom_ui_.reset(delegate_->CreateDOMUIForRenderManager(entry.url()));
+
   // If the current render_view_host_ isn't live, we should create it so
   // that we don't show a sad tab while the dest_render_view_host fetches
   // its first page.  (Bug 1145340)
@@ -92,7 +98,7 @@ RenderViewHost* RenderViewHostManager::Navigate(const NavigationEntry& entry) {
       dest_render_view_host->view()->Hide();
     } else {
       // This is our primary renderer, notify here as we won't be calling
-      // SwapToRenderView (which does the notify).
+      // CommitPending (which does the notify).
       RenderViewHostSwitchedDetails details;
       details.new_host = render_view_host_;
       details.old_host = NULL;
@@ -112,10 +118,8 @@ void RenderViewHostManager::Stop() {
 
   // If we are cross-navigating, we should stop the pending renderers.  This
   // will lead to a DidFailProvisionalLoad, which will properly destroy them.
-  if (cross_navigation_pending_) {
+  if (cross_navigation_pending_)
     pending_render_view_host_->Stop();
-
-  }
 }
 
 void RenderViewHostManager::SetIsLoading(bool is_loading) {
@@ -157,19 +161,25 @@ bool RenderViewHostManager::ShouldCloseTabOnUnresponsiveRenderer() {
 void RenderViewHostManager::DidNavigateMainFrame(
     RenderViewHost* render_view_host) {
   if (!cross_navigation_pending_) {
+    DCHECK(!pending_render_view_host_);
+
     // We should only hear this from our current renderer.
     DCHECK(render_view_host == render_view_host_);
+
+    // Even when there is no pending RVH, there may be a pending DOM UI.
+    if (pending_dom_ui_.get())
+      CommitPending();
     return;
   }
 
   if (render_view_host == pending_render_view_host_) {
     // The pending cross-site navigation completed, so show the renderer.
-    SwapToRenderView(&pending_render_view_host_, true);
+    CommitPending();
     cross_navigation_pending_ = false;
   } else if (render_view_host == render_view_host_) {
     // A navigation in the original page has taken place.  Cancel the pending
     // one.
-    CancelPendingRenderView();
+    CancelPending();
     cross_navigation_pending_ = false;
   } else {
     // No one else should be sending us DidNavigate in this state.
@@ -237,7 +247,7 @@ void RenderViewHostManager::ShouldClosePage(bool proceed) {
       pending_render_view_host_->SetNavigationsSuspended(false);
   } else {
     // Current page says to cancel.
-    CancelPendingRenderView();
+    CancelPending();
     cross_navigation_pending_ = false;
   }
 }
@@ -397,7 +407,7 @@ bool RenderViewHostManager::CreatePendingRenderView(SiteInstance* instance) {
     // Don't show the view until we get a DidNavigate from it.
     pending_render_view_host_->view()->Hide();
   } else {
-    CancelPendingRenderView();
+    CancelPending();
   }
   return success;
 }
@@ -415,9 +425,17 @@ RenderViewHost* RenderViewHostManager::CreateRenderViewHost(
   }
 }
 
-void RenderViewHostManager::SwapToRenderView(
-    RenderViewHost** new_render_view_host,
-    bool destroy_after) {
+void RenderViewHostManager::CommitPending() {
+  // First commit the DOM UI, if any.
+  dom_ui_.swap(pending_dom_ui_);
+  pending_dom_ui_.reset();
+
+  // It's possible for the pending_render_view_host_ to be NULL when we aren't
+  // crossing process boundaries. If so, we just needed to handle the DOM UI
+  // committing above and we're done.
+  if (!pending_render_view_host_)
+    return;
+
   // Remember if the page was focused so we can focus the new renderer in
   // that case.
   bool focus_render_view = render_view_host_->view() &&
@@ -431,8 +449,8 @@ void RenderViewHostManager::SwapToRenderView(
   RenderViewHost* old_render_view_host = render_view_host_;
 
   // Swap in the pending view and make it active.
-  render_view_host_ = (*new_render_view_host);
-  (*new_render_view_host) = NULL;
+  render_view_host_ = pending_render_view_host_;
+  pending_render_view_host_ = NULL;
 
   // If the view is gone, then this RenderViewHost died while it was hidden.
   // We ignored the RenderViewGone call at the time, so we should send it now
@@ -456,21 +474,20 @@ void RenderViewHostManager::SwapToRenderView(
       Source<NavigationController>(delegate_->GetControllerForRenderManager()),
       Details<RenderViewHostSwitchedDetails>(&details));
 
-  if (destroy_after)
-    old_render_view_host->Shutdown();
+  old_render_view_host->Shutdown();
 
   // Let the task manager know that we've swapped RenderViewHosts, since it
   // might need to update its process groupings.
   delegate_->NotifySwappedFromRenderManager();
 }
 
-RenderViewHost* RenderViewHostManager::UpdateRendererStateNavigate(
+RenderViewHost* RenderViewHostManager::UpdateRendererStateForNavigate(
     const NavigationEntry& entry) {
   // If we are cross-navigating, then we want to get back to normal and navigate
   // as usual.
   if (cross_navigation_pending_) {
     if (pending_render_view_host_)
-      CancelPendingRenderView();
+      CancelPending();
     cross_navigation_pending_ = false;
   }
 
@@ -485,7 +502,8 @@ RenderViewHost* RenderViewHostManager::UpdateRendererStateNavigate(
   if (ShouldTransitionCrossSite())
     new_instance = GetSiteInstanceForEntry(entry, curr_instance);
 
-  if (new_instance != curr_instance || ShouldSwapRenderViewsForNavigation(
+  if (new_instance != curr_instance ||
+      ShouldSwapRenderViewsForNavigation(
           delegate_->GetLastCommittedNavigationEntryForRenderManager(),
           &entry)) {
     // New SiteInstance.
@@ -504,7 +522,7 @@ RenderViewHost* RenderViewHostManager::UpdateRendererStateNavigate(
         // navigate.  Just switch to the pending RVH now and go back to non
         // cross-navigating (Note that we don't care about on{before}unload
         // handlers if the current RVH isn't live.)
-        SwapToRenderView(&pending_render_view_host_, true);
+        CommitPending();
         return render_view_host_;
       } else {
         NOTREACHED();
@@ -546,15 +564,17 @@ RenderViewHost* RenderViewHostManager::UpdateRendererStateNavigate(
   return render_view_host_;
 }
 
-void RenderViewHostManager::CancelPendingRenderView() {
+void RenderViewHostManager::CancelPending() {
   RenderViewHost* pending_render_view_host = pending_render_view_host_;
   pending_render_view_host_ = NULL;
   pending_render_view_host->Shutdown();
+
+  pending_dom_ui_.reset();
 }
 
 void RenderViewHostManager::CrossSiteNavigationCanceled() {
   DCHECK(cross_navigation_pending_);
   cross_navigation_pending_ = false;
   if (pending_render_view_host_)
-    CancelPendingRenderView();
+    CancelPending();
 }
diff --git a/chrome/browser/tab_contents/render_view_host_manager.h b/chrome/browser/tab_contents/render_view_host_manager.h
index 160ecff..9d9bb67 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.h
+++ b/chrome/browser/tab_contents/render_view_host_manager.h
@@ -6,10 +6,12 @@
 #define CHROME_BROWSER_TAB_CONTENTS_RENDER_VIEW_HOST_MANAGER_H_
 
 #include "base/basictypes.h"
+#include "base/scoped_ptr.h"
 #include "chrome/browser/renderer_host/render_view_host.h"
 #include "chrome/common/notification_registrar.h"
 #include "chrome/common/notification_observer.h"
 
+class DOMUI;
 class InterstitialPage;
 class NavigationController;
 class NavigationEntry;
@@ -48,6 +50,11 @@ class RenderViewHostManager : public NotificationObserver {
     virtual void NotifySwappedFromRenderManager() = 0;
     virtual NavigationController* GetControllerForRenderManager() = 0;
 
+    // Creates a DOMUI object for the given URL if one applies. Ownership of the
+    // returned pointer will be passed to the caller. If no DOMUI applies,
+    // returns NULL.
+    virtual DOMUI* CreateDOMUIForRenderManager(const GURL& url) = 0;
+
     // Returns the navigation entry of the current navigation, or NULL if there
     // is none.
     virtual NavigationEntry*
@@ -94,6 +101,17 @@ class RenderViewHostManager : public NotificationObserver {
     return render_view_host_->view();
   }
 
+  // Returns the pending render view host, or NULL if there is no pending one.
+  RenderViewHost* pending_render_view_host() const {
+    return pending_render_view_host_;
+  }
+
+  // Returns the current committed DOM UI or NULL if none applies.
+  DOMUI* dom_ui() const { return dom_ui_.get(); }
+
+  // Returns the DOM UI for the pending navigation, or NULL of none applies.
+  DOMUI* pending_dom_ui() const { return pending_dom_ui_.get(); }
+
   // Called when we want to instruct the renderer to navigate to the given
   // navigation entry. It may create a new RenderViewHost or re-use an existing
   // one. The RenderViewHost to navigate will be returned. Returns NULL if one
@@ -195,19 +213,15 @@ class RenderViewHostManager : public NotificationObserver {
                                        int routing_id,
                                        base::WaitableEvent* modal_dialog_event);
 
-  // Replaces the currently shown render_view_host_ with the RenderViewHost in
-  // the field pointed to by |new_render_view_host|, and then NULLs the field.
-  // Callers should only pass pointers to the pending_render_view_host_,
-  // interstitial_render_view_host_, or original_render_view_host_ fields of
-  // this object.  If |destroy_after|, this method will call
-  // ScheduleDeferredDestroy on the previous render_view_host_.
-  void SwapToRenderView(RenderViewHost** new_render_view_host,
-                        bool destroy_after);
+  // Sets the pending RenderViewHost/DOMUI to be the active one. Note that this
+  // doesn't require the pending render_view_host_ pointer to be non-NULL, since
+  // there could be DOM UI switching as well. Call this for every commit.
+  void CommitPending();
 
   // Helper method to terminate the pending RenderViewHost.
-  void CancelPendingRenderView();
+  void CancelPending();
 
-  RenderViewHost* UpdateRendererStateNavigate(const NavigationEntry& entry);
+  RenderViewHost* UpdateRendererStateForNavigate(const NavigationEntry& entry);
 
   // Our delegate, not owned by us. Guaranteed non-NULL.
   Delegate* delegate_;
@@ -224,13 +238,23 @@ class RenderViewHostManager : public NotificationObserver {
   // the RenderViewHosts that we create.
   RenderViewHostDelegate* render_view_delegate_;
 
-  // Our RenderView host. This object is responsible for all communication with
+  // Our RenderView host and its associated DOM UI (if any, will be NULL for
+  // non-DOM-UI pages). This object is responsible for all communication with
   // a child RenderView instance.
   RenderViewHost* render_view_host_;
+  scoped_ptr<DOMUI> dom_ui_;
 
-  // A RenderViewHost used to load a cross-site page.  This remains hidden
-  // while a cross-site request is pending until it calls DidNavigate.
+  // A RenderViewHost used to load a cross-site page. This remains hidden
+  // while a cross-site request is pending until it calls DidNavigate. It may
+  // have an associated DOM UI, in which case the DOM UI pointer will be non-
+  // NULL.
+  //
+  // The pending_dom_ui may be non-NULL even when the pending_render_view_host_
+  // is. This will happen when we're transitioning between two DOM UI pages:
+  // the RVH won't be swapped, so the pending pointer will be unused, but there
+  // will be a pending DOM UI associated with the navigation.
   RenderViewHost* pending_render_view_host_;
+  scoped_ptr<DOMUI> pending_dom_ui_;
 
   // The intersitial page currently shown if any, not own by this class
   // (the InterstitialPage is self-owned, it deletes itself when hidden).
diff --git a/chrome/browser/tab_contents/web_contents.cc b/chrome/browser/tab_contents/web_contents.cc
index fa7e904..6a8b721 100644
--- a/chrome/browser/tab_contents/web_contents.cc
+++ b/chrome/browser/tab_contents/web_contents.cc
@@ -347,9 +347,11 @@ void WebContents::Destroy() {
 }
 
 const string16& WebContents::GetTitle() const {
-  if (dom_ui_.get()) {
+  DOMUI* our_dom_ui = render_manager_.pending_dom_ui() ?
+      render_manager_.pending_dom_ui() : render_manager_.dom_ui();
+  if (our_dom_ui) {
     // Give the DOM UI the chance to override our title.
-    const string16& title = dom_ui_->overridden_title();
+    const string16& title = our_dom_ui->overridden_title();
     if (!title.empty())
       return title;
   }
@@ -361,14 +363,30 @@ SiteInstance* WebContents::GetSiteInstance() const {
 }
 
 bool WebContents::ShouldDisplayURL() {
-  if (dom_ui_.get())
-    return !dom_ui_->should_hide_url();
+  if (controller()->GetPendingEntry()) {
+    // When there is a pending entry, that should determine whether the URL is
+    // displayed (including getting the default behavior if the DOMUI doesn't
+    // specify).
+    if (render_manager_.pending_dom_ui())
+      return !render_manager_.pending_dom_ui()->should_hide_url();
+    return true;
+  }
+
+  if (render_manager_.dom_ui())
+    return !render_manager_.dom_ui()->should_hide_url();
   return true;
 }
 
 bool WebContents::ShouldDisplayFavIcon() {
-  if (dom_ui_.get())
-    return !dom_ui_->hide_favicon();
+  if (controller()->GetPendingEntry()) {
+    // See ShouldDisplayURL.
+    if (render_manager_.pending_dom_ui())
+      return !render_manager_.pending_dom_ui()->hide_favicon();
+    return true;
+  }
+  
+  if (render_manager_.dom_ui())
+    return !render_manager_.dom_ui()->hide_favicon();
   return true;
 }
 
@@ -402,10 +420,6 @@ std::wstring WebContents::GetStatusText() const {
 bool WebContents::NavigateToPendingEntry(bool reload) {
   const NavigationEntry& entry = *controller()->GetPendingEntry();
 
-  // This will possibly create (or NULL out) a DOM UI object for the page. We'll
-  // use this later when the page starts doing stuff to allow it to do so.
-  dom_ui_.reset(DOMUIFactory::CreateDOMUIForURL(this, entry.url()));
-
   RenderViewHost* dest_render_view_host = render_manager_.Navigate(entry);
   if (!dest_render_view_host)
     return false;  // Unable to create the desired render view host.
@@ -518,8 +532,20 @@ void WebContents::HideContents() {
 }
 
 bool WebContents::IsBookmarkBarAlwaysVisible() {
-  if (dom_ui_.get())
-    return dom_ui_->force_bookmark_bar_visible();
+  // We want the bookmarks bar to go with the committed entry. This way, when
+  // you're on the new tab page and navigate, the bookmarks bar doesn't
+  // disappear until the next load commits (the same time the page changes).
+  if (!controller()->GetLastCommittedEntry()) {
+    // However, when there is no committed entry (the first load of the tab),
+    // then we fall back on the pending entry. This means that the bookmarks bar
+    // will be visible before the new tab page load commits.
+    if (render_manager_.pending_dom_ui())
+      return render_manager_.pending_dom_ui()->force_bookmark_bar_visible();
+    return false;
+  }
+
+  if (render_manager_.dom_ui())
+    return render_manager_.dom_ui()->force_bookmark_bar_visible();
   return false;
 }
 
@@ -537,9 +563,12 @@ void WebContents::PopupNotificationVisibilityChanged(bool visible) {
 }
 
 bool WebContents::FocusLocationBarByDefault() {
-  // Allow the DOM Ui to override the default.
-  if (dom_ui_.get())
-    return dom_ui_->focus_location_bar_by_default();
+  // Allow the DOM UI to override the default. We use the pending DOM UI since
+  // that's what the user "just did" so should control what happens after they
+  // did it. Using the committed one would mean when they navigate from a DOMUI
+  // to a regular page, the location bar would be focused.
+  if (render_manager_.pending_dom_ui())
+    return render_manager_.pending_dom_ui()->focus_location_bar_by_default();
   return false;
 }
 
@@ -726,8 +755,10 @@ void WebContents::RenderViewCreated(RenderViewHost* render_view_host) {
   if (!entry)
     return;
 
-  if (dom_ui_.get()) {
-    dom_ui_->RenderViewCreated(render_view_host);
+  // When we're creating views, we're still doing initial setup, so we always
+  // use the pending DOM UI rather than any possibly existing committed one.
+  if (render_manager_.pending_dom_ui()) {
+    render_manager_.pending_dom_ui()->RenderViewCreated(render_view_host);
   } else if (entry->IsViewSourceMode()) {
     // Put the renderer in view source mode.
     render_view_host->Send(
@@ -1070,7 +1101,7 @@ void WebContents::DidDownloadImage(
 
 void WebContents::RequestOpenURL(const GURL& url, const GURL& referrer,
                                  WindowOpenDisposition disposition) {
-  if (dom_ui_.get()) {
+  if (render_manager_.dom_ui()) {
     // When we're a DOM UI, it will provide a page transition type for us (this
     // is so the new tab page can specify AUTO_BOOKMARK for automatically
     // generated suggestions).
@@ -1079,7 +1110,8 @@ void WebContents::RequestOpenURL(const GURL& url, const GURL& referrer,
     // want web sites to see a referrer of "chrome-ui://blah" (and some
     // chrome-ui URLs might have search terms or other stuff we don't want to
     // send to the site), so we send no referrer.
-    OpenURL(url, GURL(), disposition, dom_ui_->link_transition_type());
+    OpenURL(url, GURL(), disposition,
+            render_manager_.dom_ui()->link_transition_type());
   } else {
     OpenURL(url, referrer, disposition, PageTransition::LINK);
   }
@@ -1095,14 +1127,14 @@ void WebContents::DomOperationResponse(const std::string& json_string,
 
 void WebContents::ProcessDOMUIMessage(const std::string& message,
                                       const std::string& content) {
-  if (!dom_ui_.get()) {
+  if (!render_manager_.dom_ui()) {
     // We shouldn't get a DOM UI message when we haven't enabled the DOM UI.
     // Because the renderer might be owned and sending random messages, we need
     // to ignore these inproper ones.
     NOTREACHED();
     return;
   }
-  dom_ui_->ProcessDOMUIMessage(message, content);
+  render_manager_.dom_ui()->ProcessDOMUIMessage(message, content);
 }
 
 void WebContents::ProcessExternalHostMessage(const std::string& message,
@@ -1404,8 +1436,9 @@ WebPreferences WebContents::GetWebkitPrefs() {
   }
   DCHECK(!web_prefs.default_encoding.empty());
 
-  // Override some prefs when we're a DOM UI, or the pages won't work.
-  if (dom_ui_.get()) {
+  // Override some prefs when we're a DOM UI, or the pages won't work. This is
+  // called during setup, so we will always use the pending one.
+  if (render_manager_.pending_dom_ui()) {
     web_prefs.loads_images_automatically = true;
     web_prefs.javascript_enabled = true;
   }
@@ -1581,6 +1614,10 @@ void WebContents::UpdateRenderViewSizeForRenderManager() {
   view_->SizeContents(view_->GetContainerSize());
 }
 
+DOMUI* WebContents::CreateDOMUIForRenderManager(const GURL& url) {
+  return DOMUIFactory::CreateDOMUIForURL(this, url);
+}
+
 NavigationEntry*
 WebContents::GetLastCommittedNavigationEntryForRenderManager() {
   if (!controller())
@@ -1591,8 +1628,10 @@ WebContents::GetLastCommittedNavigationEntryForRenderManager() {
 bool WebContents::CreateRenderViewForRenderManager(
     RenderViewHost* render_view_host) {
   // When we're running a DOM UI, the RenderViewHost needs to be put in DOM UI
-  // mode before CreateRenderView is called.
-  if (dom_ui_.get())
+  // mode before CreateRenderView is called. When we're asked to create a
+  // RenderView, that means it's for the pending entry, so we have to use the
+  // pending DOM UI.
+  if (render_manager_.pending_dom_ui())
     render_view_host->AllowDOMUIBindings();
 
   RenderWidgetHostView* rwh_view = view_->CreateViewForWidget(render_view_host);
diff --git a/chrome/browser/tab_contents/web_contents.h b/chrome/browser/tab_contents/web_contents.h
index 21ca7eb..b3bdf15 100644
--- a/chrome/browser/tab_contents/web_contents.h
+++ b/chrome/browser/tab_contents/web_contents.h
@@ -109,6 +109,11 @@ class WebContents : public TabContents,
     return view_.get();
   }
 
+#ifdef UNIT_TEST
+  // Expose the render manager for testing.
+  RenderViewHostManager* render_manager() { return &render_manager_; }
+#endif
+
   // Page state getters & setters ----------------------------------------------
 
   bool is_starred() const { return is_starred_; }
@@ -449,6 +454,7 @@ class WebContents : public TabContents,
   virtual NavigationController* GetControllerForRenderManager() {
     return controller();
   }
+  virtual DOMUI* CreateDOMUIForRenderManager(const GURL& url);
   virtual NavigationEntry* GetLastCommittedNavigationEntryForRenderManager();
 
   // Initializes the given renderer if necessary and creates the view ID
@@ -635,11 +641,6 @@ class WebContents : public TabContents,
   // PluginInstaller, lazily created.
   scoped_ptr<PluginInstaller> plugin_installer_;
 
-  // When the current page is a DOM UI page, this will point to the specific
-  // DOMUI object handling it. When we don't have a DOM UI page, this will be
-  // null.
-  scoped_ptr<DOMUI> dom_ui_;
-
   // Handles downloading favicons.
   FavIconHelper fav_icon_helper_;
 
diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp
index 67a0492..49d389b 100644
--- a/chrome/chrome.gyp
+++ b/chrome/chrome.gyp
@@ -2026,6 +2026,7 @@
         'browser/cocoa/bookmark_bar_state_controller_unittest.mm',
         'browser/cocoa/bookmark_menu_bridge_unittest.mm',
         'browser/command_updater_unittest.cc',
+        'browser/dom_ui/dom_ui_unittest.cc',
         'browser/download/download_manager_unittest.cc',
         'browser/download/download_request_manager_unittest.cc',
         'browser/download/save_package_unittest.cc',
diff --git a/chrome/test/unit/unittests.vcproj b/chrome/test/unit/unittests.vcproj
index 9aa8dab..46f501b 100644
--- a/chrome/test/unit/unittests.vcproj
+++ b/chrome/test/unit/unittests.vcproj
@@ -452,6 +452,10 @@
 				>
 			</File>
 			<File
+				RelativePath="..\..\browser\dom_ui\dom_ui_unittest.cc"
+				>
+			</File>
+			<File
 				RelativePath="..\..\browser\download\download_manager_unittest.cc"
 				>
 			</File>