Merge 113149 - Reland 112770, but with test disabled.

Prevent a crash in the first run search engine selector.

It is possible to try and close the window now before the TemplateURLService is loaded. In this case we will not have a fallback search engine to select by default. So we will just reject attempts to close the window in this circumstance until it is loaded and a fallback choice has been set. It should be an edge case.

BUG=106078
TEST=unit test

Review URL: http://codereview.chromium.org/8771018

TBR=ben@chromium.org
Review URL: http://codereview.chromium.org/8870007

git-svn-id: svn://svn.chromium.org/chrome/branches/963/src@113514 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 98 insertions, 5 deletions

diff --git a/chrome/browser/ui/views/first_run_search_engine_view.cc b/chrome/browser/ui/views/first_run_search_engine_view.cc
index 1f1e13f..665189f 100644
--- a/chrome/browser/ui/views/first_run_search_engine_view.cc
+++ b/chrome/browser/ui/views/first_run_search_engine_view.cc
@@ -140,7 +140,8 @@ void SearchEngineChoice::SetChoiceViewBounds(int x, int y, int width,
 
 FirstRunSearchEngineView::FirstRunSearchEngineView(Profile* profile,
                                                    bool randomize)
-    : background_image_(NULL),
+    : views::ClientView(NULL, NULL),
+      background_image_(NULL),
       template_url_service_(TemplateURLServiceFactory::GetForProfile(profile)),
       text_direction_is_rtl_(base::i18n::IsRTL()),
       added_to_view_hierarchy_(false),
@@ -169,6 +170,15 @@ string16 FirstRunSearchEngineView::GetWindowTitle() const {
   return l10n_util::GetStringUTF16(IDS_FIRSTRUN_DLG_TITLE);
 }
 
+views::View* FirstRunSearchEngineView::GetContentsView() {
+  return this;
+}
+
+views::ClientView* FirstRunSearchEngineView::CreateClientView(
+    views::Widget* widget) {
+  return this;
+}
+
 void FirstRunSearchEngineView::WindowClosing() {
   // If the window is closed by clicking the close button, we default to the
   // engine in the first slot.
@@ -178,6 +188,21 @@ void FirstRunSearchEngineView::WindowClosing() {
     MessageLoop::current()->Quit();
 }
 
+views::Widget* FirstRunSearchEngineView::GetWidget() {
+  return View::GetWidget();
+}
+
+const views::Widget* FirstRunSearchEngineView::GetWidget() const {
+  return View::GetWidget();
+}
+
+bool FirstRunSearchEngineView::CanClose() {
+  // We need a valid search engine to set as default, so if the user tries to
+  // close the window before the template URL service is loaded, we must prevent
+  // this from happening.
+  return fallback_choice_ != NULL;
+}
+
 void FirstRunSearchEngineView::ButtonPressed(views::Button* sender,
                                              const views::Event& event) {
   ChooseSearchEngine(static_cast<SearchEngineChoice*>(sender));
@@ -477,7 +502,7 @@ void FirstRunSearchEngineView::AddSearchEnginesIfPossible() {
 
 void FirstRunSearchEngineView::ChooseSearchEngine(SearchEngineChoice* choice) {
   user_chosen_engine_ = true;
-  DCHECK(template_url_service_);
+  DCHECK(choice && template_url_service_);
   template_url_service_->SetSearchEngineDialogSlot(choice->slot());
   const TemplateURL* default_search = choice->GetSearchEngine();
   if (default_search)
diff --git a/chrome/browser/ui/views/first_run_search_engine_view.h b/chrome/browser/ui/views/first_run_search_engine_view.h
index 28ef439..841c2e5 100644
--- a/chrome/browser/ui/views/first_run_search_engine_view.h
+++ b/chrome/browser/ui/views/first_run_search_engine_view.h
@@ -13,6 +13,7 @@
 #include "ui/views/controls/button/text_button.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget_delegate.h"
+#include "ui/views/window/client_view.h"
 
 class Profile;
 class TemplateURL;
@@ -74,7 +75,8 @@ class SearchEngineChoice : public views::NativeTextButton {
 
 // This class displays a large search engine choice dialog view during
 // initial first run import.
-class FirstRunSearchEngineView : public views::WidgetDelegateView,
+class FirstRunSearchEngineView : public views::ClientView,
+                                 public views::WidgetDelegate,
                                  public views::ButtonListener,
                                  public TemplateURLServiceObserver {
  public:
@@ -84,10 +86,16 @@ class FirstRunSearchEngineView : public views::WidgetDelegateView,
 
   virtual ~FirstRunSearchEngineView();
 
-  // Overridden from views::WidgetDelegateView:
+  // Overridden from views::WidgetDelegate:
   virtual string16 GetWindowTitle() const OVERRIDE;
-  virtual views::View* GetContentsView() OVERRIDE { return this; }
+  virtual views::View* GetContentsView() OVERRIDE;
+  virtual views::ClientView* CreateClientView(views::Widget* widget) OVERRIDE;
   virtual void WindowClosing() OVERRIDE;
+  virtual views::Widget* GetWidget() OVERRIDE;
+  virtual const views::Widget* GetWidget() const OVERRIDE;
+
+  // Overridden from views::ClientView:
+  virtual bool CanClose() OVERRIDE;
 
   // Overridden from views::ButtonListener:
   virtual void ButtonPressed(views::Button* sender,
diff --git a/chrome/browser/ui/views/first_run_search_engine_view_unittest.cc b/chrome/browser/ui/views/first_run_search_engine_view_unittest.cc
index f0e191f..8d47890 100644
--- a/chrome/browser/ui/views/first_run_search_engine_view_unittest.cc
+++ b/chrome/browser/ui/views/first_run_search_engine_view_unittest.cc
@@ -7,7 +7,12 @@
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/browser/browser_process_sub_thread.h"
+#include "content/public/browser/notification_service.h"
+#include "content/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/views/focus/accelerator_handler.h"
 #include "ui/views/test/views_test_base.h"
@@ -49,3 +54,56 @@ TEST_F(FirstRunSearchEngineViewTest, ClosingSelectsFirstEngine) {
   ASSERT_TRUE(!template_urls.empty());
   EXPECT_EQ(template_urls.front(), service->GetDefaultSearchProvider());
 }
+
+TEST_F(FirstRunSearchEngineViewTest, ClosingBeforeServiceLoadedAbortsClose) {
+  // This ensures the current thread is named the UI thread, so code that checks
+  // that this is the UI thread doesn't assert.
+  content::TestBrowserThread ui_thread(content::BrowserThread::UI,
+                                       message_loop());
+  content::BrowserProcessSubThread db_thread(content::BrowserThread::DB);
+  db_thread.StartWithOptions(base::Thread::Options());
+
+  TestingProfile profile;
+  // We need to initialize the web database before accessing the template url
+  // service otherwise the template url service will init itself synchronously
+  // and appear to be loaded.
+  profile.CreateWebDataService(false);
+  profile.CreateTemplateURLService();
+
+  // Instead of giving the template url service a chance to load, try and close
+  // the window immediately.
+  FirstRunSearchEngineView* contents =
+      new FirstRunSearchEngineView(&profile, false);
+  contents->set_quit_on_closing(false);
+  views::Widget* window = views::Widget::CreateWindow(contents);
+  window->Show();
+  EXPECT_TRUE(window->IsVisible());
+  window->Close();
+  // The window wouldn't actually be closed until a return to the message loop,
+  // which we don't want to spin here because the window shouldn't have closed
+  // in the correct case. The window is however actually hidden immediately when
+  // the window is allowed to close, so we can test for visibility to make sure
+  // it hasn't.
+  EXPECT_TRUE(window->IsVisible());
+
+  // Now let the template url service a chance to load.
+  ui_test_utils::WindowedNotificationObserver service_load_observer(
+      chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED,
+      content::NotificationService::AllSources());
+  service_load_observer.Wait();
+
+  // .. and try and close the window. It should be allowed to now.
+  window->Close();
+  EXPECT_FALSE(window->IsVisible());
+
+  // Allow the window to actually close.
+  RunPendingMessages();
+
+  // Verify goodness. Because we actually went to the trouble of starting the
+  // WebDB, we will have real data in the model, so we can verify a choice was
+  // made without having to seed the model with dummy data.
+  TemplateURLService* service =
+      TemplateURLServiceFactory::GetForProfile(&profile);
+  ASSERT_TRUE(service != NULL);
+  EXPECT_TRUE(service->GetDefaultSearchProvider() != NULL);
+}
diff --git a/ui/views/test/views_test_base.h b/ui/views/test/views_test_base.h
index 51a9df6..eed6889 100644
--- a/ui/views/test/views_test_base.h
+++ b/ui/views/test/views_test_base.h
@@ -35,6 +35,8 @@ class ViewsTestBase : public testing::Test {
     views_delegate_.reset(views_delegate);
   }
 
+  MessageLoop* message_loop() { return &message_loop_; }
+
  private:
   MessageLoopForUI message_loop_;
   scoped_ptr<TestViewsDelegate> views_delegate_;