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

chrome/utility/importer/bookmark_html_reader.cc

Index: chrome/utility/importer/bookmark_html_reader.cc
diff --git a/chrome/utility/importer/bookmark_html_reader.cc b/chrome/utility/importer/bookmark_html_reader.cc
index e11dc1d7761f68631aede24d93a78fe37839f162..b4e2e896193eabfcecf8bbf6f91742f62870227f 100644
--- a/chrome/utility/importer/bookmark_html_reader.cc
+++ b/chrome/utility/importer/bookmark_html_reader.cc
@@ -10,10 +10,13 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/common/importer/imported_bookmark_entry.h"
 #include "chrome/common/importer/imported_favicon_usage.h"
 #include "chrome/utility/importer/favicon_reencode.h"
+#include "components/search_engines/search_terms_data.h"
+#include "components/search_engines/template_url.h"
 #include "net/base/data_url.h"
 #include "net/base/escape.h"
 #include "url/gurl.h"
@@ -91,6 +94,7 @@ void ImportBookmarksFile(
       const base::Callback<bool(const GURL&)>& valid_url_callback,
       const base::FilePath& file_path,
       std::vector<ImportedBookmarkEntry>* bookmarks,
+      std::vector<importer::SearchEngineInfo>* search_engines,
       std::vector<ImportedFaviconUsage>* favicons) {
   std::string content;
   base::ReadFileToString(file_path, &content);
@@ -150,6 +154,18 @@ void ImportBookmarksFile(
                                         &add_date, &post_data) ||
         internal::ParseMinimumBookmarkFromLine(line, charset, &title, &url);
 
+    importer::SearchEngineInfo search_engine;
+    bool is_valid_replaceable_url =
+        CanImportURLAsSearchEngine(url, shortcut, title, &search_engine);

[Ilya Sherman, 2014/11/10 23:00:22]

Optional nit: I'd probably inline this into the if-stmt condition, rather than
defining a separate variable for it.

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

Done.

+
+    // If bookmark contains a valid replaceable url and a keyword then import
+    // it as search engine.
+    if (is_bookmark && post_data.empty() && is_valid_replaceable_url &&
+        !shortcut.empty()) {
+      search_engines->push_back(search_engine);
+      continue;
+    }
+
     if (is_bookmark)
       last_folder_is_empty = false;
 
@@ -238,6 +254,34 @@ void ImportBookmarksFile(
   }
 }
 
+bool CanImportURLAsSearchEngine(const GURL& url,
+                                const base::string16& keyword,
+                                const base::string16& title,
+                                importer::SearchEngineInfo* search_engine) {

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

Rather than taking a SearchEngineInfo*, it seems like this function should just
take a (string) URL outparam.  The caller can construct the SearchEngineInfo
itself if it makes sense.  This also lets you remove the meaningless keyword and
title strings from the unittest code that exercises this function.

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

My understanding was that the caller indeed needs a SearchEngineInfo anyway. But
I agree with you. The caller can construct it, if needed. In this way we can
decouple the checking (which is the real purpose of this function) from
SearchEngineInfo creation.

+  std::string url_spec = url.possibly_invalid_spec();
+
+  if (url_spec.empty())
+    return false;
+
+  std::string raw_url = net::UnescapeURLComponent(
+      url_spec,
+      net::UnescapeRule::URL_SPECIAL_CHARS);
+
+  search_engine->url.assign(base::UTF8ToUTF16(raw_url));
+  search_engine->keyword = keyword;
+  search_engine->display_name = title;
+
+  const std::string kReplacementTerm("%s");
+  const std::string kSearchTerms("{searchTerms}");
+  // Replace replacement terms in the |raw_url| with {searchTerms}. This is
+  // necessary so that |raw_url| can be parsed for replacement terms by
+  // |ParseURL| which will be called through |SupportsReplacement|.
+  ReplaceSubstringsAfterOffset(&raw_url, 0, kReplacementTerm, kSearchTerms);

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

Please use TemplateURLRef::DisplayURLToURLRef() instead.

Why are we not doing this transform on the URL before putting it into
|search_engine|?  Seems like that would prevent us having to do the same
transform again later, as you've done in
in_process_importer_bridge.cc:CreateTemplateURL().

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

Many many thanks for this suggestion.
Agree with you.

+  TemplateURLData data;
+  data.SetURL(raw_url);
+  return TemplateURL(data).SupportsReplacement(SearchTermsData());
+}
+
 namespace internal {
 
 bool ParseCharsetFromLine(const std::string& line, std::string* charset) {