Get the Firefox branding name dynamically

chrome/browser/importer/firefox_importer_utils.cc

Index: chrome/browser/importer/firefox_importer_utils.cc
===================================================================
--- chrome/browser/importer/firefox_importer_utils.cc	(revision 148898)
+++ chrome/browser/importer/firefox_importer_utils.cc	(working copy)
@@ -417,3 +417,37 @@
 
   return content.substr(start, stop - start);
 }
+
+std::string GetFirefoxBrandingName(const FilePath& app_path) {
+  const FilePath app_ini_file = app_path.Append("application.ini");
+  if (file_util::PathExists(app_ini_file)) {

[Ilya Sherman, 2012/08/03 23:12:29]

nit: To reduce nesting, prefer writing code like this with early returns:

if (!file_util::PathExists(app_ini_file))
  return;

// remainder of the code

[cristian.patrasciuc, 2012/08/07 15:46:49]

Now, if the file does not exist we don't just return, we have to obtain the
localized name for Firefox. I chose to leave it this way to keep the default
fallback value in one single place (at the end of the function).

On 2012/08/03 23:12:29, Ilya Sherman wrote:

+    std::string content;
+    file_util::ReadFileToString(app_ini_file, &content);
+    ReplaceSubstringsAfterOffset(&content, 0, "\r\n", "\n");
+    std::vector<std::string> lines;
+    base::SplitString(content, '\n', &lines);
+    bool in_app_section = false;
+    const std::string name_attr("Name=");

[Ilya Sherman, 2012/08/03 23:12:29]

nit: This looks like it should be a file-level constant defined in the anonymous
namespace at the top of the file.

[cristian.patrasciuc, 2012/08/06 15:12:39]

It is only used here. Should I really make it a file-level constant in this
case?

On 2012/08/03 23:12:29, Ilya Sherman wrote:

[Ilya Sherman, 2012/08/06 21:07:37]

It's fine either way.  Typically, we prefer to have file-level constants to
inline strings; but looking at this particular code again, I can see why it
might be more convenient to just have the constant inlined here.

[cristian.patrasciuc, 2012/08/07 15:46:49]

Done.

+    for (unsigned int i = 0; i < lines.size(); ++i) {

[Ilya Sherman, 2012/08/03 23:12:29]

nit: For Chromium code, prefer |size_t| to |unsigned int|.

[cristian.patrasciuc, 2012/08/07 15:46:49]

Done.

+      TrimWhitespace(lines[i], TRIM_ALL, &lines[i]);
+      if (in_app_section) {
+        if (lines[i].find(name_attr) == 0) {
+          return lines[i].substr(name_attr.size());
+        } else if (lines[i][0] == '[') {

[Ilya Sherman, 2012/08/06 21:07:37]

You should check whether lines[i] is empty prior to indexing into it.

[cristian.patrasciuc, 2012/08/07 15:46:49]

Done.

+          break;
+        }

[Ilya Sherman, 2012/08/03 23:12:29]

nit: Since the if- and else-statements have one-line bodies, no need for curly
braces.  Also, we generally prefer to avoid else statements balancing if
statements that return a value; i.e. we prefer to write

if (foo)
  return bar;
else
  fizzbang();

as

if (foo)
  return bar;

fizzbang()

[cristian.patrasciuc, 2012/08/06 15:12:39]

I added the curly braces because otherwise the compiler would give me a warning
(treated as an error by Chromium):
error: suggest explicit braces to avoid ambiguous ‘else’

I can probably remove some of them while still keeping the compiler happy, but I
chose to add all for clarity.

On 2012/08/03 23:12:29, Ilya Sherman wrote:

[Ilya Sherman, 2012/08/06 21:07:37]

The else is not ambiguous if the outer if-stmt has curly braces (which it is
required to by the Chromium style guide).
This is obviously a very minor style issue, but the common style for Chromium
code is to omit curly braces for if- and else-stmts whose conditions and bodies
are exactly one line each, and to include them otherwise.  Since this is the
common style throughout the codebase, adhering to it helps readability;
otherwise, developers familiar with the working style will be distracted by the
curly braces, spending a few thought cycles wondering if they are there because
there's something subtle about the code that requires them.  Again, it's not a
big deal, but this is why I'd encourage you to omit the curly braces.

[cristian.patrasciuc, 2012/08/07 15:46:49]

Done.

+      } else if (lines[i] == "[App]") {
+        in_app_section = true;

[Ilya Sherman, 2012/08/03 23:12:29]

Are there any other sections for which "Name=" can be defined?  If so, you
should probably set in_app_section back to false when any other section is
encountered.

[cristian.patrasciuc, 2012/08/06 15:12:39]

When you are in the [App] section and another section starts, the for-loop exits
(see line 436-437).

On 2012/08/03 23:12:29, Ilya Sherman wrote:

[Ilya Sherman, 2012/08/06 21:07:37]

Ah, you're right.  Apologies for missing that.

+      }
+    }
+    LOG_IF(WARNING, in_app_section) << "There is no line starting with "
+                                    << name_attr << " in the [App] section of "
+                                    << app_ini_file.value();
+    LOG_IF(WARNING, !in_app_section) << app_ini_file.value()
+                                     << " has no [App] section";

[Ilya Sherman, 2012/08/03 23:12:29]

nit: Ditto to the comment below for these logging statements.

[cristian.patrasciuc, 2012/08/07 15:46:49]

Done.

+  } else {
+    LOG(WARNING) << "Could not detect Firefox branding name since "
+                 << app_ini_file.value() << " does not exist.";

[Ilya Sherman, 2012/08/03 23:12:29]

nit: No need to log a warning in this case.  Log statements add to the binary
size, and if all code were to log all possible rare cases, the logs would grow
to be completely unmanageable.  Logging statements like this one are generally
only added temporarily to debug a problem.

[cristian.patrasciuc, 2012/08/07 15:46:49]

Done.

+  }
+  return std::string();
+}