Get the Firefox branding name dynamically

chrome/browser/importer/importer_list.cc

Index: chrome/browser/importer/importer_list.cc
===================================================================
--- chrome/browser/importer/importer_list.cc	(revision 148898)
+++ chrome/browser/importer/importer_list.cc	(working copy)
@@ -75,12 +75,21 @@
   } else if (version >= 3) {
     firefox_type = importer::TYPE_FIREFOX3;
   } else {
-    // Ignores other versions of firefox.
+    LOG(WARNING) << "Importing from Firefox version " << version
+                 << " is not supported.";

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

Please revert this change.  Chrome generally only logs warnings and errors in
cases of exceptional code paths.  Anything that is intended to be user-visible
should be shown in the browser's UI.

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

Done.

     return;
   }
 
   importer::SourceProfile* firefox = new importer::SourceProfile;
-  firefox->importer_name = l10n_util::GetStringUTF16(IDS_IMPORT_FROM_FIREFOX);
+  const std::string branding_name = GetFirefoxBrandingName(app_path);

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

I would suggestion moving the below if-else logic into GetFirefoxBrandingName,
and renaming that function to GetFirefoxImporterName.  The code in this file
should need to know as little as possible about each of the different import
sources.

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

This will make unit tests depend on l10n/generated resources. Also, it would be
harder to test if the Firefox importer name came up because that's what the
application.ini file contains or just because something unexpected happened and
the Firefox name is used as a fallback.

So, should I still move this to GetFirefoxImporterName in this case?

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

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

That's ok.
You'll still be able to test with both test inputs, so you're not actually
losing any test coverage.  In fact, you're gaining coverage, as this code in
importer_list is currently untested.
I think so, yes :)

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

Done.

+  if (branding_name.find("Iceweasel") != std::string::npos ||
+      branding_name.find("iceweasel") != std::string::npos) {
+    firefox->importer_name =
+        l10n_util::GetStringUTF16(IDS_IMPORT_FROM_ICEWEASEL);
+  } else {
+    firefox->importer_name =
+        l10n_util::GetStringUTF16(IDS_IMPORT_FROM_FIREFOX);
+  }
   firefox->importer_type = firefox_type;
   firefox->source_path = profile_path;
 #if defined(OS_WIN)