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 "net/base/data_url.h"
 #include "net/base/escape.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
+#include "url/url_util.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,
                   const std::string& attribute,
                   std::string* value) {
   const char kQuote[] = "\"";
 
@@ skipping 53 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::URLKeywordInfo>* url_keywords,
       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 42 matching lines @@
     //                keywords yet.
     is_bookmark =
         internal::ParseBookmarkFromLine(line, charset, &title,
                                         &url, &favicon, &shortcut,
                                         &add_date, &post_data) ||
         internal::ParseMinimumBookmarkFromLine(line, charset, &title, &url);
 
     if (is_bookmark)
       last_folder_is_empty = false;
 
+    base::string16 decoded_url;
+
     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)
+         || CanImportURLAsSearchEngine(url, &decoded_url))) {

[Ilya Sherman, 2014/09/30 20:54:12]

nit: The "||" should be on the previous line.  ("git cl format" can help you
find and fix formatting errors)

[Tapu Ghose, 2014/10/07 02:02:44]

Done.

       if (toolbar_folder_index > path.size() && !path.empty()) {
         NOTREACHED();  // error in parsing.
         break;
       }
 
+      // If bookmark contains a keyword then import it as search engine.
+      // If |url| is invalid, set raw_url property of |url_keyword_info|
+      // which will be used in importing as search engine.
+      if (!shortcut.empty()) {

[Peter Kasting, 2014/10/01 01:02:55]

Doesn't this mean we'll also import non-replaceable bookmarked URLs with
keywords as search engines?  That doesn't seem right.

[Tapu Ghose, 2014/10/07 02:02:44]

Agree with you. Updated logic to address your comment.

+        importer::URLKeywordInfo url_keyword_info;
+        if (url.is_valid())
+          url_keyword_info.url = url;
+        else
+          url_keyword_info.raw_url = decoded_url;
+        url_keyword_info.keyword.assign(shortcut);
+        url_keyword_info.display_name = title;
+        url_keywords->push_back(url_keyword_info);
+        continue;
+      }
+
       ImportedBookmarkEntry entry;
       entry.creation_time = add_date;
       entry.url = url;
       entry.title = title;
 
       if (toolbar_folder_index) {
         // The toolbar folder should be at the top level.
         entry.in_toolbar = true;
         entry.path.assign(path.begin() + toolbar_folder_index - 1, path.end());
       } else {
@@ skipping 57 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, base::string16* decoded_url) {
+  // If |url| is valid then we are not supposed to be here.

[Peter Kasting, 2014/10/01 01:02:55]

That implies callers shouldn't be calling us.  I don't think that's actually
what you mean.

[Tapu Ghose, 2014/10/07 02:02:44]

Updated comments.

+  if (url.is_valid())
+    return false;
+
+  std::string raw_url = url.possibly_invalid_spec();
+
+  // Decode raw_url.
+  url::RawCanonOutputW<1024> canon_output;

[Ilya Sherman, 2014/09/30 20:54:12]

Hmm, how do you know that 1024 characters is enough?  Can you use an
automatically resizing class instead?

[Peter Kasting, 2014/10/01 01:02:55]

It's not necessarily enough.  This is almost certainly an exploitable security
hole.

[Tapu Ghose, 2014/10/07 02:02:44]

Acknowledged.

[Tapu Ghose, 2014/10/07 02:02:44]

Acknowledged.

+  url::DecodeURLEscapeSequences(raw_url.c_str(), raw_url.size(), &canon_output);

[Peter Kasting, 2014/10/01 01:02:55]

Why can't we use the higher-level unescaping functions?  Using the raw
canonicalizer functions is both cumbersome and dangerous.

[Tapu Ghose, 2014/10/07 02:02:44]

My bad. I could not find higher-level unescaping function earlier. Replacing it
with UnescapeURLComponent from net/base/escape.h.

+  decoded_url->assign(base::string16(canon_output.data(),
+                                     canon_output.length()));
+
+  raw_url = base::UTF16ToUTF8(*decoded_url);
+  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.
+  size_t n = 0;
+  const std::string kReplacementValue("val");
+  while ((n = raw_url.find(kReplacementTerm, n)) != std::string::npos) {
+    raw_url.replace(n, kReplacementTerm.size(), kReplacementValue);
+    n += kReplacementValue.size();
+  }

[Ilya Sherman, 2014/09/30 20:54:12]

Please use a utility function from base/strings/string_util.h rather than
writing your own version here.

[Peter Kasting, 2014/10/01 01:02:55]

Furthermore, there's already code in Chrome that attempts to determine if some
string is a valid, replaceable URL for TemplateURL purposes.  This whole
function should be using that code, not reimplementing it.

[Tapu Ghose, 2014/10/07 02:02:44]

Before writing my custom code I tried to use ReplaceSearchTerms function from
components/search_engines/template_url.h. But it was giving me following fatal
error:
'components/metrics/proto/omnibox_event.pb.h' file not found.
Later on I found the file under out/Debug/gen/protoc_out/ directory (which was
generated after compiling). Hence, thought not to use this approach.

[Tapu Ghose, 2014/10/07 02:02:44]

Replaced my own version with ReplaceSubstringsAfterOffset function from
base/strings/string_util.h.

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

That sounds like a dependency issue, caused by the fact that you're writing code
within //chrome/utility.  I'm guessing that it would be fine to fix the
dependencies, by adding //components/search_engines to the list of dependencies
in [
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_util...
].  It's definitely better to reuse the existing code rather than reimplementing
it.

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

Tried after adding '../components/search_engines.gypi:search_engines'. But no
luck.

[Ilya Sherman, 2014/10/14 00:41:49]

Hmm, based on local testing, it looks like you need to add these two lines:

        '../components/components.gyp:component_metrics_proto',
        '../components/components.gyp:search_engines',

The first one shouldn't be necessary; instead, //components/metrics.gypi or
//components/search_engines.gypi should somehow specify this dependency. 
Unfortunately, I'm not sufficiently well versed in the build system to know how
exactly to correctly express this.  I'd recommend asking around on the #chromium
IRC channel.  Note that you should actually update both the .gyp* files and the
BUILD.gn files, and verify that compilation works using both systems.

[Tapu Ghose, 2014/10/19 02:46:33]

After modifying .gyp* and BUILD.gn do I need to run any specific command other
than "ninja -C out/Debug chrome"?

+  return GURL(raw_url).is_valid();
+}
+
 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