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

chrome/browser/importer/in_process_importer_bridge.cc

Index: chrome/browser/importer/in_process_importer_bridge.cc
diff --git a/chrome/browser/importer/in_process_importer_bridge.cc b/chrome/browser/importer/in_process_importer_bridge.cc
index 3bd1fde5be8c905e1ea3b3014adf69bb691b80f0..6e540cfd809305a519332af9ca6be2119a288bd4 100644
--- a/chrome/browser/importer/in_process_importer_bridge.cc
+++ b/chrome/browser/importer/in_process_importer_bridge.cc
@@ -91,25 +91,32 @@ class FirefoxURLParameterFilter : public TemplateURLParser::ParameterFilter {
   DISALLOW_COPY_AND_ASSIGN(FirefoxURLParameterFilter);
 };
 
-// Creates a TemplateURL with the |keyword| and |url|. |title| may be empty.
+// Creates a TemplateURL with the |keyword| and |url|. |url| must be non-empty.
+// |keyword| may be empty, provided that |url| can be converted to a valid GURL.
+// If |url| contains replacement term(s) in the host then it cannot be converted
+// into a valid GURL. In such a scenario, |keyword| can not be empty. |title|
+// may be empty.

[Peter Kasting, 2014/11/11 22:42:11]

Nit: How about this slight tweak:

Attempts to create a TemplateURL from the provided data.  |title| is optional. 
If |keyword| is not specified, the function will attempt to generate the keyword
from |url|, which in that case must be a valid GURL.  This can fail if e.g. the
replacement term is in the host.  If TemplateURL creation fails, returns NULL.

[Tapu Ghose, 2014/11/15 05:22:19]

Rephrased comment as you suggested.

 // This function transfers ownership of the created TemplateURL to the caller.
 TemplateURL* CreateTemplateURL(const base::string16& title,
                                const base::string16& keyword,
-                               const GURL& url) {
-  // Skip if the url is invalid.
-  if (!url.is_valid())
+                               const base::string16& url) {
+  if (url.empty())
     return NULL;
 
   TemplateURLData data;
-  if (keyword.empty())
-    data.SetKeyword(TemplateURL::GenerateKeyword(url));
-  else
+  if (keyword.empty()) {
+    GURL gurl = GURL(url);
+    if (!gurl.is_valid())
+      return NULL;
+    data.SetKeyword(TemplateURL::GenerateKeyword(gurl));
+  } else {
     data.SetKeyword(keyword);
+  }
+
   // We set short name by using the title if it exists.
   // Otherwise, we use the shortcut.
   data.short_name = title.empty() ? keyword : title;
-  data.SetURL(
-      TemplateURLRef::DisplayURLToURLRef(base::UTF8ToUTF16(url.spec())));
+  data.SetURL(TemplateURLRef::DisplayURLToURLRef(url));
   return new TemplateURL(data);
 }
 
@@ -221,14 +228,14 @@ void InProcessImporterBridge::SetHistoryItems(
 }
 
 void InProcessImporterBridge::SetKeywords(
-    const std::vector<importer::URLKeywordInfo>& url_keywords,
+    const std::vector<importer::SearchEngineInfo>& search_engines,
     bool unique_on_host_and_path) {
   ScopedVector<TemplateURL> owned_template_urls;
-  for (size_t i = 0; i < url_keywords.size(); ++i) {
+  for (size_t i = 0; i < search_engines.size(); ++i) {

[Peter Kasting, 2014/11/11 22:42:11]

Nit: C++11:

  for (const auto& search_engine : search_engines) {
    ... use |search_engine| here ...

[Tapu Ghose, 2014/11/15 05:22:19]

I like this range-based for loop. Since the existing code was using traditional
for-loop syntax, I thought not to play with it.

     owned_template_urls.push_back(
-        CreateTemplateURL(url_keywords[i].display_name,
-                          url_keywords[i].keyword,
-                          url_keywords[i].url));
+        CreateTemplateURL(search_engines[i].display_name,

[Peter Kasting, 2014/11/11 22:42:11]

Shouldn't we check for NULL before blindly pushing onto the vector?  Otherwise,
ProfileWriter::AddKeywords() has to check for embedded NULLs, and from looking
at that, I don't think it does.  (Seems like there should be a unittest for
this?)

[Tapu Ghose, 2014/11/15 05:22:19]

Updated CreateTemplateURL to make use that either keyword or generated keyword
is used for short name, if title is empty. Please let me know if my approach is
wrong.

[Peter Kasting, 2014/11/17 21:30:35]

That's fine, but that's not what I meant by this comment.

What I meant was, CreateTemplateURL() can fail and return NULL, but we don't
check the return value here.  That means we can push NULL onto
|owned_template_urls|.  That will cause ProfileWriter::AddKeywords() to crash
later.  This should be fixed, and a test added that would crash with your patch
without the fix.

[Tapu Ghose, 2014/12/07 02:49:54]

Got it. Checked NULL before pushing onto |owned_template_urls|. Also, I am going
to revert the changes that I have made in CreateTemplateURL for setting short
name either from keyword or generated keyword.

Added a test case (http://%x.example.%s.com without keyword) in
//chrome/browser/importer/firefox_importer_browsertest.cc that would result in
segmentation fault, if NULL was not checked before pushing onto
|owned_template_urls|.

Please let me know if the test case was supposed to be handled in some other
place.

+                          search_engines[i].keyword,
+                          search_engines[i].url));
   }
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
       base::Bind(&ProfileWriter::AddKeywords, writer_,