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..5d5fabf91411ffc79946d767642976f76cccfc2f 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);
@@ -153,14 +157,27 @@ void ImportBookmarksFile(
     if (is_bookmark)
       last_folder_is_empty = false;
 
+    importer::SearchEngineInfo search_engine;
+    bool is_valid_replaceable_url =
+        CanImportURLAsSearchEngine(url, shortcut,
+                                   title, &search_engine);

[Ilya Sherman, 2014/11/07 00:22:34]

nit: Looks like this doesn't need to wrap.

[Tapu Ghose, 2014/11/09 14:03:07]

Done.

+
     if (is_bookmark &&
         post_data.empty() &&
-        (valid_url_callback.is_null() || valid_url_callback.Run(url))) {
+        (valid_url_callback.is_null() || valid_url_callback.Run(url) ||
+         is_valid_replaceable_url)) {

[Ilya Sherman, 2014/11/07 00:22:34]

Should you also check "!shortcut.empty()" here?  It might be simpler to move the
inner if-stmt that you've added -- the one with the condition
"is_valid_replaceable_url && !shortcut.empty()" -- to be above this if-stmt,
rather than within it.

[Tapu Ghose, 2014/11/09 14:03:07]

Moved above the parent if-stmt.

       if (toolbar_folder_index > path.size() && !path.empty()) {
         NOTREACHED();  // error in parsing.
         break;
       }
 
+      // If bookmark contains a valid replaceable url and a keyword then import
+      // it as search engine.
+      if (is_valid_replaceable_url && !shortcut.empty()) {
+        search_engines->push_back(search_engine);
+        continue;
+      }
+
       ImportedBookmarkEntry entry;
       entry.creation_time = add_date;
       entry.url = url;
@@ -238,6 +255,37 @@ void ImportBookmarksFile(
   }
 }
 
+bool CanImportURLAsSearchEngine(const GURL& url,
+                                const base::string16& keyword,
+                                const base::string16& title,
+                                importer::SearchEngineInfo* search_engine) {
+  std::string url_spec =
+      url.is_valid() ? url.spec() : url.possibly_invalid_spec();

[Ilya Sherman, 2014/11/07 00:22:34]

nit: Why not just always grab the possibly_invalid_spec?

[Tapu Ghose, 2014/11/09 14:03:07]

Agreed.

+
+  if (url_spec.empty())
+    return false;
+
+  std::string raw_url = net::UnescapeURLComponent(
+      url_spec,
+      net::UnescapeRule::SPACES |
+      net::UnescapeRule::URL_SPECIAL_CHARS);

[Ilya Sherman, 2014/11/07 00:22:34]

Do you really need both of these unescape rules?  If so, could you please
clarify why?  Naively, it seems to me like you could skip unescaping the url
entirely.

[Tapu Ghose, 2014/11/09 14:03:07]

I think unescaping the url is necessary. For instance, url
"http://example.%s.com" without unescape rule appears as
"http://example.%25s.com". However, UnescapeRule::SPACES rule is not required. I
have removed this particular rule.

Moreover, unescaping the url is needed to show proper url in the UI ("Manager
Search Engines" from chrome://settings).

+
+  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);
+  TemplateURLData data;
+  data.SetURL(raw_url);
+  SearchTermsData search_terms_data;
+  return TemplateURL(data).SupportsReplacement(search_terms_data);

[Ilya Sherman, 2014/11/07 00:22:34]

nit: I believe that you can combine the above two lines as

"return TemplateURL(data).SupportsReplacement(SearchTermsData());"

[Tapu Ghose, 2014/11/09 14:03:06]

Done.

+}
+
 namespace internal {
 
 bool ParseCharsetFromLine(const std::string& line, std::string* charset) {