Implement SearchTabHelperDelegate.

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index b4ef1c2bbf0e87f86d01122d49e8f88b50241170..68ffb799a7af530b488e95ef59c48f332dd85b09 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -59,6 +59,7 @@
 #include "chrome/browser/file_select_helper.h"
 #include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/google/google_url_tracker.h"
+#include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/infobars/simple_alert_infobar_delegate.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
@@ -131,6 +132,7 @@
 #include "chrome/browser/ui/tab_modal_confirm_dialog.h"
 #include "chrome/browser/ui/tabs/tab_menu_model.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/tabs/tab_strip_model_utils.h"
 #include "chrome/browser/ui/toolbar/toolbar_model_impl.h"
 #include "chrome/browser/ui/unload_controller.h"
 #include "chrome/browser/ui/validation_message_bubble.h"
@@ -773,12 +775,6 @@ void Browser::VisibleSSLStateChanged(content::WebContents* web_contents) {
     UpdateToolbar(false);
 }
 
-void Browser::OnWebContentsInstantSupportDisabled(
-    const content::WebContents* web_contents) {
-  DCHECK(web_contents);
-  if (tab_strip_model_->GetActiveWebContents() == web_contents)
-    UpdateToolbar(false);
-}
 
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, Assorted browser commands:
@@ -1754,6 +1750,47 @@ void Browser::ConfirmAddSearchProvider(TemplateURL* template_url,
 }
 
 ///////////////////////////////////////////////////////////////////////////////
+// Browser, SearchTabHelperDelegate implementation:
+
+void Browser::NavigateToURL(const GURL& url,
+                            WindowOpenDisposition disposition,
+                            content::WebContents* source_contents) {
+  DCHECK(source_contents);
+  chrome::NavigateParams params(this, url,
+                                content::PAGE_TRANSITION_AUTO_BOOKMARK);

[Peter Kasting, 2014/04/08 20:47:52]

What kinds of navigations are these?  I want to be sure AUTO_BOOKMARK is
actually correct.

[kmadhusu, 2014/04/08 21:11:36]

When an user clicks on a most visited thumbnail, we invoke this function to
navigate the page to the most visited item url.

[Peter Kasting, 2014/04/08 21:45:46]

You may want to encode that in the function name, or at least its comments: e.g.
name it "NavigateOnThumbnailClick()" or something, then write a comment in here
that we're guaranteed that AUTO_BOOKMARK is the right transition since this only
gets called for thumbnail clicks.

This will discourage people in the future from seeing this as "ah ha, an easy
way to navigate in response to anything!" and adding more callers which want a
different transition.

[kmadhusu, 2014/04/09 00:00:42]

Renamed "NavigateToURL" to "NavigateOnClick" and added a detailed comment
regarding the page transition. 

[Peter Kasting, 2014/04/09 00:03:24]

I don't think that's a great function name -- link clicks are clicks, but we'd
expect them to use PAGE_TRANSITION_LINK.

[kmadhusu, 2014/04/09 01:31:04]

Renamed "NavigateOnClick" to "NavigateOnThumbnailClick".

(repeating our offline conversation for reference): As of now, this function is
called to navigate to thumbnail URLs and to privileged Chrome URLs. Ideally, any
page transitions initiating from the search results page should be marked as
"LINK" instead of "AUTO_BOOKMARK". I will submit a follow up CL to fix this.

+  params.referrer = content::Referrer();
+  params.source_contents = source_contents;
+  params.disposition = disposition;
+  params.is_renderer_initiated = false;
+  params.initiating_profile = profile_;
+  chrome::Navigate(&params);
+}
+
+void Browser::OnWebContentsInstantSupportDisabled(
+    const content::WebContents* web_contents) {
+  DCHECK(web_contents);
+  if (tab_strip_model_->GetActiveWebContents() == web_contents)
+    UpdateToolbar(false);
+}
+
+OmniboxView* Browser::GetOmniboxView() {
+  LocationBar* location_bar = window_ ? window_->GetLocationBar() : NULL;
+  return location_bar ? location_bar->GetOmniboxView() : NULL;

[Peter Kasting, 2014/04/08 20:47:52]

Can |window_| ever not have a location bar, e.g. for App windows or something? 
If not, you could simplify to:

  return window_ ? window_->GetLocationBar()->GetOmniboxView() : NULL;

[kmadhusu, 2014/04/08 21:11:36]

Done. SearchTabHelper invokes this function. window_->GetLocationBar() will not
be NULL. Simplified as per your suggestion.

[Peter Kasting, 2014/04/08 21:45:46]

Also make sure to think through:
* Startup/shutdown, especially race conditions in processing events during
shutdown
* Testing

I'm happy if we can legally simplify this, I just want to make sure doing so is
truly robust.  When I asked the original question, I honestly didn't know the
answer.

[kmadhusu, 2014/04/09 00:00:42]

In the current code
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...),
we assumed that "window_->GetLocationBar()" is always non-null. I also ran
several tests to make sure this change doesn't regress the current behavior.
Having said that, the simplified code is safe when this function is invoked from
SearchTabHelper. 

But after committing this CL, this function can be invoked by any other code.
Just to be on the safer side, I prefer to go back to my previous version of code
and validate "window_->GetLocationBar()" return value. 

[Peter Kasting, 2014/04/09 00:03:24]

If your code expects this to be non-NULL, then don't check for NULL "just to be
safe".  If you have to add anything, add a DCHECK to imply that callers are not
allowed to call you if this is NULL.  But adding nothing at all seems
preferable.

[kmadhusu, 2014/04/09 01:31:04]

Done.

+}
+
+std::set<std::string> Browser::GetOpenUrls() {
+  history::TopSites* top_sites = profile_->GetTopSites();
+  if (!tab_strip_model_ || !top_sites) {
+    NOTREACHED();

[Peter Kasting, 2014/04/08 20:47:52]

Do not handle DCHECK failure.

Either just DCHECK that the variables are non-NULL (or do nothing, if there's no
reason for a reader to wonder about them), or if they can legitimately be NULL,
remove the NOTREACHED().

[kmadhusu, 2014/04/08 21:11:36]

Done.

+    return std::set<std::string>();
+  }
+
+  std::set<std::string> open_urls;
+  chrome::GetOpenUrls(*tab_strip_model_, *top_sites, &open_urls);
+  return open_urls;
+}
+
+///////////////////////////////////////////////////////////////////////////////
 // Browser, web_modal::WebContentsModalDialogManagerDelegate implementation:
 
 void Browser::SetWebContentsBlocked(content::WebContents* web_contents,
@@ -2136,6 +2173,7 @@ void Browser::SetAsDelegate(WebContents* web_contents, Browser* delegate) {
       SetDelegate(delegate);
   CoreTabHelper::FromWebContents(web_contents)->set_delegate(delegate);
   SearchEngineTabHelper::FromWebContents(web_contents)->set_delegate(delegate);
+  SearchTabHelper::FromWebContents(web_contents)->set_delegate(delegate);
   ZoomController::FromWebContents(web_contents)->set_observer(delegate);
   TranslateTabHelper* translate_tab_helper =
       TranslateTabHelper::FromWebContents(web_contents);