Implement SearchTabHelperDelegate.

chrome/browser/ui/browser.cc

 // Copyright 2012 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/ui/browser.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shellapi.h>
 #endif  // defined(OS_WIN)
@@ skipping 41 matching lines @@
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/download/download_shelf.h"
 #include "chrome/browser/extensions/browser_extension_window_controller.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/favicon/favicon_tab_helper.h"
 #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"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/pepper_broker_infobar_delegate.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_destroyer.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_metrics.h"
@@ skipping 52 matching lines @@
 #include "chrome/browser/ui/search/search_tab_helper.h"
 #include "chrome/browser/ui/search_engines/search_engine_tab_helper.h"
 #include "chrome/browser/ui/singleton_tabs.h"
 #include "chrome/browser/ui/status_bubble.h"
 #include "chrome/browser/ui/sync/browser_synced_window_delegate.h"
 #include "chrome/browser/ui/tab_contents/core_tab_helper.h"
 #include "chrome/browser/ui/tab_helpers.h"
 #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"
 #include "chrome/browser/ui/website_settings/permission_bubble_manager.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service.h"
 #include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/browser/ui/window_sizer/window_sizer.h"
 #include "chrome/browser/ui/zoom/zoom_controller.h"
 #include "chrome/browser/upgrade_detector.h"
 #include "chrome/browser/web_applications/web_app.h"
@@ skipping 622 matching lines @@
 }
 
 void Browser::VisibleSSLStateChanged(content::WebContents* web_contents) {
   // When the current tab's SSL state changes, we need to update the URL
   // bar to reflect the new state.
   DCHECK(web_contents);
   if (tab_strip_model_->GetActiveWebContents() == 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:
 
 void Browser::ToggleFullscreenModeWithExtension(const GURL& extension_url) {
   fullscreen_controller_->
       ToggleBrowserFullscreenModeWithExtension(extension_url);
 }
 
 bool Browser::SupportsWindowFeature(WindowFeature feature) const {
@@ skipping 955 matching lines @@
 
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, SearchEngineTabHelperDelegate implementation:
 
 void Browser::ConfirmAddSearchProvider(TemplateURL* template_url,
                                        Profile* profile) {
   window()->ConfirmAddSearchProvider(template_url, profile);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
+// 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,
                                     bool blocked) {
   int index = tab_strip_model_->GetIndexOfWebContents(web_contents);
   if (index == TabStripModel::kNoTab) {
     NOTREACHED();
     return;
   }
   tab_strip_model_->SetTabBlocked(index, blocked);
@@ skipping 362 matching lines @@
 void Browser::SetAsDelegate(WebContents* web_contents, Browser* delegate) {
   // WebContents...
   web_contents->SetDelegate(delegate);
 
   // ...and all the helpers.
   BookmarkTabHelper::FromWebContents(web_contents)->set_delegate(delegate);
   WebContentsModalDialogManager::FromWebContents(web_contents)->
       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);
   translate_tab_helper->translate_driver().set_observer(
       delegate ? delegate->translate_driver_observer_.get() : NULL);
 }
 
 void Browser::CloseFrame() {
   window_->Close();
 }
@@ skipping 214 matching lines @@
   if (contents && !allow_js_access) {
     contents->web_contents()->GetController().LoadURL(
         target_url,
         content::Referrer(),
         content::PAGE_TRANSITION_LINK,
         std::string());  // No extra headers.
   }
 
   return contents != NULL;
 }