Clicks broken in NTP section of Search overlay

This removes prior solution where a duplicate web_contents for the NTP was
stored in the search_tab_helper.  Replaces that solution with this where the
main browser window's web view is reparented into the search overlay when the
search overlay is active and displaying the NTP.  When the NTP is dismissed or
navigated away, the web view is reparented back to the main browser_view's
contents container as per usual.

BUG=133529
TEST=--enable-instant-extended-api, load NTP, click on a most visited link.  Expect navigation in main browser window.
R=sky@chromium.org


Review URL: https://chromiumcodereview.appspot.com/10832216

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

chrome/browser/ui/search/search_tab_helper.cc

@@ -48,23 +48,6 @@ SearchTabHelper::SearchTabHelper(
 SearchTabHelper::~SearchTabHelper() {
 }
 
-content::WebContents* SearchTabHelper::GetNTPWebContents() {
-  if (!ntp_web_contents_.get()) {
-    ntp_web_contents_.reset(content::WebContents::Create(
-        model_.tab_contents()->profile(),
-        model_.tab_contents()->web_contents()->GetSiteInstance(),
-        MSG_ROUTING_NONE,
-        NULL,
-        NULL));
-    ntp_web_contents_->GetController().LoadURL(
-        GURL(chrome::kChromeUINewTabURL),
-        content::Referrer(),
-        content::PAGE_TRANSITION_START_PAGE,
-        std::string());
-  }
-  return ntp_web_contents_.get();
-}
-
 void SearchTabHelper::OmniboxEditModelChanged(OmniboxEditModel* edit_model) {
   if (!is_search_enabled_)
     return;
@@ -92,7 +75,6 @@ void SearchTabHelper::NavigateToPendingEntry(
     return;
 
   UpdateModel(url);
-  FlushNTP(url);
 }
 
 void SearchTabHelper::Observe(
@@ -103,7 +85,6 @@ void SearchTabHelper::Observe(
   content::LoadCommittedDetails* committed_details =
       content::Details<content::LoadCommittedDetails>(details).ptr();
   UpdateModel(committed_details->entry->GetURL());
-  FlushNTP(committed_details->entry->GetURL());
 }
 
 void SearchTabHelper::UpdateModel(const GURL& url) {
@@ -115,12 +96,5 @@ void SearchTabHelper::UpdateModel(const GURL& url) {
   model_.SetMode(Mode(type, true));
 }
 
-void SearchTabHelper::FlushNTP(const GURL& url) {
-  if (!IsNTP(url) &&
-      !google_util::IsInstantExtendedAPIGoogleSearchUrl(url.spec())) {
-    ntp_web_contents_.reset();
-  }
-}
-
 }  // namespace search
 }  // namespace chrome

chrome/browser/ui/search/search_tab_helper.h

@@ -6,7 +6,6 @@
 #define CHROME_BROWSER_UI_SEARCH_SEARCH_TAB_HELPER_H_
 
 #include "base/basictypes.h"
-#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/ui/search/search_model.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -15,10 +14,6 @@
 class OmniboxEditModel;
 class TabContents;
 
-namespace content {
-class WebContents;
-};
-
 namespace chrome {
 namespace search {
 
@@ -34,9 +29,6 @@ class SearchTabHelper : public content::WebContentsObserver,
     return &model_;
   }
 
-  // Lazily create web contents for NTP.  Owned by SearchTabHelper.
-  content::WebContents* GetNTPWebContents();
-
   // Invoked when the OmniboxEditModel changes state in some way that might
   // affect the search mode.
   void OmniboxEditModelChanged(OmniboxEditModel* edit_model);
@@ -55,17 +47,11 @@ class SearchTabHelper : public content::WebContentsObserver,
   // Sets the mode of the model based on |url|.
   void UpdateModel(const GURL& url);
 
-  // On navigation away from NTP and Search pages, delete |ntp_web_contents_|.
-  void FlushNTP(const GURL& url);
-
   const bool is_search_enabled_;
 
   // Model object for UI that cares about search state.
   SearchModel model_;
 
-  // Lazily created web contents for NTP.
-  scoped_ptr<content::WebContents> ntp_web_contents_;
-
   content::NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(SearchTabHelper);

chrome/browser/ui/search/search_ui.cc

@@ -23,7 +23,10 @@ const int kNTPOmniboxFontSize = 18;
 const int kNTPOmniboxHeight = 40;
 // See the comments in browser_defaults on kAutocompleteEditFontPixelSize.
 const int kOmniboxFontSize = 16;
+// Relative to top of ContentsContainer.
 const int kOmniboxYPosition = 310;
+// Relative to top of WebView in search overlay.
+const int kOmniboxYOffset = -50;
 const int kSearchResultsHeight = 122;
 
 gfx::Rect GetNTPOmniboxBounds(const gfx::Size& web_contents_size) {
@@ -33,7 +36,7 @@ gfx::Rect GetNTPOmniboxBounds(const gfx::Size& web_contents_size) {
   int width = static_cast<int>(kNTPPageWidthRatio *
                                static_cast<double>(web_contents_size.width()));
   int x = (web_contents_size.width() - width) / 2;
-  return gfx::Rect(x, kOmniboxYPosition, width, 0);
+  return gfx::Rect(x, kOmniboxYOffset, width, 0);
 }
 
 gfx::Font GetNTPOmniboxFont(const gfx::Font& font) {

chrome/browser/ui/views/frame/browser_view.cc

@@ -1933,8 +1933,7 @@ void BrowserView::Init() {
   // SearchViewController doesn't work on windows yet.
 #if defined(USE_AURA)
   if (chrome::search::IsInstantExtendedAPIEnabled(browser_->profile())) {
-    search_view_controller_.reset(
-        new SearchViewController(browser_->profile(), contents_));
+    search_view_controller_.reset(new SearchViewController(contents_));
     omnibox_popup_view_parent =
         search_view_controller_->omnibox_popup_view_parent();
   }
@@ -2458,6 +2457,8 @@ void BrowserView::ProcessTabSelected(TabContents* new_contents) {
   // When we toggle the NTP floating bookmarks bar and/or the info bar,
   // we don't want any WebContents to be attached, so that we
   // avoid an unnecessary resize and re-layout of a WebContents.
+  // This also applies to the |search_view_controller_| logic, as it can
+  // reparent the |contents_container_|.
   if (change_tab_contents)
     contents_container_->SetWebContents(NULL);
   infobar_container_->ChangeTabContents(new_contents->infobar_tab_helper());
@@ -2467,8 +2468,6 @@ void BrowserView::ProcessTabSelected(TabContents* new_contents) {
         BookmarkBar::DONT_ANIMATE_STATE_CHANGE);
   }
   UpdateUIForContents(new_contents);
-  if (change_tab_contents)
-    contents_container_->SetWebContents(new_contents->web_contents());
 
 #if defined(USE_AURA)
   // |change_tab_contents| can mean same WebContents but different TabContents,
@@ -2477,6 +2476,9 @@ void BrowserView::ProcessTabSelected(TabContents* new_contents) {
     search_view_controller_->SetTabContents(new_contents);
 #endif
 
+  if (change_tab_contents)
+    contents_container_->SetWebContents(new_contents->web_contents());
+
   UpdateDevToolsForContents(new_contents);
   if (!browser_->tab_strip_model()->closing_all() && GetWidget()->IsActive() &&
       GetWidget()->IsVisible()) {

chrome/browser/ui/views/frame/contents_container.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/views/frame/contents_container.h"
 
 #include "base/logging.h"
+#include "ui/views/controls/webview/webview.h"
 
 using content::WebContents;
 
@@ -12,7 +13,7 @@ using content::WebContents;
 const char ContentsContainer::kViewClassName[] =
     "browser/ui/views/frame/ContentsContainer";
 
-ContentsContainer::ContentsContainer(views::View* active)
+ContentsContainer::ContentsContainer(views::WebView* active)
     : active_(active),
       overlay_(NULL),
       preview_(NULL),
@@ -24,6 +25,16 @@ ContentsContainer::ContentsContainer(views::View* active)
 ContentsContainer::~ContentsContainer() {
 }
 
+void ContentsContainer::SetActive(views::WebView* active) {
+  if (active_)
+    RemoveChildView(active_);
+  active_ = active;
+  // Note the active view is always the first child.
+  if (active_)
+    AddChildViewAt(active_, 0);
+  Layout();
+}
+
 void ContentsContainer::SetOverlay(views::View* overlay) {
   if (overlay_)
     RemoveChildView(overlay_);
@@ -42,7 +53,7 @@ void ContentsContainer::MakePreviewContentsActiveContents() {
   Layout();
 }
 
-void ContentsContainer::SetPreview(views::View* preview,
+void ContentsContainer::SetPreview(views::WebView* preview,
                                    WebContents* preview_web_contents) {
   if (preview == preview_)
     return;
@@ -77,7 +88,8 @@ void ContentsContainer::Layout() {
   int content_y = active_top_margin_;
   int content_height = std::max(0, height() - content_y);
 
-  active_->SetBounds(0, content_y, width(), content_height);
+  if (active_)
+    active_->SetBounds(0, content_y, width(), content_height);
 
   if (overlay_)
     overlay_->SetBounds(0, 0, width(), height());

chrome/browser/ui/views/frame/contents_container.h

@@ -11,6 +11,10 @@ namespace content {
 class WebContents;
 }
 
+namespace views {
+class WebView;
+}
+
 // ContentsContainer is responsible for managing the WebContents views.
 // ContentsContainer has up to two children: one for the currently active
 // WebContents and one for instant's WebContents.
@@ -19,9 +23,15 @@ class ContentsContainer : public views::View {
   // Internal class name
   static const char kViewClassName[];
 
-  explicit ContentsContainer(views::View* active);
+  explicit ContentsContainer(views::WebView* active);
   virtual ~ContentsContainer();
 
+  // Sets the active web view first in stacking order.  This view is deactivated
+  // when the |SearchViewController| is displaying the NTP, and activated
+  // otherwise.  Deactivation removes the active view from the view hierarchy.
+  void SetActive(views::WebView* active);
+  views::WebView* active() { return active_; }
+
   // Sets the overlay. The overlay is sized to the bounds of this view.
   void SetOverlay(views::View* overlay);
   views::View* overlay() { return overlay_; }
@@ -32,7 +42,7 @@ class ContentsContainer : public views::View {
   void MakePreviewContentsActiveContents();
 
   // Sets the preview view. This does not delete the old.
-  void SetPreview(views::View* preview,
+  void SetPreview(views::WebView* preview,
                   content::WebContents* preview_web_contents);
 
   content::WebContents* preview_web_contents() const {
@@ -51,9 +61,9 @@ class ContentsContainer : public views::View {
   virtual std::string GetClassName() const OVERRIDE;
 
  private:
-  views::View* active_;
+  views::WebView* active_;
   views::View* overlay_;
-  views::View* preview_;
+  views::WebView* preview_;
   content::WebContents* preview_web_contents_;
 
   // The margin between the top and the active view. This is used to make the

chrome/browser/ui/views/search_view_controller.cc

@@ -236,10 +236,8 @@ void SearchViewController::OmniboxPopupViewParent::ChildPreferredSizeChanged(
 // SearchViewController --------------------------------------------------------
 
 SearchViewController::SearchViewController(
-    content::BrowserContext* browser_context,
     ContentsContainer* contents_container)
-    : browser_context_(browser_context),
-      contents_container_(contents_container),
+    : contents_container_(contents_container),
       location_bar_container_(NULL),
       state_(STATE_NOT_VISIBLE),
       tab_contents_(NULL),
@@ -340,7 +338,6 @@ void SearchViewController::UpdateState() {
       break;
   }
   SetState(new_state);
-  MaybeLoadNTP();
 }
 
 void SearchViewController::SetState(State state) {
@@ -357,9 +354,6 @@ void SearchViewController::SetState(State state) {
     case STATE_NTP:
       DestroyViews();
       CreateViews();
-      // TODO(dhollowa): This is temporary. The |content_view_| should pull its
-      // web contents from the current tab's |search_tab_helper|.
-      content_view_->LoadInitialURL(GURL(chrome::kChromeUINewTabURL));
       break;
 
     case STATE_ANIMATING:
@@ -432,8 +426,11 @@ void SearchViewController::CreateViews() {
   logo_view_->SetPaintToLayer(true);
   logo_view_->SetFillsBoundsOpaquely(false);
 
-  content_view_ = new views::WebView(browser_context_);
-  content_view_->SetFillsBoundsOpaquely(false);
+  // Reparent the main web contents view out of |contents_container_| and
+  // in to |ntp_view_| below.  Reparent back in destructor.
+  content_view_ = contents_container_->active();
+  DCHECK(content_view_);
+  contents_container_->SetActive(NULL);
 
   ntp_view_->SetLayoutManager(
       new NTPViewLayoutManager(logo_view_, content_view_));
@@ -458,10 +455,15 @@ void SearchViewController::DestroyViews() {
   omnibox_popup_view_parent_->parent()->RemoveChildView(
       omnibox_popup_view_parent_);
 
-  if (content_view_)
-    content_view_->SetWebContents(NULL);
-
+  // Restore control/parenting of the web_contents back to the
+  // |main_contents_view_|.
+  ntp_view_->SetLayoutManager(NULL);
+  ntp_view_->RemoveChildView(content_view_);
+  if (content_view_->web_contents())
+    content_view_->web_contents()->GetNativeView()->layer()->SetOpacity(1.0f);
+  contents_container_->SetActive(content_view_);
   contents_container_->SetOverlay(NULL);
+
   delete search_container_;
   search_container_ = NULL;
   ntp_view_ = NULL;
@@ -480,14 +482,6 @@ void SearchViewController::PopupVisibilityChanged() {
   }
 }
 
-void SearchViewController::MaybeLoadNTP() {
-  if (state_ != STATE_NTP || !content_view_)
-    return;
-
-  content_view_->SetWebContents(
-      tab_contents_->search_tab_helper()->GetNTPWebContents());
-}
-
 chrome::search::SearchModel* SearchViewController::search_model() {
   return tab_contents_ ? tab_contents_->search_tab_helper()->model() : NULL;
 }

chrome/browser/ui/views/search_view_controller.h

@@ -38,8 +38,7 @@ class SearchViewController
     : public chrome::search::SearchModelObserver,
       public ui::ImplicitAnimationObserver {
  public:
-  SearchViewController(content::BrowserContext* browser_context,
-                       ContentsContainer* contents_container);
+  explicit SearchViewController(ContentsContainer* contents_container);
   virtual ~SearchViewController();
 
   views::View* omnibox_popup_view_parent();
@@ -102,19 +101,12 @@ class SearchViewController
   // Invoked when the visibility of the omnibox popup changes.
   void PopupVisibilityChanged();
 
-  // Load the NTP from the associated |SearchTabHelper| if in NTP mode
-  // and the current |tab_contents_| has changed.
-  void MaybeLoadNTP();
-
   // Access active search model.
   chrome::search::SearchModel* search_model();
 
   // Access active web contents.
   content::WebContents* web_contents();
 
-  // The profile.  Weak.
-  content::BrowserContext* browser_context_;
-
   // Where the overlay is placed.  Weak.
   ContentsContainer* contents_container_;
 
@@ -158,7 +150,11 @@ class SearchViewController
   views::View* search_container_;
   views::View* ntp_view_;
   views::View* logo_view_;
+
+  // An alias to |contents_container_->active()|, but reparented within
+  // |ntp_view_| when in the NTP state.
   views::WebView* content_view_;
+
   OmniboxPopupViewParent* omnibox_popup_view_parent_;
 
   DISALLOW_COPY_AND_ASSIGN(SearchViewController);