Fixing Firefox 21+ password import

chrome/common/importer/firefox_importer_utils.cc

Index: chrome/common/importer/firefox_importer_utils.cc
diff --git a/chrome/common/importer/firefox_importer_utils.cc b/chrome/common/importer/firefox_importer_utils.cc
index 4f6a2019e781551b516cdde924b5aca31f755e1e..98158199f82d2dc5624c9419a2f88e988d0e4c2c 100644
--- a/chrome/common/importer/firefox_importer_utils.cc
+++ b/chrome/common/importer/firefox_importer_utils.cc
@@ -106,7 +106,7 @@ bool GetFirefoxVersionAndPathFromProfile(const base::FilePath& profile_path,
   base::SplitString(content, '\n', &lines);
 
   for (size_t i = 0; i < lines.size(); ++i) {
-    const std::string& line = lines[i];
+    std::string& line = lines[i];

[jeremy, 2014/01/07 12:37:27]

maintain const and work on a copy of the string below in the #ifdef

     if (line.empty() || line[0] == '#' || line[0] == ';')
       continue;
     size_t equal = line.find('=');
@@ -115,11 +115,29 @@ bool GetFirefoxVersionAndPathFromProfile(const base::FilePath& profile_path,
       if (key == "LastVersion") {
         base::StringToInt(line.substr(equal + 1), version);
         ret = true;
-      } else if (key == "LastAppDir") {
+      } else if (key == "LastPlatformDir") {

[jeremy, 2014/01/07 12:37:27]

Why are we changing this? This code is cross-platform, are we sure changing the
key doesn't break other platforms?

LastAppDir contains the .app path, right?

         // TODO(evanm): If the path in question isn't convertible to
         // UTF-8, what does Firefox do?  If it puts raw bytes in the
         // file, we could go straight from bytes -> filepath;
         // otherwise, we're out of luck here.
+#if defined (OS_MACOSX)

[jeremy, 2014/01/07 12:37:27]

s/defined (/defined(/

+        // Find the "*.app" component of the path and build up from there.
+        // The resulting path will be .../Firefox.app/Contents/MacOS.
+        // We do this because we don't trust LastPlatformDir to always be
+        // this particular path, without any subdirs, and we want to make
+        // our assumption about Firefox's root being in that path explicit.
+        const std::string dot_app(".app");
+        const size_t pos = line.find(dot_app);

[jeremy, 2014/01/07 12:37:27]

Recommend manipulating as a FilePath:
 * Split using GetComponents().
 *  then build up a new FilePath using Append() to iteratively add path
components.

+        if (pos == std::string::npos) {
+          LOG(ERROR) << line << " doesn't look like a valid Firefox "
+                     << "installation path: missing /*.app/ directory.";
+          return false;
+        }
+        // Replace everything after .app with /Contents/MacOS.
+        line.replace(pos + dot_app.length(),
+                     std::string::npos,
+                     "/Contents/MacOS");
+#endif

[jeremy, 2014/01/07 12:37:27]

#endif  // OS_MACOSX

         *app_path = base::FilePath::FromUTF8Unsafe(line.substr(equal + 1));
       }
     }