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

chrome/utility/importer/bookmark_html_reader.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/utility/importer/bookmark_html_reader.h"
 
 #include "base/callback.h"
 #include "base/files/file_util.h"
 #include "base/i18n/icu_string_conversions.h"
 #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"
 #include "url/url_constants.h"
 
 namespace {
 
 // Fetches the given |attribute| value from the |attribute_list|. Returns true
 // if successful, and |value| will contain the value.
 bool GetAttribute(const std::string& attribute_list,
@@ skipping 57 matching lines @@
 
 }  // namespace
 
 namespace bookmark_html_reader {
 
 void ImportBookmarksFile(
       const base::Callback<bool(void)>& cancellation_callback,
       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);
   std::vector<std::string> lines;
   base::SplitString(content, '\n', &lines);
 
   base::string16 last_folder;
   bool last_folder_on_toolbar = false;
   bool last_folder_is_empty = true;
   bool has_subfolder = false;
@@ skipping 39 matching lines @@
     base::string16 post_data;
     bool is_bookmark;
     // TODO(jcampan): http://b/issue?id=1196285 we do not support POST based
     //                keywords yet.
     is_bookmark =
         internal::ParseBookmarkFromLine(line, charset, &title,
                                         &url, &favicon, &shortcut,
                                         &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;
 
     if (is_bookmark &&
         post_data.empty() &&
         (valid_url_callback.is_null() || valid_url_callback.Run(url))) {
       if (toolbar_folder_index > path.size() && !path.empty()) {
         NOTREACHED();  // error in parsing.
         break;
       }
@@ skipping 68 matching lines @@
         // Parent folder include current one, so it's not empty.
         last_folder_is_empty = false;
       }
 
       if (toolbar_folder_index > path.size())
         toolbar_folder_index = 0;
     }
   }
 }
 
+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) {
   const char kCharset[] = "charset=";
   if (StartsWithASCII(line, "<META", false) &&
       (line.find("CONTENT=\"") != std::string::npos ||
           line.find("content=\"") != std::string::npos)) {
     size_t begin = line.find(kCharset);
     if (begin == std::string::npos)
       return false;
@@ skipping 182 matching lines @@
       *url = GURL(value);
     }
   }
 
   return true;
 }
 
 }  // namespace internal
 
 }  // namespace bookmark_html_reader