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..0821359189c21bf7c84ad62ee35d48d806d66988 100644
--- a/chrome/utility/importer/bookmark_html_reader.cc
+++ b/chrome/utility/importer/bookmark_html_reader.cc
@@ -10,6 +10,7 @@
 #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"
@@ -91,6 +92,7 @@ void ImportBookmarksFile(
       const base::Callback<bool(const GURL&)>& valid_url_callback,
       const base::FilePath& file_path,
       std::vector<ImportedBookmarkEntry>* bookmarks,
+      std::vector<importer::URLKeywordInfo>* search_engine_keywords,
       std::vector<ImportedFaviconUsage>* favicons) {
   std::string content;
   base::ReadFileToString(file_path, &content);
@@ -153,14 +155,27 @@ void ImportBookmarksFile(
     if (is_bookmark)
       last_folder_is_empty = false;
 
+    importer::URLKeywordInfo search_engine_keyword;
+    bool is_valid_replaceable_url =

[Peter Kasting, 2014/10/07 23:17:38]

Nit: Why are the variable and the function named totally different things?

[Tapu Ghose, 2014/10/12 00:58:20]

Renamed to search_engine with type as SearchEngineInfo.

+        CanImportURLAsSearchEngine(url, shortcut,
+                                   title, &search_engine_keyword);

[Ilya Sherman, 2014/10/07 22:08:22]

nit: This formatting looks off.  Please run "git cl format" over this CL.

[Peter Kasting, 2014/10/07 23:17:38]

No, this formatting is legal.

[Tapu Ghose, 2014/10/12 00:58:20]

Acknowledged.

[Tapu Ghose, 2014/10/12 00:58:20]

Acknowledged.

+
     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)) {
       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_engine_keywords->push_back(search_engine_keyword);
+        continue;
+      }
+
       ImportedBookmarkEntry entry;
       entry.creation_time = add_date;
       entry.url = url;
@@ -238,6 +253,36 @@ void ImportBookmarksFile(
   }
 }
 
+bool CanImportURLAsSearchEngine(const GURL& url,
+                                const base::string16& keyword,
+                                const base::string16& title,
+                                importer::URLKeywordInfo* search_engine_info) {
+  // |url| can be imported as search engine if it contains replacement term(s).
+  // Usually, a |url| containing replacement term in host is considered as
+  // invalid. Hence, return false for valid |url|.

[Ilya Sherman, 2014/10/07 22:08:22]

Hmm, what if the replacement term is outside the host?  Are there any test cases
that would fail if this early return were removed entirely?

[Tapu Ghose, 2014/10/12 00:58:20]

Removed the condition. After looking at the flow in
chrome/utility/importer/firefox_importer.cc, I thought the function should only
be called for invalid urls. However, by looking at flow of ImportBookmarksFile,
I think we should call it even for valid urls.

Test case added.
(chrome/test/data/bookmark_html_reader/firefox_bookmark_keyword.html)

+  if (url.is_valid())
+    return false;
+
+  std::string raw_url = net::UnescapeURLComponent(
+      url.possibly_invalid_spec(),
+      net::UnescapeRule::SPACES |
+      net::UnescapeRule::URL_SPECIAL_CHARS |
+      net::UnescapeRule::REPLACE_PLUS_WITH_SPACE);

[Ilya Sherman, 2014/10/07 22:08:22]

Hmm, why do you need to unescape spaces?

[Peter Kasting, 2014/10/07 23:17:38]

And pluses?  Does this need to be some sort of "fully unescaped" string lest we
double-escape it later?  If so that needs to be clearer.

Can we just replace basically this whole function with something like this?:

  TemplateURLData data;
  data.SetURL(url_as_string);
  SearchTermsData search_terms_data;
  return TemplateURL(data).SupportsReplacement(search_terms_data);

This lets the TemplateURL machinery do the parsing instead of you rewriting it.

[Tapu Ghose, 2014/10/12 00:58:20]

Removed REPLACE_PLUS_WITH_SPACE.

[Tapu Ghose, 2014/10/12 00:58:20]

As I mentioned in Patch Set 1......

when I try to include "components/search_engines/template_url.h" in the header I
get following fatal error: 'components/metrics/proto/omnibox_event.pb.h' file
not found.

Since I am writing code in //chrome/utility, Ilya thought it as a dependency
issue and suggested to add //components/search_engines to the list of
dependencies in //chrome/chrome_utility.gypi. I added

"../components/search_engines.gypi:search_engines" as suggested. But no luck.

I was wondering if anyone can give little more knowledge on this issue. I am
definitely wishing to re-use the existing code (instead of writing new code with
same functionality....which makes no sense).

[Tapu Ghose, 2014/11/06 00:23:52]

Finally, the dependency issue has been resolved. I raised this dependency issue
in #chromium IRC channel. The dev who worked on search_engines componentization
submitted a path to fix the dependency issue. Thanks to him and all other
members who threw their advice in the thread.

I then replaced my own function as Peter suggested. But then
TemplateURL(data).SupportsReplacement(search_terms_data) was always returning
false. After investigation I found that it is failing in ParseURL function of 

../components/search_engines/template_url.cc (line 672)

For my test case the value of parsed_url was "http://example.%s.com/". To
overcome this I have replaced the replacement term (%s) by "{searchTerms}"
before calling SupportsReplacement function.

+  search_engine_info->raw_url.assign(base::UTF8ToUTF16(raw_url));
+  search_engine_info->keyword = keyword;
+  search_engine_info->display_name = title;
+
+  const std::string kReplacementTerm("%s");
+  if (raw_url.find(kReplacementTerm) == std::string::npos)
+    return false;
+
+  // Substitute replacement term with arbitrary value. Return false if the
+  // resulted output is an invalid url.
+  const std::string kReplacementVal("val");
+  ReplaceSubstringsAfterOffset(&raw_url, 0, kReplacementTerm, kReplacementVal);
+  return GURL(raw_url).is_valid();
+}
+
 namespace internal {
 
 bool ParseCharsetFromLine(const std::string& line, std::string* charset) {