Rejiggers the keyword editor so that the UI is independent of the model rather than being derived from it. This reduces the spaghetti somewhat. Also decouples the notion of a native view hierarchy from the location in TabContents::PageHasOSDD where the template URL fetcher is spawned. The Template URL Fetcher now simply retains a reference to the TabContents that created it. If the TabContents is destroyed before the fetch completes, we just discard the data retrieved without adding a keyword.

BUG=none
TEST=none

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

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

5 files changed, 174 insertions, 163 deletions

diff --git a/chrome/browser/search_engines/edit_keyword_controller_base.h b/chrome/browser/search_engines/edit_keyword_controller_base.h
deleted file mode 100644
index d01f6e9..0000000
--- a/chrome/browser/search_engines/edit_keyword_controller_base.h
+++ /dev/null
@@ -1,99 +0,0 @@
-// Copyright (c) 2009 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.
-
-#ifndef CHROME_BROWSER_SEARCH_ENGINES_EDIT_KEYWORD_CONTROLLER_BASE_H_
-#define CHROME_BROWSER_SEARCH_ENGINES_EDIT_KEYWORD_CONTROLLER_BASE_H_
-
-#include <string>
-
-#include "base/gfx/native_widget_types.h"
-
-class Profile;
-class TemplateURL;
-
-// EditKeywordControllerBase provides the platform independent logic and
-// interface for implementing a dialog for editing keyword searches.
-class EditKeywordControllerBase {
- public:
-  class Delegate {
-   public:
-    virtual ~Delegate() {}
-
-    // Invoked from the EditKeywordController when the user accepts the edits.
-    // NOTE: |template_url| is the value supplied to EditKeywordController's
-    // constructor, and may be null. A null value indicates a new TemplateURL
-    // should be created rather than modifying an existing TemplateURL.
-    virtual void OnEditedKeyword(const TemplateURL* template_url,
-                                 const std::wstring& title,
-                                 const std::wstring& keyword,
-                                 const std::wstring& url) = 0;
-  };
-
-  // Create and show the platform's implementation of the dialog.
-  static void Create(gfx::NativeWindow parent_window,
-                     const TemplateURL* template_url,
-                     Delegate* delegate,
-                     Profile* profile);
-
-  // The |template_url| and/or |edit_keyword_delegate| may be NULL.
-  EditKeywordControllerBase(const TemplateURL* template_url,
-                            Delegate* edit_keyword_delegate,
-                            Profile* profile);
-  virtual ~EditKeywordControllerBase() {}
-
- protected:
-  // Interface to platform specific view
-  virtual std::wstring GetURLInput() const = 0;
-  virtual std::wstring GetKeywordInput() const = 0;
-  virtual std::wstring GetTitleInput() const = 0;
-
-  // Check if content of Title entry is valid.
-  bool IsTitleValid() const;
-
-  // Returns true if the currently input URL is valid. The URL is valid if it
-  // contains no search terms and is a valid url, or if it contains a search
-  // term and replacing that search term with a character results in a valid
-  // url.
-  bool IsURLValid() const;
-
-  // Fixes up and returns the URL the user has input. The returned URL is
-  // suitable for use by TemplateURL.
-  std::wstring GetURL() const;
-
-  // Returns whether the currently entered keyword is valid. The keyword is
-  // valid if it is non-empty and does not conflict with an existing entry.
-  // NOTE: this is just the keyword, not the title and url.
-  bool IsKeywordValid() const;
-
-  // Deletes an unused TemplateURL, if its add was cancelled and it's not
-  // already owned by the TemplateURLModel.
-  void AcceptAddOrEdit();
-
-  // Deletes an unused TemplateURL, if its add was cancelled and it's not
-  // already owned by the TemplateURLModel.
-  void CleanUpCancelledAdd();
-
-  const TemplateURL* template_url() const {
-    return template_url_;
-  }
-
-  const Profile* profile() const {
-    return profile_;
-  }
-
- private:
-  // The TemplateURL we're displaying information for. It may be NULL. If we
-  // have a keyword_editor_view, we assume that this TemplateURL is already in
-  // the TemplateURLModel; if not, we assume it isn't.
-  const TemplateURL* template_url_;
-
-  // We may have been created by this, in which case we will call back to it on
-  // success to add/modify the entry.  May be NULL.
-  Delegate* edit_keyword_delegate_;
-
-  // Profile whose TemplateURLModel we're modifying.
-  Profile* profile_;
-};
-
-#endif  // CHROME_BROWSER_SEARCH_ENGINES_EDIT_KEYWORD_CONTROLLER_BASE_H_
diff --git a/chrome/browser/search_engines/edit_keyword_controller_base.cc b/chrome/browser/search_engines/edit_search_engine_controller.cc
index 8206d1d..110af38 100644
--- a/chrome/browser/search_engines/edit_keyword_controller_base.cc
+++ b/chrome/browser/search_engines/edit_search_engine_controller.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/search_engines/edit_keyword_controller_base.h"
+#include "chrome/browser/search_engines/edit_search_engine_controller.h"
 
 #include "chrome/browser/metrics/user_metrics.h"
 #include "chrome/browser/net/url_fixer_upper.h"
