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

chrome/browser/importer/in_process_importer_bridge.cc

 // Copyright (c) 2012 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/browser/importer/in_process_importer_bridge.h"
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 73 matching lines @@
         low_value.find("moz:") != std::string::npos) {
       return false;
     }
     return true;
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FirefoxURLParameterFilter);
 };
 
-// Creates a TemplateURL with the |keyword| and |url|. |title| may be empty.
+// 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.
 // 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(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;

[Peter Kasting, 2014/12/09 23:10:19]

I think this should be "keyword()" instead of "keyword" here, so that if we
generate the keyword, we still have a title.

You should probably ensure one of the tests checks for this (i.e. that entries
with autogenerated keywords have a non-empty title).

However, see later comments about my skepticism that we actually want to allow
keyword autogeneration for imports at all.

[Tapu Ghose, 2014/12/10 04:41:55]

Made use of "data.keyword()" instead of "keyword".

 
Updated logic to reflect your skepticism regarding keyword autogeneration for
bookmark import. Hence, this test check is not necessary.
 
Based on your skepticism I have updated the logic such that we do not import
bookmarks that have empty keyword. In other words, we do not need keyword
autogeneration for bookmark import. However, I have not removed the existing
logic for autogenerating keyword from this function. I was wondering if I should
remove this in order to make the code cleaner. I hope cleaning this won't break
something else.

[Peter Kasting, 2014/12/10 17:41:16]

Well, where else is this called from?  You should remove it if we don't need it,
and if we do need it, you should document why.

[Tapu Ghose, 2014/12/11 03:12:21]

Looks like this is called while importing bookmark from HTML file or browser. In
both cases, before calling this function, we make a check if keyword is
non-empty. This means it is safe enough if the logic for keyword autogeneration
is removed.

-  data.SetURL(
-      TemplateURLRef::DisplayURLToURLRef(base::UTF8ToUTF16(url.spec())));
+  data.SetURL(TemplateURLRef::DisplayURLToURLRef(url));
   return new TemplateURL(data);
 }
 
 // Parses the OpenSearch XML files in |xml_files| and populates |search_engines|
 // with the resulting TemplateURLs.
 void ParseSearchEnginesFromFirefoxXMLData(
     const std::vector<std::string>& xml_data,
     std::vector<TemplateURL*>* search_engines) {
   DCHECK(search_engines);
 
@@ skipping 91 matching lines @@
       ConvertImporterVisitSourceToHistoryVisitSource(visit_source);
   BrowserThread::PostTask(BrowserThread::UI,
                           FROM_HERE,
                           base::Bind(&ProfileWriter::AddHistoryPage,
                                      writer_,
                                      converted_rows,
                                      converted_visit_source));
 }
 
 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) {
-    owned_template_urls.push_back(
-        CreateTemplateURL(url_keywords[i].display_name,
-                          url_keywords[i].keyword,
-                          url_keywords[i].url));
+  TemplateURL* owned_template_url;

[Peter Kasting, 2014/12/09 23:10:19]

Nit: Declare this where it's defined, in the loop below, since we don't need its
lifetime to extend beyond the loop end.

[Tapu Ghose, 2014/12/10 04:41:55]

Agreed.

+  for (const auto& search_engine : search_engines) {
+    owned_template_url = CreateTemplateURL(search_engine.display_name,
+                             search_engine.keyword,
+                             search_engine.url);
+    if (owned_template_url)
+      owned_template_urls.push_back(owned_template_url);
   }
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
       base::Bind(&ProfileWriter::AddKeywords, writer_,
                  base::Passed(&owned_template_urls), unique_on_host_and_path));
 }
 
 void InProcessImporterBridge::SetFirefoxSearchEnginesXMLData(
     const std::vector<std::string>& search_engine_data) {
   std::vector<TemplateURL*> search_engines;
   ParseSearchEnginesFromFirefoxXMLData(search_engine_data, &search_engines);
@@ skipping 55 matching lines @@
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&ExternalProcessImporterHost::NotifyImportEnded, host_));
 }
 
 base::string16 InProcessImporterBridge::GetLocalizedString(int message_id) {
   return l10n_util::GetStringUTF16(message_id);
 }
 
 InProcessImporterBridge::~InProcessImporterBridge() {}