Importing certain bookmarks from firefox and HTML file as search engines.

chrome/utility/importer/firefox_importer.cc

Index: chrome/utility/importer/firefox_importer.cc
diff --git a/chrome/utility/importer/firefox_importer.cc b/chrome/utility/importer/firefox_importer.cc
index fdb14cd09472b666bb08d0070c65b2e20c471719..5506417213e39511d623efe5c5198dfde5e80f9a 100644
--- a/chrome/utility/importer/firefox_importer.cc
+++ b/chrome/utility/importer/firefox_importer.cc
@@ -50,10 +50,12 @@ void LoadDefaultBookmarks(const base::FilePath& app_path,
   urls->clear();
 
   std::vector<ImportedBookmarkEntry> bookmarks;
+  std::vector<importer::SearchEngineInfo> search_engines;
   bookmark_html_reader::ImportBookmarksFile(base::Callback<bool(void)>(),
                                             base::Callback<bool(const GURL&)>(),
                                             file,
                                             &bookmarks,
+                                            &search_engines,
                                             NULL);
   for (size_t i = 0; i < bookmarks.size(); ++i)
     urls->insert(bookmarks[i].url);
@@ -225,7 +227,7 @@ void FirefoxImporter::ImportBookmarks() {
     GetWholeBookmarkFolder(&db, &list, i, NULL);
 
   std::vector<ImportedBookmarkEntry> bookmarks;
-  std::vector<importer::URLKeywordInfo> url_keywords;
+  std::vector<importer::SearchEngineInfo> search_engines;
   FaviconMap favicon_map;
 
   // TODO(jcampan): http://b/issue?id=1196285 we do not support POST based
@@ -247,16 +249,23 @@ void FirefoxImporter::ImportBookmarks() {
   for (size_t i = 0; i < list.size(); ++i) {
     BookmarkItem* item = list[i];
 
-    if (item->type == TYPE_FOLDER) {
+    if (item->type != TYPE_BOOKMARK) {
       // Folders are added implicitly on adding children, so we only explicitly
       // add empty folders.
-      if (!item->empty_folder)
+      if ((item->type != TYPE_FOLDER) || !item->empty_folder)
         continue;
-    } else if (item->type == TYPE_BOOKMARK) {
-      // Import only valid bookmarks
-      if (!CanImportURL(item->url))
-        continue;
-    } else {
+    } else if (!CanImportURL(item->url)) {

[Peter Kasting, 2014/11/11 22:42:12]

This will only convert invalid bookmark URLs to search engines.  I'd think most
"search engines" would have valid bookmark URLs, e.g. "http://foo.com/?q=%s". 
Shouldn't we check CanImportURLAsSearchEngine() first?

[Tapu Ghose, 2014/11/15 05:22:20]

I think valid bookmark URLs are handled at line 327 (though it only looking for
URL with keywords).

[Peter Kasting, 2014/11/17 21:30:35]

It's really confusing that we add things as search engines in two different
places, and they have two different ways of constructing the SearchEngineInfo
object and two different URL validity checks.

Can we combine these into one place that handles everything, maybe by removing
the CanImportURL() check here and executing this stuff unconditionally?

[Tapu Ghose, 2014/12/07 02:49:54]

Agreed.

+      importer::SearchEngineInfo search_engine_info;
+      // Import invalid bookmark's keyword as search engine, provided that the
+      // bookmark's url is a valid search engine url.
+      if (!item->keyword.empty() &&

[Peter Kasting, 2014/11/11 22:42:12]

Shouldn't we be willing to import bookmarks with empty keywords too (and
autogenerate the keywords)?

[Tapu Ghose, 2014/11/15 05:22:20]

You are right.

+          bookmark_html_reader::CanImportURLAsSearchEngine(
+              item->url,
+              base::UTF8ToUTF16(item->keyword),
+              item->title,
+              &search_engine_info)) {
+        search_engines.push_back(search_engine_info);
+      }
       continue;
     }
 
@@ -316,11 +325,11 @@ void FirefoxImporter::ImportBookmarks() {
 
       // This bookmark has a keyword, we should import it.
       if (!item->keyword.empty() && item->url.is_valid()) {
-        importer::URLKeywordInfo url_keyword_info;
-        url_keyword_info.url = item->url;
-        url_keyword_info.keyword.assign(base::UTF8ToUTF16(item->keyword));
-        url_keyword_info.display_name = item->title;
-        url_keywords.push_back(url_keyword_info);
+        importer::SearchEngineInfo search_engine_info;
+        search_engine_info.url.assign(base::UTF8ToUTF16(item->url.spec()));
+        search_engine_info.keyword.assign(base::UTF8ToUTF16(item->keyword));
+        search_engine_info.display_name = item->title;
+        search_engines.push_back(search_engine_info);
       }
     }
   }
@@ -333,8 +342,8 @@ void FirefoxImporter::ImportBookmarks() {
         bridge_->GetLocalizedString(IDS_BOOKMARK_GROUP_FROM_FIREFOX);
     bridge_->AddBookmarks(bookmarks, first_folder_name);
   }
-  if (!url_keywords.empty() && !cancelled()) {
-    bridge_->SetKeywords(url_keywords, false);
+  if (!search_engines.empty() && !cancelled()) {
+    bridge_->SetKeywords(search_engines, false);
   }
   if (!favicon_map.empty() && !cancelled()) {
     std::vector<ImportedFaviconUsage> favicons;