Focus the location bar on any navigation to the NTP. Specifically, this focuses it on back/forward navigations.

This becomes important for some upcoming omnibox M5 changes, and was what Glen and Scott and I agreed was the right behavior.

BUG=none
TEST=ctrl-t + navigate + click in page + hit back button = location bar focused
Review URL: http://codereview.chromium.org/1423003

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

9 files changed, 94 insertions, 13 deletions

diff --git a/chrome/browser/dom_ui/dom_ui_unittest.cc b/chrome/browser/dom_ui/dom_ui_unittest.cc
index 81df016..678229d4 100644
--- a/chrome/browser/dom_ui/dom_ui_unittest.cc
+++ b/chrome/browser/dom_ui/dom_ui_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -150,3 +150,55 @@ TEST_F(DOMUITest, StandardToDOMUI) {
 
   // Committing DOM UI is tested above.
 }
+
+class TabContentsForFocusTest : public TestTabContents {
+ public:
+  TabContentsForFocusTest(Profile* profile, SiteInstance* instance)
+      : TestTabContents(profile, instance), focus_called_(0) {
+  }
+
+  virtual void SetFocusToLocationBar() { ++focus_called_; }
+  int focus_called() const { return focus_called_; }
+
+ private:
+  int focus_called_;
+};
+
+TEST_F(DOMUITest, FocusOnNavigate) {
+  // Setup.  |tc| will be used to track when we try to focus the location bar.
+  TabContentsForFocusTest* tc = new TabContentsForFocusTest(
+      contents()->profile(),
+      SiteInstance::CreateSiteInstance(contents()->profile()));
+  tc->controller().CopyStateFrom(controller());
+  scoped_ptr<TestTabContents> tc_scoped_ptr(tc);
+  contents_.swap(tc_scoped_ptr);
+  profile_->CreateProfileSyncService();
+  int page_id = 200;
+
+  // Load the NTP.
+  GURL new_tab_url(chrome::kChromeUINewTabURL);
+  controller().LoadURL(new_tab_url, GURL(), PageTransition::LINK);
+  rvh()->SendNavigate(page_id, new_tab_url);
+
+  // Navigate to another page.
+  GURL next_url("http://google.com/");
+  int next_page_id = page_id + 1;
+  controller().LoadURL(next_url, GURL(), PageTransition::LINK);
+  pending_rvh()->SendNavigate(next_page_id, next_url);
+
+  // Navigate back.  Should focus the location bar.
+  int focus_called = tc->focus_called();
+  ASSERT_TRUE(controller().CanGoBack());
+  controller().GoBack();
+  pending_rvh()->SendNavigate(page_id, new_tab_url);
+  EXPECT_LT(focus_called, tc->focus_called());
+
+  // Navigate forward.  Shouldn't focus the location bar.
+  focus_called = tc->focus_called();
+  ASSERT_TRUE(controller().CanGoForward());
+  controller().GoForward();
+  pending_rvh()->SendNavigate(next_page_id, next_url);
+  EXPECT_EQ(focus_called, tc->focus_called());
+
+  contents_.swap(tc_scoped_ptr);
+}
diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc
index fd5057f..2619b67 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -438,20 +438,29 @@ bool RenderViewHostManager::CreatePendingRenderView(SiteInstance* instance) {
 }
 
 void RenderViewHostManager::CommitPending() {
-  // First commit the DOM UI, if any.
+  // First check whether we're going to want to focus the location bar after
+  // this commit.  We do this now because the navigation hasn't formally
+  // committed yet, so if we've already cleared |pending_dom_ui_| the call chain
+  // this triggers won't be able to figure out what's going on.
+  bool will_focus_location_bar = delegate_->FocusLocationBarByDefault();
+
+  // Next 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_)
+  if (!pending_render_view_host_) {
+    if (will_focus_location_bar)
+      delegate_->SetFocusToLocationBar();
     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() &&
-      render_view_host_->view()->HasFocus();
+  bool focus_render_view = !will_focus_location_bar &&
+      render_view_host_->view() && render_view_host_->view()->HasFocus();
 
   // Hide the current view and prepare to destroy it.
   // TODO(creis): Get the old RenderViewHost to send us an UpdateState message
@@ -475,7 +484,9 @@ void RenderViewHostManager::CommitPending() {
   // Make sure the size is up to date.  (Fix for bug 1079768.)
   delegate_->UpdateRenderViewSizeForRenderManager();
 
-  if (focus_render_view && render_view_host_->view())
+  if (will_focus_location_bar)
+    delegate_->SetFocusToLocationBar();
+  else if (focus_render_view && render_view_host_->view())
     render_view_host_->view()->Focus();
 
   RenderViewHostSwitchedDetails details;
diff --git a/chrome/browser/tab_contents/render_view_host_manager.h b/chrome/browser/tab_contents/render_view_host_manager.h
index 648fab3..d945f59 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.h
+++ b/chrome/browser/tab_contents/render_view_host_manager.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -60,6 +60,13 @@ class RenderViewHostManager
     // is none.
     virtual NavigationEntry*
         GetLastCommittedNavigationEntryForRenderManager() = 0;
+
+    // Returns true if the location bar should be focused by default rather than
+    // the page contents.
+    virtual bool FocusLocationBarByDefault() = 0;
+
+    // Focuses the location bar.
+    virtual void SetFocusToLocationBar() = 0;
   };
 
   // Both delegate pointers must be non-NULL and are not owned by this class.
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index 7035226..3bbbbf9 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -955,6 +955,11 @@ bool TabContents::FocusLocationBarByDefault() {
   return false;
 }
 
+void TabContents::SetFocusToLocationBar() {
+  if (delegate())
+    delegate()->SetFocusToLocationBar();
+}
+
 void TabContents::AddInfoBar(InfoBarDelegate* delegate) {
   if (delegate_ && !delegate_->infobars_enabled()) {
     delegate->InfoBarClosed();
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index c64fe16..385ed6d 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -431,10 +431,16 @@ class TabContents : public PageNavigator,
   // is true when using Shift-Tab).
   void FocusThroughTabTraversal(bool reverse);
 
+  // These next two functions are declared on RenderViewHostManager::Delegate
+  // but also accessed directly by other callers.
+
   // Returns true if the location bar should be focused by default rather than
   // the page contents. The view calls this function when the tab is focused
   // to see what it should do.
-  bool FocusLocationBarByDefault();
+  virtual bool FocusLocationBarByDefault();
+
+  // Focuses the location bar.
+  virtual void SetFocusToLocationBar();
 
   // Infobars ------------------------------------------------------------------
 
diff --git a/chrome/browser/tab_contents/tab_contents_view_gtk.cc b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
index 804cf56..c5db5c0 100644
--- a/chrome/browser/tab_contents/tab_contents_view_gtk.cc
+++ b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
@@ -257,7 +257,7 @@ void TabContentsViewGtk::Focus() {
 
 void TabContentsViewGtk::SetInitialFocus() {
   if (tab_contents()->FocusLocationBarByDefault())
-    tab_contents()->delegate()->SetFocusToLocationBar();
+    tab_contents()->SetFocusToLocationBar();
   else
     Focus();
 }
diff --git a/chrome/browser/tab_contents/tab_contents_view_mac.mm b/chrome/browser/tab_contents/tab_contents_view_mac.mm
index a2b622e..393d707 100644
--- a/chrome/browser/tab_contents/tab_contents_view_mac.mm
+++ b/chrome/browser/tab_contents/tab_contents_view_mac.mm
@@ -186,7 +186,7 @@ void TabContentsViewMac::Focus() {
 
 void TabContentsViewMac::SetInitialFocus() {
   if (tab_contents()->FocusLocationBarByDefault())
-    tab_contents()->delegate()->SetFocusToLocationBar();
+    tab_contents()->SetFocusToLocationBar();
   else
     [[cocoa_view_.get() window] makeFirstResponder:GetContentNativeView()];
 }
diff --git a/chrome/browser/views/tab_contents/tab_contents_view_gtk.cc b/chrome/browser/views/tab_contents/tab_contents_view_gtk.cc
index 880413b..e302838 100644
--- a/chrome/browser/views/tab_contents/tab_contents_view_gtk.cc
+++ b/chrome/browser/views/tab_contents/tab_contents_view_gtk.cc
@@ -267,7 +267,7 @@ void TabContentsViewGtk::Focus() {
 
 void TabContentsViewGtk::SetInitialFocus() {
   if (tab_contents()->FocusLocationBarByDefault())
-    tab_contents()->delegate()->SetFocusToLocationBar();
+    tab_contents()->SetFocusToLocationBar();
   else
     Focus();
 }
diff --git a/chrome/browser/views/tab_contents/tab_contents_view_win.cc b/chrome/browser/views/tab_contents/tab_contents_view_win.cc
index 6925765..ab0a397 100644
--- a/chrome/browser/views/tab_contents/tab_contents_view_win.cc
+++ b/chrome/browser/views/tab_contents/tab_contents_view_win.cc
@@ -207,7 +207,7 @@ void TabContentsViewWin::Focus() {
 
 void TabContentsViewWin::SetInitialFocus() {
   if (tab_contents()->FocusLocationBarByDefault())
-    tab_contents()->delegate()->SetFocusToLocationBar();
+    tab_contents()->SetFocusToLocationBar();
   else
     Focus();
 }