Fixes bug in importer where we could set the default search provider

to one that doesn't support replacement. Also changed uniquing to
consider invalid OSDD urls. Need this to pick up Windows Live Search.

I also changed the ff importer to return out early on if it couldn't
find the value for the search provider. I encountered this do to
hitting a NOTREACHED.

BUG=1507
TEST=In IE set your default search to Live Search. Import from IE and
  make sure Chrome sets the default search to Live Search. Also make
  sure this didn't break keyword importing.

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

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

2 files changed, 50 insertions, 14 deletions

diff --git a/chrome/browser/importer/firefox_importer_utils.cc b/chrome/browser/importer/firefox_importer_utils.cc
index c9d6caa..5796e10 100644
--- a/chrome/browser/importer/firefox_importer_utils.cc
+++ b/chrome/browser/importer/firefox_importer_utils.cc
@@ -328,6 +328,14 @@ int GetFirefoxDefaultSearchEngineIndex(
   std::wstring default_se_name = UTF8ToWide(
       ReadPrefsJsValue(profile_path, "browser.search.selectedEngine"));
 
+  if (default_se_name.empty()) {
+    // browser.search.selectedEngine does not exist if the user has not changed
+    // from the default (or has selected the default).
+    // TODO: should fallback to 'browser.search.defaultengine' if selectedEngine
+    // is empty.
+    return -1;
+  }
+
   int default_se_index = -1;
   for (std::vector<TemplateURL*>::const_iterator iter = search_engines.begin();
        iter != search_engines.end(); ++iter) {
diff --git a/chrome/browser/importer/importer.cc b/chrome/browser/importer/importer.cc
index 4618f50..2a63072 100644
--- a/chrome/browser/importer/importer.cc
+++ b/chrome/browser/importer/importer.cc
@@ -139,16 +139,38 @@ void ProfileWriter::AddFavicons(
 
 typedef std::map<std::string, const TemplateURL*> HostPathMap;
 
+// Returns the key for the map built by BuildHostPathMap. If url_string is not
+// a valid URL, an empty string is returned, otherwise host+path is returned.
+static std::string HostPathKeyForURL(const std::wstring& url_string) {
+  GURL url(url_string);
+  return url.is_valid() ? url.host() + url.path() : std::string();
+}
+
 // Builds the key to use in HostPathMap for the specified TemplateURL. Returns
 // an empty string if a host+path can't be generated for the TemplateURL.
-// If an empty string is returned, it should not be added to HostPathMap.
-static std::string BuildHostPathKey(const TemplateURL* t_url) {
-  if (t_url->url() && t_url->url()->SupportsReplacement()) {
-    GURL search_url(t_url->url()->ReplaceSearchTerms(
-        *t_url, L"random string", TemplateURLRef::NO_SUGGESTIONS_AVAILABLE,
-        std::wstring()));
-    if (search_url.is_valid())
-      return search_url.host() + search_url.path();
+// If an empty string is returned, the TemplateURL should not be added to
+// HostPathMap.
+//
+// If |try_url_if_invalid| is true, and |t_url| isn't valid, a string is built
+// from the raw TemplateURL string. Use a value of true for |try_url_if_invalid|
+// when checking imported URLs as the imported URL may not be valid yet may
+// match the host+path of one of the default URLs. This is used to catch the
+// case of IE using an invalid OSDD URL for Live Search, yet the host+path
+// matches our prepopulate data. IE's URL for Live Search is something like
+// 'http://...{Language}...'. As {Language} is not a valid OSDD parameter value
+// the TemplateURL is invalid.
+static std::string BuildHostPathKey(const TemplateURL* t_url,
+                                    bool try_url_if_invalid) {
+  if (t_url->url()) {
+    if (try_url_if_invalid && !t_url->url()->IsValid())
+      return HostPathKeyForURL(t_url->url()->url());
+
+    if (t_url->url()->SupportsReplacement()) {
+      return HostPathKeyForURL(
+          t_url->url()->ReplaceSearchTerms(
+              *t_url, L"random string",
+              TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, std::wstring()));
+    }
   }
   return std::string();
 }
@@ -159,7 +181,7 @@ static void BuildHostPathMap(const TemplateURLModel& model,
                              HostPathMap* host_path_map) {
   std::vector<const TemplateURL*> template_urls = model.GetTemplateURLs();
   for (size_t i = 0; i < template_urls.size(); ++i) {
-    const std::string host_path = BuildHostPathKey(template_urls[i]);
+    const std::string host_path = BuildHostPathKey(template_urls[i], false);
     if (!host_path.empty()) {
       const TemplateURL* existing_turl = (*host_path_map)[host_path];
       if (!existing_turl ||
@@ -208,10 +230,11 @@ void ProfileWriter::AddKeywords(const std::vector<TemplateURL*>& template_urls,
     // sure the search engines we provide aren't replaced by those from the
     // imported browser.
     if (unique_on_host_and_path &&
-        host_path_map.find(BuildHostPathKey(t_url)) != host_path_map.end()) {
+        host_path_map.find(
+            BuildHostPathKey(t_url, true)) != host_path_map.end()) {
       if (default_keyword) {
         const TemplateURL* turl_with_host_path =
-            host_path_map[BuildHostPathKey(t_url)];
+            host_path_map[BuildHostPathKey(t_url, true)];
         if (turl_with_host_path)
           model->SetDefaultSearchProvider(turl_with_host_path);
         else
@@ -220,9 +243,14 @@ void ProfileWriter::AddKeywords(const std::vector<TemplateURL*>& template_urls,
       delete t_url;
       continue;
     }
-    model->Add(t_url);
-    if (default_keyword)
-      model->SetDefaultSearchProvider(t_url);
+    if (t_url->url() && t_url->url()->IsValid()) {
+      model->Add(t_url);
+      if (default_keyword && t_url->url() && t_url->url()->SupportsReplacement())
+        model->SetDefaultSearchProvider(t_url);
+    } else {
+      // Don't add invalid TemplateURLs to the model.
+      delete t_url;
+    }
   }
 }