@@ -10,9 +10,9 @@
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_model.h"
 
-EditKeywordControllerBase::EditKeywordControllerBase(
+EditSearchEngineController::EditSearchEngineController(
     const TemplateURL* template_url,
-    Delegate* edit_keyword_delegate,
+    EditSearchEngineControllerDelegate* edit_keyword_delegate,
     Profile* profile)
     : template_url_(template_url),
       edit_keyword_delegate_(edit_keyword_delegate),
@@ -20,12 +20,14 @@ EditKeywordControllerBase::EditKeywordControllerBase(
   DCHECK(profile_);
 }
 
-bool EditKeywordControllerBase::IsTitleValid() const {
-  return !GetTitleInput().empty();
+bool EditSearchEngineController::IsTitleValid(
+    const std::wstring& title_input) const {
+  return !title_input.empty();
 }
 
-bool EditKeywordControllerBase::IsURLValid() const {
-  std::wstring url = GetURL();
+bool EditSearchEngineController::IsURLValid(
+    const std::wstring& url_input) const {
+  std::wstring url = GetFixedUpURL(url_input);
   if (url.empty())
     return false;
 
@@ -44,47 +46,24 @@ bool EditKeywordControllerBase::IsURLValid() const {
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, std::wstring()))).is_valid();
 }
 
-std::wstring EditKeywordControllerBase::GetURL() const {
-  std::wstring url;
-  TrimWhitespace(TemplateURLRef::DisplayURLToURLRef(GetURLInput()),
-                 TRIM_ALL, &url);
-  if (url.empty())
-    return url;
-
-  // Parse the string as a URL to determine the scheme. If we need to, add the
-  // scheme. As the scheme may be expanded (as happens with {google:baseURL})
-  // we need to replace the search terms before testing for the scheme.
-  TemplateURL t_url;
-  t_url.SetURL(url, 0, 0);
-  std::wstring expanded_url =
-      t_url.url()->ReplaceSearchTerms(t_url, L"x", 0, std::wstring());
-  url_parse::Parsed parts;
-  std::string scheme(
-      URLFixerUpper::SegmentURL(WideToUTF8(expanded_url), &parts));
-  if(!parts.scheme.is_valid()) {
-    scheme.append("://");
-    url.insert(0, UTF8ToWide(scheme));
-  }
-
-  return url;
-}
-
-bool EditKeywordControllerBase::IsKeywordValid() const {
-  std::wstring keyword = GetKeywordInput();
-  if (keyword.empty())
+bool EditSearchEngineController::IsKeywordValid(
+    const std::wstring& keyword_input) const {
+  if (keyword_input.empty())
     return true;  // Always allow no keyword.
   const TemplateURL* turl_with_keyword =
-      profile_->GetTemplateURLModel()->GetTemplateURLForKeyword(keyword);
+      profile_->GetTemplateURLModel()->GetTemplateURLForKeyword(keyword_input);
   return (turl_with_keyword == NULL || turl_with_keyword == template_url_);
 }
 
-void EditKeywordControllerBase::AcceptAddOrEdit() {
-  std::wstring url_string = GetURL();
+void EditSearchEngineController::AcceptAddOrEdit(
+    const std::wstring& title_input,
+    const std::wstring& keyword_input,
+    const std::wstring& url_input) {
+  std::wstring url_string = GetFixedUpURL(url_input);
   DCHECK(!url_string.empty());
-  std::wstring keyword = GetKeywordInput();
 
   const TemplateURL* existing =
-      profile_->GetTemplateURLModel()->GetTemplateURLForKeyword(keyword);
+      profile_->GetTemplateURLModel()->GetTemplateURLForKeyword(keyword_input);
   if (existing &&
       (!edit_keyword_delegate_ || existing != template_url_)) {
     // An entry may have been added with the same keyword string while the
@@ -105,8 +84,8 @@ void EditKeywordControllerBase::AcceptAddOrEdit() {
     // does in a similar situation (updating an existing TemplateURL with
     // data from a new one).
     TemplateURL* modifiable_url = const_cast<TemplateURL*>(template_url_);
-    modifiable_url->set_short_name(GetTitleInput());
-    modifiable_url->set_keyword(keyword);
+    modifiable_url->set_short_name(title_input);
+    modifiable_url->set_keyword(keyword_input);
     modifiable_url->SetURL(url_string, 0, 0);
     // TemplateURLModel takes ownership of template_url_.
     profile_->GetTemplateURLModel()->Add(modifiable_url);
@@ -114,13 +93,13 @@ void EditKeywordControllerBase::AcceptAddOrEdit() {
   } else {
     // Adding or modifying an entry via the Delegate.
     edit_keyword_delegate_->OnEditedKeyword(template_url_,
-                                            GetTitleInput(),
-                                            GetKeywordInput(),
+                                            title_input,
+                                            keyword_input,
                                             url_string);
   }
 }
 
-void EditKeywordControllerBase::CleanUpCancelledAdd() {
+void EditSearchEngineController::CleanUpCancelledAdd() {
   if (!edit_keyword_delegate_ && template_url_) {
     // When we have no Delegate, we know that the template_url_ hasn't yet been
     // added to the model, so we need to clean it up.
@@ -128,3 +107,30 @@ void EditKeywordControllerBase::CleanUpCancelledAdd() {
     template_url_ = NULL;
   }
 }
+
+std::wstring EditSearchEngineController::GetFixedUpURL(
+    const std::wstring& url_input) const {
+  std::wstring url;
+  TrimWhitespace(TemplateURLRef::DisplayURLToURLRef(url_input),
+                 TRIM_ALL, &url);
+  if (url.empty())
+    return url;
+
+  // Parse the string as a URL to determine the scheme. If we need to, add the
+  // scheme. As the scheme may be expanded (as happens with {google:baseURL})
+  // we need to replace the search terms before testing for the scheme.
+  TemplateURL t_url;
+  t_url.SetURL(url, 0, 0);
+  std::wstring expanded_url =
+      t_url.url()->ReplaceSearchTerms(t_url, L"x", 0, std::wstring());
+  url_parse::Parsed parts;
+  std::string scheme(
+      URLFixerUpper::SegmentURL(WideToUTF8(expanded_url), &parts));
+  if(!parts.scheme.is_valid()) {
+    scheme.append("://");
+    url.insert(0, UTF8ToWide(scheme));
+  }
+
+  return url;
+}
+
diff --git a/chrome/browser/search_engines/edit_search_engine_controller.h b/chrome/browser/search_engines/edit_search_engine_controller.h
new file mode 100644
index 0000000..6333dbf
--- /dev/null
+++ b/chrome/browser/search_engines/edit_search_engine_controller.h
@@ -0,0 +1,86 @@
+// Copyright (c) 2009 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.
+
+#ifndef CHROME_BROWSER_SEARCH_ENGINES_EDIT_SEARCH_ENGINE_CONTROLLER_H_
+#define CHROME_BROWSER_SEARCH_ENGINES_EDIT_SEARCH_ENGINE_CONTROLLER_H_
+
+#include <string>
+
+#include "base/gfx/native_widget_types.h"
+
+class Profile;
+class TemplateURL;
+
+class EditSearchEngineControllerDelegate {
+ public:
+  // Invoked from the EditSearchEngineController when the user accepts the
+  // edits. NOTE: |template_url| is the value supplied to
+  // EditSearchEngineController's constructor, and may be NULL. A NULL value
+  // indicates a new TemplateURL should be created rather than modifying an
+  // existing TemplateURL.
+  virtual void OnEditedKeyword(const TemplateURL* template_url,
+                               const std::wstring& title,
+                               const std::wstring& keyword,
+                               const std::wstring& url) = 0;
+};
+
+// EditSearchEngineController provides the core platform independent logic
+// for the Edit Search Engine dialog.
+class EditSearchEngineController {
+ public:
+  // The |template_url| and/or |edit_keyword_delegate| may be NULL.
+  EditSearchEngineController(
+      const TemplateURL* template_url,
+      EditSearchEngineControllerDelegate* edit_keyword_delegate,
+      Profile* profile);
+  ~EditSearchEngineController() {}
+
+  // Returns true if the value of |title_input| is a valid search engine name.
+  bool IsTitleValid(const std::wstring& title_input) const;
+
+  // Returns true if the value of |url_input| represents a valid search engine
+  // URL. The URL is valid if it contains no search terms and is a valid
+  // url, or if it contains a search term and replacing that search term with a
+  // character results in a valid url.
+  bool IsURLValid(const std::wstring& url_input) const;
+
+  // Returns true if the value of |keyword_input| represents a valid keyword.
+  // The keyword is valid if it is non-empty and does not conflict with an
+  // existing entry. NOTE: this is just the keyword, not the title and url.
+  bool IsKeywordValid(const std::wstring& keyword_input) const;
+
+  // Completes the add or edit of a search engine.
+  void AcceptAddOrEdit(const std::wstring& title_input,
+                       const std::wstring& keyword_input,
+                       const std::wstring& url_input);
+
+  // Deletes an unused TemplateURL, if its add was cancelled and it's not
+  // already owned by the TemplateURLModel.
+  void CleanUpCancelledAdd();
+
+  // Accessors.
+  const TemplateURL* template_url() const { return template_url_; }
+  const Profile* profile() const { return profile_; }
+
+ private:
+  // Fixes up and returns the URL the user has input. The returned URL is
+  // suitable for use by TemplateURL.
+  std::wstring GetFixedUpURL(const std::wstring& url_input) const;
+
+  // The TemplateURL we're displaying information for. It may be NULL. If we
+  // have a keyword_editor_view, we assume that this TemplateURL is already in
+  // the TemplateURLModel; if not, we assume it isn't.
+  const TemplateURL* template_url_;
+
+  // We may have been created by this, in which case we will call back to it on
+  // success to add/modify the entry.  May be NULL.
+  EditSearchEngineControllerDelegate* edit_keyword_delegate_;
+
+  // Profile whose TemplateURLModel we're modifying.
+  Profile* profile_;
+
+  DISALLOW_COPY_AND_ASSIGN(EditSearchEngineController);
+};
+
+#endif  // CHROME_BROWSER_SEARCH_ENGINES_EDIT_SEARCH_ENGINE_CONTROLLER_H_
diff --git a/chrome/browser/search_engines/template_url_fetcher.cc b/chrome/browser/search_engines/template_url_fetcher.cc
index 8b14aa3..d881d92 100644
--- a/chrome/browser/search_engines/template_url_fetcher.cc
+++ b/chrome/browser/search_engines/template_url_fetcher.cc
@@ -8,19 +8,24 @@
 
 #include "chrome/browser/net/url_fetcher.h"
 #include "chrome/browser/profile.h"
-#include "chrome/browser/search_engines/edit_keyword_controller_base.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_model.h"
 #include "chrome/browser/search_engines/template_url_parser.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
+#include "chrome/browser/tab_contents/tab_contents_delegate.h"
+#include "chrome/common/notification_registrar.h"
+#include "chrome/common/notification_source.h"
+#include "chrome/common/notification_type.h"
 
 // RequestDelegate ------------------------------------------------------------
-class TemplateURLFetcher::RequestDelegate : public URLFetcher::Delegate {
+class TemplateURLFetcher::RequestDelegate : public URLFetcher::Delegate,
+                                            public NotificationObserver {
  public:
   RequestDelegate(TemplateURLFetcher* fetcher,
                   const std::wstring& keyword,
                   const GURL& osdd_url,
                   const GURL& favicon_url,
-                  gfx::NativeWindow parent_window,
+                  TabContents* source,
                   bool autodetected)
       : ALLOW_THIS_IN_INITIALIZER_LIST(url_fetcher_(osdd_url,
                                                     URLFetcher::GET, this)),
@@ -29,11 +34,15 @@ class TemplateURLFetcher::RequestDelegate : public URLFetcher::Delegate {
         osdd_url_(osdd_url),
         favicon_url_(favicon_url),
         autodetected_(autodetected),
-        parent_window_(parent_window) {
+        source_(source) {
     url_fetcher_.set_request_context(fetcher->profile()->GetRequestContext());
     url_fetcher_.Start();
+    registrar_.Add(this,
+                   NotificationType::TAB_CONTENTS_DESTROYED,
+                   Source<TabContents>(source_));
   }
 
+  // URLFetcher::Delegate:
   // If data contains a valid OSDD, a TemplateURL is created and added to
   // the TemplateURLModel.
   virtual void OnURLFetchComplete(const URLFetcher* source,
@@ -43,6 +52,15 @@ class TemplateURLFetcher::RequestDelegate : public URLFetcher::Delegate {
                                   const ResponseCookies& cookies,
                                   const std::string& data);
 
+  // NotificationObserver:
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) {
+    DCHECK(type == NotificationType::TAB_CONTENTS_DESTROYED);
+    DCHECK(source == Source<TabContents>(source_));
+    source_ = NULL;
+  }
+
   // URL of the OSDD.
   const GURL& url() const { return osdd_url_; }
 
@@ -57,9 +75,12 @@ class TemplateURLFetcher::RequestDelegate : public URLFetcher::Delegate {
   const GURL favicon_url_;
   bool autodetected_;
 
-  // Used to determine where to place a confirmation dialog. May be NULL,
-  // in which case the confirmation will be centered in the screen if needed.
-  gfx::NativeWindow parent_window_;
+  // The TabContents where this request originated. Can be NULL if the
+  // originating tab is closed. If NULL, the engine is not added.
+  TabContents* source_;
+
+  // Handles registering for our notifications.
+  NotificationRegistrar registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(RequestDelegate);
 };
@@ -118,18 +139,15 @@ void TemplateURLFetcher::RequestDelegate::OnURLFetchComplete(
       // Mark the keyword as replaceable so it can be removed if necessary.
       template_url->set_safe_for_autoreplace(true);
       model->Add(template_url.release());
-    } else {
-#if defined(OS_WIN) || !defined(TOOLKIT_VIEWS)
+    } else if (source_ && source_->delegate()) {
       // Confirm addition and allow user to edit default choices. It's ironic
       // that only *non*-autodetected additions get confirmed, but the user
       // expects feedback that his action did something.
-      // The edit controller will take care of adding the URL to the model,
-      // which takes ownership, or of deleting it if the add is cancelled.
-      EditKeywordControllerBase::Create(parent_window_,
-                                        template_url.release(),
-                                        NULL, // no KeywordEditorView
-                                        fetcher_->profile());
-#endif
+      // The source TabContents' delegate takes care of adding the URL to the
+      // model, which takes ownership, or of deleting it if the add is
+      // cancelled.
+      source_->delegate()->ConfirmAddSearchProvider(template_url.release(),
+                                                    fetcher_->profile());
     }
   }
   fetcher_->RequestCompleted(this);
@@ -148,7 +166,7 @@ TemplateURLFetcher::~TemplateURLFetcher() {
 void TemplateURLFetcher::ScheduleDownload(const std::wstring& keyword,
                                           const GURL& osdd_url,
                                           const GURL& favicon_url,
-                                          const gfx::NativeWindow parent_window,
+                                          TabContents* source,
                                           bool autodetected) {
   DCHECK(!keyword.empty() && osdd_url.is_valid());
   // Make sure we aren't already downloading this request.
@@ -159,7 +177,7 @@ void TemplateURLFetcher::ScheduleDownload(const std::wstring& keyword,
   }
 
   requests_->push_back(
-      new RequestDelegate(this, keyword, osdd_url, favicon_url, parent_window,
+      new RequestDelegate(this, keyword, osdd_url, favicon_url, source,
                           autodetected));
 }
 
diff --git a/chrome/browser/search_engines/template_url_fetcher.h b/chrome/browser/search_engines/template_url_fetcher.h
index 48264dc..c6527d6 100644
--- a/chrome/browser/search_engines/template_url_fetcher.h
+++ b/chrome/browser/search_engines/template_url_fetcher.h
@@ -29,7 +29,7 @@ class TemplateURLFetcher {
   void ScheduleDownload(const std::wstring& keyword,
                         const GURL& osdd_url,
                         const GURL& favicon_url,
-                        const gfx::NativeWindow parent_window,
+                        TabContents* source,
                         bool autodetected);
 
  private: