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

chrome/utility/importer/bookmark_html_reader.h

Index: chrome/utility/importer/bookmark_html_reader.h
diff --git a/chrome/utility/importer/bookmark_html_reader.h b/chrome/utility/importer/bookmark_html_reader.h
index 274c9a272169ee622e9b8f1c64d395e3764ab045..98cb62ec1fe74d7889cc133f0956cbb5b40d51cc 100644
--- a/chrome/utility/importer/bookmark_html_reader.h
+++ b/chrome/utility/importer/bookmark_html_reader.h
@@ -10,6 +10,7 @@
 
 #include "base/callback_forward.h"
 #include "base/strings/string16.h"
+#include "chrome/common/importer/importer_data_types.h"
 
 class GURL;
 struct ImportedBookmarkEntry;
@@ -37,6 +38,9 @@ namespace bookmark_html_reader {
 // |bookmarks| is a pointer to a vector, which is filled with the imported
 // bookmarks. It may not be NULL.
 //
+// |search_engine_keywords| is a pointer to a vector, which is filled with the
+// imported keywords that will be imported as search engines.

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

Nit: "filled with the imported search engines".  (I don't know what a "keyword
that will be imported as a search engine" is supposed to mean.)

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

Rephrased as you suggested.

However, I meant to say "filled with the imported bookmark keywords that will be
imported as search engine". Here, we are trying to import bookmarks as search
engines if the corresponding bookmark contains any keyword.

+//
 // |favicons| is a pointer to a vector, which is filled with the favicons of
 // imported bookmarks. It may be NULL, in which case favicons are not imported.
 void ImportBookmarksFile(
@@ -44,8 +48,19 @@ 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);
 
+// Checks if a bookmark that contains invalid |url| can be imported as search

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

Wait, so |url| is known to be invalid when this function is called?

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

After looking at the flow in chrome/utility/importer/firefox_importer.cc, I
thought that this particular function will only be called if a bookmark contains
invalid |url|. According to the flow, if |url| contains replacement term in host
(e.g., http://example.%s.com) then GURL treats it as invalid. However, it can be
imported as search engine. And from firefox_importer.cc this function is only
called when url.is_valid() returns false.

But when I was addressing one of Ilya's comments for
chrome/utility/importer/bookmark_html_reader.cc, I realized that this function
should also be called for valid url (e.g., http://example.com?id=%s).

In short, |url| can be either valid or invalid when this function is called.

+// engine. Scans |url| to check if there is any replacement term, %s. If yes
+// then substitutes the replacement term(s) with an arbitrary value. Returns
+// true if the resulting url is a valid one.

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

This is describing more what the implementation does than what the caller cares
about from an API perspective.

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

Acknowledged.

+// |search_engine_info| holds search engine data which is composed of |title|,
+// |keyword| and decoded url derived from |url|.

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

This is mostly a description of the structure definition of URLKeywordInfo,
which doesn't belong here.

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

Acknowledged.

+bool CanImportURLAsSearchEngine(const GURL& url,
+                                const base::string16& keyword,
+                                const base::string16& title,
+                                importer::URLKeywordInfo* search_engine_info);

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

nit: Please leave a blank line after this one.

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

Done.

 namespace internal {
 
 // The file format that BookmarkHTMLReader parses starts with a heading