Clicks broken in NTP section of Search overlay

chrome/browser/ui/views/search_view_controller.cc

 // Copyright (c) 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/views/search_view_controller.h"
 
 #include "chrome/browser/ui/search/search_model.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
 #include "chrome/browser/ui/search/search_types.h"
 #include "chrome/browser/ui/search/search_ui.h"
@@ skipping 218 matching lines @@
     parent()->Layout();
   if (child_visible_ != child->visible()) {
     child_visible_ = child->visible();
     search_view_controller_->PopupVisibilityChanged();
   }
 }
 
 // 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),
       search_container_(NULL),
       ntp_view_(NULL),
       logo_view_(NULL),
       content_view_(NULL),
       omnibox_popup_view_parent_(NULL) {
   omnibox_popup_view_parent_ = new OmniboxPopupViewParent(this);
 }
 
 SearchViewController::~SearchViewController() {
   if (search_model())
     search_model()->RemoveObserver(this);
 
   // If the |omnibox_popup_view_| isn't parented, delete it. Otherwise it'll be
   // deleted by its parent.
   if (!omnibox_popup_view_parent_->parent())
     delete omnibox_popup_view_parent_;
 }

[sky, 2012/08/09 00:39:14]

Are there any timing issues here if content_view_ has been reparented? By that I
mean I believe this could change the order the content_view_ is destroyed in. I
don't know if that'll cause problems.

[dhollowa, 2012/08/09 16:07:25]

It is certainly true that this has changed the destruction order of the
content_view_.  I'm not seeing any adverse effects after playing around with
different scenarios.  And from my read of ~BrowserView() the shutdown
dependencies are  on profile and browser, not on the content_view_.  Also, the
web_contents is not owned by the content_view_, so I think this is ok.

 
 views::View* SearchViewController::omnibox_popup_view_parent() {
   return omnibox_popup_view_parent_;
 }
 
 void SearchViewController::SetTabContents(TabContents* tab_contents) {
   if (tab_contents_ == tab_contents)
     return;
 
   if (search_model())
@@ skipping 60 matching lines @@
       } else {
         // Only enter into MODE_SEARCH if the omnibox is visible.
         if (omnibox_popup_view_parent_->is_child_visible())
           new_state = STATE_SEARCH;
         else
           new_state = STATE_NOT_VISIBLE;
       }
       break;
   }
   SetState(new_state);
-  MaybeLoadNTP();
 }
 
 void SearchViewController::SetState(State state) {
   if (state_ == state)
     return;
 
   State old_state = state_;
   state_ = state;
   switch (state_) {
     case STATE_NOT_VISIBLE:
       DestroyViews();
       break;
 
     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:
       // Should only animate from the ntp.
       DCHECK_EQ(STATE_NTP, old_state);
       StartAnimation();
       break;
 
     case STATE_SEARCH:
       DestroyViews();
@@ skipping 52 matching lines @@
   ntp_view_->layer()->SetMasksToBounds(true);
 
   logo_view_ = new views::View;
   logo_view_->SetLayoutManager(
       new FixedSizeLayoutManager(gfx::Size(300, 200)));
   logo_view_->set_background(
       views::Background::CreateSolidBackground(SK_ColorRED));
   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();
+  contents_container_->SetActive(NULL);

[sky, 2012/08/09 00:39:14]

How about a DCHECK(content_view_) between 431/432.

[dhollowa, 2012/08/09 16:07:25]

Done.

 
   ntp_view_->SetLayoutManager(
       new NTPViewLayoutManager(logo_view_, content_view_));
   ntp_view_->AddChildView(logo_view_);
   ntp_view_->AddChildView(content_view_);
 
   search_container_ =
       new SearchContainerView(ntp_view_, omnibox_popup_view_parent_);
   search_container_->SetPaintToLayer(true);
   search_container_->SetLayoutManager(new views::FillLayout);
   search_container_->layer()->SetMasksToBounds(true);
 
   contents_container_->SetOverlay(search_container_);
 }
 
 void SearchViewController::DestroyViews() {
   if (!search_container_)
     return;
 
   // We persist the parent of the omnibox so that we don't have to inject a new
   // parent into ToolbarView.
   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);
 
-  contents_container_->SetOverlay(NULL);
   delete search_container_;
   search_container_ = NULL;
   ntp_view_ = NULL;
   logo_view_ = NULL;
   content_view_ = NULL;
 
   state_ = STATE_NOT_VISIBLE;
 }
 
 void SearchViewController::PopupVisibilityChanged() {
   // Don't do anything while animating if the child is visible. Otherwise we'll
   // prematurely cancel the animation.
   if (state_ != STATE_ANIMATING ||
       !omnibox_popup_view_parent_->is_child_visible()) {
     UpdateState();
   }
 }
 
-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;
 }
 
 content::WebContents* SearchViewController::web_contents() {
   return tab_contents_ ? tab_contents_->web_contents() : NULL;
 }