Import keywords for search engines imported from Firefox

chrome/utility/importer/firefox_importer.cc

Index: chrome/utility/importer/firefox_importer.cc
diff --git a/chrome/utility/importer/firefox_importer.cc b/chrome/utility/importer/firefox_importer.cc
index a682dbdf70d1bd85a81ba90f79ac42de44725c6f..f53f9663b7ed651da7d42773c9f7c29ed386ecf8 100644
--- a/chrome/utility/importer/firefox_importer.cc
+++ b/chrome/utility/importer/firefox_importer.cc
@@ -480,6 +480,22 @@ void FirefoxImporter::GetSearchEnginesXMLData(
 
 void FirefoxImporter::GetSearchEnginesXMLDataFromJSON(
     std::vector<std::string>* search_engine_data) {
+  // search-metadata.json contains keywords for search engines. This
+  // file exists only if user has set keywords for search engines.

[Ilya Sherman, 2014/07/28 22:18:45]

nit: "exists only if user has set" -> "exists only if the user has set" (i.e.
add "the")

[Nikhil, 2014/07/29 08:29:21]

Done.

+  base::FilePath search_metadata_json_file =
+      source_path_.AppendASCII("search-metadata.json");
+  bool search_metadata_available = false;
+  if (base::PathExists(search_metadata_json_file))
+    search_metadata_available = true;

[Ilya Sherman, 2014/07/28 22:18:45]

nit: These three lines can be condensed to

bool search_metadata_available = base::PathExists(search_metadata_json_file);

[Nikhil, 2014/07/29 08:29:21]

I've removed this boolean as check for search_metadata_root is sufficient. I
tested this by removing search-metadata.json as well.

+
+  const base::DictionaryValue* search_metadata_root = NULL;

[Ilya Sherman, 2014/07/28 22:18:45]

nit: Please move this down a few lines, to be just above the if-stmt, so as to
match the formatting of lines 503-507.  (If you really prefer, you could instead
update those lines to match your new formatting).

[Peter Kasting, 2014/07/28 22:48:31]

FWIW I prefer the line 503-507 formatting, as the style guide explicitly says to
declare variables as close to their first use as possible.

[Nikhil, 2014/07/29 08:29:21]

Done.

+  JSONFileValueSerializer metadata_serializer(search_metadata_json_file);

[Ilya Sherman, 2014/07/28 22:18:45]

Why do this if search_metadata_available is false?  Alternately, if it's safe to
do this when that's false, why have the search_metadata_available variable at
all?

[Nikhil, 2014/07/29 08:29:21]

Removed search_metadata_available as it's not required.

+  scoped_ptr<base::Value> metadata_root(
+      metadata_serializer.Deserialize(NULL, NULL));
+  if (metadata_root)
+    metadata_root->GetAsDictionary(&search_metadata_root);
+
+  // search.json contains information about search engines to import.
   base::FilePath search_json_file = source_path_.AppendASCII("search.json");
   if (!base::PathExists(search_json_file))
     return;
@@ -556,6 +572,31 @@ void FirefoxImporter::GetSearchEnginesXMLDataFromJSON(
 
         std::string file_data;
         base::ReadFileToString(xml_file, &file_data);
+
+        // If keyword is mentioned for this search engine, then add

[Peter Kasting, 2014/07/28 22:48:31]

Nit: keyword -> a keyword

[Nikhil, 2014/07/29 08:29:21]

Done.

+        // it in xml string as <Alias> element and use this updated

[Ilya Sherman, 2014/07/28 22:18:45]

nit: "add it in xml string as <Alias> element" -> "add it to the XML string as
an <Alias> element"

[Nikhil, 2014/07/29 08:29:21]

Done.

+        // string.
+        if (search_metadata_available && search_metadata_root &&
+            search_metadata_root->HasKey(file_path)) {
+          // Keyword is stored in field named as "alias".

[Peter Kasting, 2014/07/28 22:48:31]

Nit: This comment is redundant.

[Nikhil, 2014/07/29 08:29:21]

Done.

+          const base::DictionaryValue* search_xml_path = NULL;
+          if (search_metadata_root->GetDictionaryWithoutPathExpansion(
+                  file_path, &search_xml_path)) {

[Peter Kasting, 2014/07/28 22:48:31]

Nit: Indent 4, not 8

[Nikhil, 2014/07/29 08:29:21]

This was actually done by clang-format. I've now manually changed it to 4.

[Ilya Sherman, 2014/07/29 17:24:55]

FWIW, I agree with clang-format, and disagree with Peter.  IMO it's not worth
locally overriding the clang-format output here, as the codebase is moving
toward general clang-format conformance.  However, if Peter chimes back in
disagreeing with me, feel free to ignore my comment and go with his request --
it's not worth losing tons of time on this :)

[Peter Kasting, 2014/07/29 18:33:31]

I'm not convinced the codebase is, in fact, moving that way :)
This particular one has been debated in a few places and there's never really
been consensus.  The style I'm advocating is classically what Chrome did
everywhere.  The style here is not terribly unreasonable, so I don't care too
much either way, but in general I think we should stay with tradition until we
make an explicit decision to change it, and clang-format is not (yet) a reason
to violate that.

Do what you want.

+            std::string alias;
+            search_xml_path->GetString("alias", &alias);
+
+            // Add it as last child element.

[Peter Kasting, 2014/07/28 22:48:31]

Nit: it -> the alias element; last -> the last

[Nikhil, 2014/07/29 08:29:21]

Done.

+            size_t end_of_parent = file_data.find("</SearchPlugin>");
+            if (end_of_parent != std::string::npos) {
+              std::string& updated_file_data = file_data.insert(
+                  end_of_parent, "<Alias>" + alias + "</Alias> \n");
+              search_engine_data->push_back(updated_file_data);
+              continue;

[Ilya Sherman, 2014/07/28 22:18:45]

IMO it would be better to simply update |file_data| here, and drop the other two
lines, since they'll be handled by the natural flow of the loop.  This also
means that if the loop is ever updated, it's less likely that this code path
would fail to be updated.

[Nikhil, 2014/07/29 08:29:21]

Done.

+            }
+          }
+        }
+
+        // No keyword is mentioned for this search-engine.
         search_engine_data->push_back(file_data);
       }
     }