Last Comment Bug 717070 - Create button to initiate migration to a clean Firefox profile
: Create button to initiate migration to a clean Firefox profile
Status: VERIFIED FIXED
[qa!]
: user-doc-needed
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: Trunk
: All All
: -- enhancement with 3 votes (vote)
: Firefox 13
Assigned To: Matthew N. [:MattN] (PM me if requests are blocking you)
:
: Matthew N. [:MattN] (PM me if requests are blocking you)
Mentors:
https://wiki.mozilla.org/Support/Fire...
Depends on: 735568 756390 273874 731047 734735 735126 749700 749931
Blocks: reset-firefox 732303
  Show dependency treegraph
 
Reported: 2012-01-10 15:21 PST by Matthew N. [:MattN] (PM me if requests are blocking you)
Modified: 2013-12-27 14:24 PST (History)
22 users (show)
ioana_damy: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v.1 migration/backend portion (17.92 KB, patch)
2012-01-24 18:19 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
asaf: review-
Details | Diff | Splinter Review
Part 1. v.2 Backend portion - Create Profile and pass key to migrate (15.34 KB, patch)
2012-02-23 17:37 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
asaf: review-
benjamin: superreview+
Details | Diff | Splinter Review
Part 2. v.1 Rest of the migration portion. Skipping import source page + progressbar (5.15 KB, patch)
2012-02-23 17:44 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
asaf: review+
Details | Diff | Splinter Review
Part 3. v.1 about:support box (9.38 KB, patch)
2012-02-23 17:50 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
no flags Details | Diff | Splinter Review
about:support reset UI screenshot (113.29 KB, image/png)
2012-02-23 18:05 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
no flags Details
about:support reset UI screenshot v2 with confirmation (112.46 KB, image/png)
2012-02-27 15:44 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
limi: ui‑review+
Details
Part 1. v.3 Backend portion - Create Profile and pass key to migrate (21.37 KB, patch)
2012-03-07 02:01 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
no flags Details | Diff | Splinter Review
Part 1. v.3.1 Backend portion - Create Profile and pass key to migrate (21.48 KB, patch)
2012-03-08 02:21 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
asaf: review+
Details | Diff | Splinter Review
Part 3. v.2 about:support box + dialog (13.82 KB, patch)
2012-03-08 02:49 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
mak77: review+
Details | Diff | Splinter Review

Description Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-10 15:21:26 PST
Add a button to about:support (only when in safe mode?) to create a new profile and initiate migration of data (bug 273874) from the old profile.
Comment 1 Verdi [:verdi] 2012-01-10 15:28:54 PST
(In reply to Matthew N. [:MattN] from comment #0)
> Add a button to about:support (only when in safe mode?) to create a new
> profile and initiate migration of data (bug 273874) from the old profile.

I think it should be in about:support whether or not you are in safe mode. If it's only available in safe mode it's much harder to get to.
Comment 2 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-10 17:17:20 PST
(In reply to Verdi from comment #1)
> If it's only available in safe mode it's much harder to get to.

That was kind of my reasoning for putting it in safe mode since user's should be trying safe mode first before using this new profile measure. I wouldn't want people using this feature just because they have one extension that is causing a problem, especially since getting back to their old profile is non-trivial.
Comment 3 Verdi [:verdi] 2012-01-11 07:19:15 PST
The major reason for creating this feature is two-fold: 1. Current recovery procedures are difficult to discover and difficult to execute; 2. Most people don't want to diagnose and fix problems (when faced with this, the easy solution is to switch browsers). So with this we want to make it as easy as possible to find and execute.
Comment 4 Asa Dotzler [:asa] 2012-01-13 16:35:13 PST
MattN, Safari offers it from the main menu. This should not depend on safe mode. about:support is plenty buried.
Comment 5 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-24 18:19:14 PST
Created attachment 591343 [details] [diff] [review]
v.1 migration/backend portion

This conflicts with bug 718608 but I don't know which will land first and I'm aiming to get this in 12.
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-25 04:42:33 PST
Comment on attachment 591343 [details] [diff] [review]
v.1 migration/backend portion

There are few things here I'm not so happy about:
1. Why is the "profile" environment variable needed? It would probably be better to support recovering just from the default profile (If you use multiple profiles, you most likely do not need recovery tools like this). As we discussed some time ago, unless you want the Firefox migrator to actually support multiple profiles, it should not pretend to do so.
If you're worried about the case in which someone uses the new feature, when a profile that's not the selected profile is used, we could add code to hide the button in this case.
  2. The MIGRATION_SOURCE stuff is somewhat unfortunate.  I don't know how easy it would be to add support for passing arguments for restart. But it seems much more cleaner, especially since we already have a "migration" argument in place.
  2.1. If you keep it, please prefix it with MOZ_FX_
  3. Why do you need to doStartup only after key3.db is copied? I was going to remove this feature in bug 718586. It would be better to just reinitialize the login manager somehow, if you're worried about this.

Overall, all of this complexity leads me to think it was somewhat a mistake to use the current migration system for this migrator. I know that there were some Mozilla->Mozilla migrators (and I guess Thunderbird still has a SM migrator), but this case is different. All we do here is to copy some files from one profile to another. This could be done in a much-much cleaner and simpler way somewhere in toolkit.
Comment 7 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-25 13:20:04 PST
(In reply to Mano from comment #6)
> Comment on attachment 591343 [details] [diff] [review]
> v.1 migration/backend portion
> 
> There are few things here I'm not so happy about:
> 1. Why is the "profile" environment variable needed? It would probably be
> better to support recovering just from the default profile (If you use
> multiple profiles, you most likely do not need recovery tools like this). As
> we discussed some time ago, unless you want the Firefox migrator to actually
> support multiple profiles, it should not pretend to do so.
> If you're worried about the case in which someone uses the new feature, when
> a profile that's not the selected profile is used, we could add code to hide
> the button in this case.

There is no profile environment variable so do you mean the argument to the migrator wizard?

I think it's bad for user experience if buttons magically show up or not.  Advanced users (ie. web developers who use two profiles) may want to use this functionality too since they may not know which files to copy to a new profile and a goal is that someone shouldn't have to find SUMO to fix their problems.

>   2. The MIGRATION_SOURCE stuff is somewhat unfortunate.  I don't know how
> easy it would be to add support for passing arguments for restart. But it
> seems much more cleaner, especially since we already have a "migration"
> argument in place.

I considered having the migration source data passed as the value of the -migration arg. but found environment variables were easier to use and I also noticed after attaching the patch that I should be using SaveFileToEnv/GetFileFromEnv in order for it to work on Windows. 

>   2.1. If you keep it, please prefix it with MOZ_FX_
>   3. Why do you need to doStartup only after key3.db is copied? I was going
> to remove this feature in bug 718586. It would be better to just
> reinitialize the login manager somehow, if you're worried about this.

Yes, I commented there and you have since replied.

> Overall, all of this complexity leads me to think it was somewhat a mistake
> to use the current migration system for this migrator. I know that there
> were some Mozilla->Mozilla migrators (and I guess Thunderbird still has a SM
> migrator), but this case is different. All we do here is to copy some files
> from one profile to another. This could be done in a much-much cleaner and
> simpler way somewhere in toolkit.

I think that in it's current form it may have been better to implement somewhere else but that's because, like you say, it's just copying files.  This leads to it only working before startup.  The plan[1] is to make the migrator more conservative and not just copy files though and then it would work while the browser is running.  We get the error handling and UI automatically as a migrator which made this faster to implement. It performs the same functions as a migrator as well.

[1] https://wiki.mozilla.org/Support/Firefox_Features/Clean_up_user_profile#9._Implementation
Comment 8 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-30 13:38:00 PST
Benjamin, could you guide me on how to implement this feature such that it aligns with the goal of replacing the toolkit profile service.  I also want to ensure that the implementation doesn't hinder other toolkit apps.

This is my plan: 
1) Add a command-line flag (ie. -createprofile) or environment variable which will create a new profile with a default name and use that profile for startup.
   * are you fine with me calling nsToolkitProfileService::CreateProfile? 
2) Callers who also want migration would include the -migration argument which sets gDoMigration = true.
3) Firefox migrator (bug 273874) runs and uses the default/selected profile as its source and the new profile as it's destination.
4) Make this new "reset" profile the default/selected profile so that the user uses it by default.
   * Are you fine with me calling nsToolkitProfileService::SetSelectedProfile?
   * If this is not the desired default behaviour for the new flag then this can be
     done from the migrator/browser code.
Comment 9 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-02-06 13:01:31 PST
bsmedberg: ping.  If you prefer to have a meeting to discuss this sometime this week then that works for me. Just let me know.
Comment 10 Benjamin Smedberg [:bsmedberg] 2012-02-17 12:38:04 PST
re: comment 8 in general that sounds fine. I'd like the flag to be called --reset-profile (with the optional --migration).
Comment 11 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-02-23 17:37:23 PST
Created attachment 600263 [details] [diff] [review]
Part 1. v.2  Backend portion - Create Profile and pass key to migrate

I'm not sure if the NS_WARNINGs are necessary when --reset-profile is not supported.
Comment 12 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-02-23 17:44:59 PST
Created attachment 600264 [details] [diff] [review]
Part 2. v.1 Rest of the migration portion. Skipping import source page + progressbar

I removed the complexity of specifying a source profile path.  Only the import source page is skipped now.
Comment 13 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-02-23 17:50:46 PST
Created attachment 600266 [details] [diff] [review]
Part 3. v.1 about:support box
Comment 14 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-02-23 18:05:12 PST
Created attachment 600270 [details]
about:support reset UI screenshot

I'm looking for feedback on the profile reset UI that will show up on about:support:
* Visual style
* Copy - Should convey that dataloss will occur as we'll only attempt to migrated the specific data listed. I prefer a whitelist of what we'll migrate over 
a blacklist since that makes it ambiguous for the data not listed.  This also reflects the actual implementation with a whitelist.

======
Reset Firefox to its default state

Warning: Everything will be reset to defaults except: bookmarks, browsing history, cookies, form & search history, and passwords.

      [ Reset Firefox ]
Comment 15 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-02-27 15:44:11 PST
Created attachment 601100 [details]
about:support reset UI screenshot v2 with confirmation

I added a confirmation dialog based on feedback that it was too easy to reset by mis-clicking.

(In reply to Matthew N. [:MattN] from comment #14)
> about:support reset UI screenshot
> 
> I'm looking for feedback on the profile reset UI that will show up on
> about:support:
> * Visual style
> * Copy - Should convey that dataloss will occur as we'll only attempt to
> migrated the specific data listed. I prefer a whitelist of what we'll
> migrate over 
> a blacklist since that makes it ambiguous for the data not listed.  This
> also reflects the actual implementation with a whitelist.

=== about:support ===
Reset Firefox to its default state

If you're having major problems which you can't resolve, start fresh with only your essential information.

      [ Reset Firefox ]

=== confirmation dialog ===
Are you sure you want to reset Firefox to its default state?

Warning: Everything will be reset to defaults except: bookmarks, browsing
history, cookies, form & search history, and passwords.
Comment 16 Alex Limi (:limi) — Firefox UX Team 2012-03-05 14:08:06 PST
Comment on attachment 601100 [details]
about:support reset UI screenshot v2 with confirmation

This looks good to me. If it's possible with this type of dialog, I'd call out the items as a itemized list to help quick skimming. Something like this:

___
Are you sure you want to reset Firefox to its initial state?

The following will be kept:
* Bookmarks
* Browsing history
* Cookies
* Form & search history
* Passwords

Everything else will be reset.
___
Comment 17 Marco Bonardo [::mak] 2012-03-05 14:37:21 PST
Comment on attachment 600263 [details] [diff] [review]
Part 1. v.2  Backend portion - Create Profile and pass key to migrate

Review of attachment 600263 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know nsAppRunner code well enough to make a proper review, though I can give some drive-by comments.

The ProfileMigrator changes in some point conflict with bug 718608, so I'd prefer if Mano looks at those since he is working on this code in these days, so has the code pretty fresh in mind. HE may also review the nsAppRunner part, I think, otherwise Mossop could.

::: toolkit/xre/nsAppRunner.cpp
@@ +1923,5 @@
> +  nsCOMPtr<nsIToolkitProfileService> profileSvc;
> +  nsresult rv = NS_NewToolkitProfileService(getter_AddRefs(profileSvc));
> +  if (rv == NS_ERROR_FILE_ACCESS_DENIED)
> +    PR_fprintf(PR_STDERR, "Error: Access was denied while trying to open files in " \
> +                "your profile directory.\n");

brace this if please, not exactly one line

@@ +1927,5 @@
> +                "your profile directory.\n");
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIToolkitProfile> newProfile;
> +  // Make the new profile "default-" + the time in seconds since epoch for uniqueness.

Not exactly unique considered the system time can be adjusted to the past and has a resolution of 16ms on Windows.  As an alternative you may use a counter and getProfileByName till you find a free slot, should be fast since profiles seem to be cached in a linked list.

@@ +1928,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIToolkitProfile> newProfile;
> +  // Make the new profile "default-" + the time in seconds since epoch for uniqueness.
> +  nsCString newProfileName = NS_LITERAL_CSTRING("default-");

an autostring would be large enough for this

@@ +1940,5 @@
> +  rv = profileSvc->Flush();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  *aNewProfile = newProfile.get();
> +  NS_IF_ADDREF(*aNewProfile);

I think you can just do
NS_IF_ADDREF(*aNewProfile = newProfile)

@@ +1957,5 @@
> +  nsCOMPtr<nsIToolkitProfileService> profileSvc;
> +  nsresult rv = NS_NewToolkitProfileService(getter_AddRefs(profileSvc));
> +  if (rv == NS_ERROR_FILE_ACCESS_DENIED)
> +    PR_fprintf(PR_STDERR, "Error: Access was denied while trying to open files in " \
> +               "your profile directory.\n");

brace

@@ +1973,5 @@
> +    nsCOMPtr<nsILocalFile> profileRoot;
> +    profile->GetRootDir(getter_AddRefs(profileRoot));
> +    bool areEqual = false;
> +    profileRoot->Equals(aCurrentProfileRoot, &areEqual);
> +    if (areEqual) {

nit: this areEqual var may get a more meaningful name about what it's testing... foundMatchingProfile maybe? Actually you may even define this outside the loop and make a while (NS_SUCCEEDED(rv) && !foundMatchingProfile)

@@ +1975,5 @@
> +    bool areEqual = false;
> +    profileRoot->Equals(aCurrentProfileRoot, &areEqual);
> +    if (areEqual) {
> +      rv = profileSvc->SetSelectedProfile(profile);
> +      profileSvc->Flush();

is it fine to hide an eventual error from Flush? wouldn't that not persist the selection?
You may want to if (NS_SUCCEEDED(rv) rv = profileSvc->Flush();

@@ +1980,5 @@
> +      break;
> +    }
> +    rv = profiles->GetNext(getter_AddRefs(profile));
> +  }
> +  return rv;

worth adding a comment that an error is returned if either we fail selecting a profile or we don't find the requested profile, since at first glance this pass-through rv looks bogus, even if it's not.

@@ +3559,5 @@
>            gDoMigration = false;
>            nsCOMPtr<nsIProfileMigrator> pm
>              (do_CreateInstance(NS_PROFILEMIGRATOR_CONTRACTID));
> +          if (pm) {
> +            nsCString aKey;

An autostring should be fine here too
Comment 18 Marco Bonardo [::mak] 2012-03-05 15:11:08 PST
Comment on attachment 600266 [details] [diff] [review]
Part 3. v.1 about:support box

Review of attachment 600266 [details] [diff] [review]:
-----------------------------------------------------------------

this still lacks the confirmation dialog code, if I'm not wrong.

::: browser/locales/en-US/chrome/overrides/resetProfile.properties
@@ +1,1 @@
> +aboutSupport.resetDescription = <span class='warning'>Warning:</span> Everything will be reset to defaults except: bookmarks, browsing history, cookies, form &amp; search history, and passwords.

We should try to avoid tags in localized strings, as far as possible. So either we remove this fancy coloring, or split and make "Warning:" a resetDescriptionPrefix string, with a nice LOCALIZATION NOTE about how the 2 combine and how it is supposed to be rendered

::: toolkit/content/aboutSupport.js
@@ +567,5 @@
> + * Profile reset is only supported for the default profile if the appropriate migrator exists.
> + */
> +function populateResetBox() {
> +  const ToolkitProfileService = "@mozilla.org/toolkit/profile-service;1";
> +  let profileService = Cc[ToolkitProfileService].getService(Ci.nsIToolkitProfileService);

nit: the const is used just once and doesn't go over 80 chars (getService can be indented below), so could just be hardcoded.

@@ +574,5 @@
> +  if (!currentProfileDir.equals(profileService.selectedProfile.rootDir))
> +    return;
> +#expand const MOZ_APP_NAME = "__MOZ_APP_NAME__";
> +#expand const MOZ_BUILD_APP = "__MOZ_BUILD_APP__";
> +  if (! "@mozilla.org/profile/migrator;1?app=" + MOZ_BUILD_APP + "&type=" + MOZ_APP_NAME in Cc)

rather than using a space, add parenthesis around the in operator

nit: the 2 ifs may be merged

@@ +580,5 @@
> +
> +  let resetBundle = Services.strings.createBundle("chrome://global/locale/resetProfile.properties");
> +  let resetDescription = resetBundle.GetStringFromName("aboutSupport.resetDescription");
> +  document.getElementById("reset-description").innerHTML = resetDescription;
> +  document.getElementById("reset-box").style.visibility = "";

"visible"

@@ +586,5 @@
> +
> +/**
> + * Restart the application to reset the profile.
> + */
> +function resetProfile() {

should be called resetProfileAndRestart to be clear about what it does

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd
@@ +52,5 @@
>  
>  <!ENTITY aboutSupport.copyToClipboard.label "Copy all to clipboard">
> +
> +<!ENTITY aboutSupport.resetTitle "Reset &brandShortName; to its default state">
> +<!ENTITY aboutSupport.reset.label "Reset &brandShortName;">

I think this should clarify that it's going to restart the browser too, it's completely hidden and the user may have reasons to delay a restart

::: toolkit/locales/en-US/chrome/global/resetProfile.properties
@@ +1,1 @@
> +aboutSupport.resetDescription = <span class='warning'>Warning:</span> Everything will be reset to defaults.

the same as for the browser version

::: toolkit/themes/winstripe/global/aboutSupport.css
@@ +120,5 @@
> +  background-color: -moz-Dialog;
> +  border: 1px solid ThreeDShadow;
> +  color: -moz-DialogText;
> +  float: right;
> +  margin: 2em 0 20px 20px;

this doesn't seem to handle RTL correctly, did you test that?

@@ +125,5 @@
> +  padding: 16px;
> +  width: 30%;
> +}
> +
> +#reset-box h3 {

child selector

@@ +129,5 @@
> +#reset-box h3 {
> +  margin-top: 0;
> +}
> +
> +#reset-box button {

child selector

@@ +132,5 @@
> +
> +#reset-box button {
> +  display: block;
> +  margin-left: auto;
> +  margin-right: auto;

would just margin: auto work?
Comment 19 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-06 01:54:41 PST
Comment on attachment 600263 [details] [diff] [review]
Part 1. v.2  Backend portion - Create Profile and pass key to migrate

As discussed over #places, this results in multiple instances of the toolkit profile service. Everything else looks fine, but I'll finish the review once the service-issues is addressed.
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-06 02:03:54 PST
Comment on attachment 600264 [details] [diff] [review]
Part 2. v.1 Rest of the migration portion. Skipping import source page + progressbar

>   <wizardpage id="migrating" pageid="migrating" label="&migrating.title;"
>               next="done"
>               onpageshow="MigrationWizard.onMigratingPageShow();">
>     <description control="migratingItems">&migrating.label;</description>
> 
>     <vbox id="migratingItems" style="overflow: auto;" align="left" role="group"/>
>+    <progressmeter mode="undetermined"/>
>   </wizardpage>

I think this should be moved to another bug, but I'll leave the final call for you (or ux, whatever works!).  Personally, I don't think an undetermined progresbar makes the current wizard suck less.

>   migrate : function Firefox_migrate(aItems, aStartup, aProfile)
>   {
>     if (aStartup) {
>       this._replaceBookmarks = true;
>     }
> 
>+    // Ensure that aProfile is not the current profile.
>+    if (this._paths.currentProfile.path === this._sourceProfile.path) {
>+      Cu.reportError("Source and destination profiles are the same");
>+      Services.obs.notifyObservers(null, "Migration:Ended", null);
>+      return;
>+    }
>+

This shouldn't happen actually (migrate shouldn't be called if getMigrateData said there's nothing to migrate), and it shouldn't notify for "Migration:Ended" if it didn'tm notify for "Migration:Started".  You can throw in this case though.


Looks OK otherwise. r=mano.
Comment 21 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-03-07 02:01:11 PST
Created attachment 603645 [details] [diff] [review]
Part 1. v.3  Backend portion - Create Profile and pass key to migrate

(In reply to Marco Bonardo [:mak] from comment #17)
> Comment on attachment 600263 [details] [diff] [review]
> Part 1. v.2  Backend portion - Create Profile and pass key to migrate

> > +  // Make the new profile "default-" + the time in seconds since epoch for uniqueness.
> 
> Not exactly unique considered the system time can be adjusted to the past
> and has a resolution of 16ms on Windows.  As an alternative you may use a
> counter and getProfileByName till you find a free slot, should be fast since
> profiles seem to be cached in a linked list.

Named profile support will be removed in bug 214675 so I'm avoiding adding callers.  I don't think this is a blocking problem since the odds of a name collision are so low.

(In reply to Mano from comment #19)
> Comment on attachment 600263 [details] [diff] [review]
> Part 1. v.2  Backend portion - Create Profile and pass key to migrate
> 
> As discussed over #places, this results in multiple instances of the toolkit
> profile service. Everything else looks fine, but I'll finish the review once
> the service-issues is addressed.

This patch creates the service earlier and passes it as an argument to more functions.  The downside is that the ToolkitProfileService will now be created during regular startup in cases when it wouldn't have before because of early returns in SelectProfile.  The standard case of launching the default profile shouldn't be slower though.  This change also means that if the ToolkitProfileService fails to init, then using the profile command line args will no longer work.
Comment 22 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-03-08 02:21:19 PST
Created attachment 603998 [details] [diff] [review]
Part 1. v.3.1  Backend portion - Create Profile and pass key to migrate

Revert while loop in SetCurrentProfileAsDefault to v2 + nit fixes since it was wrong.

Mano, any chance this can make it on the next train?  I know support really wants this feature.
Comment 23 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-03-08 02:30:44 PST
(In reply to Matthew N. [:MattN] from comment #22)
> Mano, any chance this can make it on the next train?  I know support really
> wants this feature.

To follow-up on my last comment, since this isn't a feature that more technical users will use, it doesn't benefit much from longer baking on Nightly.  It is also easy to disable the Firefox migrator by removing it from BrowserProfileMigrators.manifest.

By the way, I addressed your comments on part 2 (removed progress bar and used throw an exception).
Comment 24 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-03-08 02:49:20 PST
Created attachment 604001 [details] [diff] [review]
Part 3. v.2 about:support box + dialog
Comment 25 Mozilla RelEng Bot 2012-03-08 08:59:37 PST
Autoland Patchset:
	Patches: 600264, 603998, 604001
	Branch: mozilla-central => try
Insufficient permissions to push to try.
Comment 26 Marco Bonardo [::mak] 2012-03-08 09:51:14 PST
Comment on attachment 604001 [details] [diff] [review]
Part 3. v.2 about:support box + dialog

Review of attachment 604001 [details] [diff] [review]:
-----------------------------------------------------------------

Some things to fix still, but globally nothing really blocking, so going on.

::: toolkit/content/aboutSupport.js
@@ +577,5 @@
> +
> +  try {
> +    if (!currentProfileDir.equals(profileService.selectedProfile.rootDir) ||
> +        !("@mozilla.org/profile/migrator;1?app=" + MOZ_BUILD_APP + "&type=" + MOZ_APP_NAME in Cc))
> +      return;

some comment above this verbose if condition may help, like "Show the reset profile option if..."

@@ +579,5 @@
> +    if (!currentProfileDir.equals(profileService.selectedProfile.rootDir) ||
> +        !("@mozilla.org/profile/migrator;1?app=" + MOZ_BUILD_APP + "&type=" + MOZ_APP_NAME in Cc))
> +      return;
> +    document.getElementById("reset-box").style.visibility = "visible";
> +  } catch (x) {

really-really-nit: we usually use ex or e in the codebase, this specific file seems to use e

@@ +594,5 @@
> +  let brandShortName = branding.GetStringFromName("brandShortName");
> +
> +  // Prompt the user to confirm.
> +  let retVals = {
> +    reset: null,

why not false?

::: toolkit/content/jar.mn
@@ +38,5 @@
>  +  content/global/mozilla.xhtml               (mozilla.xhtml)
>  *+ content/global/nsDragAndDrop.js            (nsDragAndDrop.js)
> +   content/global/resetProfile.css            (resetProfile.css)
> +*  content/global/resetProfile.js             (resetProfile.js)
> +*  content/global/resetProfile.xul            (resetProfile.xul)

ot: one day we should start partitioning a bit this folder, like we are doing with browser/base.content...

::: toolkit/content/resetProfile.js
@@ +13,5 @@
> +  var bundle = Services.strings.createBundle("chrome://" + MOZ_BUILD_APP +
> +                                             "/locale/migration/migration.properties");
> +
> +  // Loop over possible data to migrate to give the user a list of what will be preserved
> +  for (var i = 1; i < 16; ++i) {

if there's no other way, please at least put this magic number into a named const

@@ +27,5 @@
> +}
> +
> +function onOK() {
> +  var retVals = window.arguments[0];
> +  retVals.reset  = true;

double whitespace to remove

::: toolkit/locales/en-US/chrome/global/resetProfile.dtd
@@ +5,5 @@
> +<!ENTITY resetProfile.dialog.title       "Reset &brandShortName;">
> +<!ENTITY resetProfile.dialog.description "Are you sure you want to reset Firefox to its initial state?">
> +<!ENTITY resetProfile.items.label        "The following will be preserved:">
> +<!ENTITY resetProfile.footer.label       "&brandShortName; will restart and everything else will be removed.">
> +<!ENTITY resetProfile.button.label       "Reset &brandShortName;">

These should all be prefixed with dialog to help localization. so resetProfile.dialog.items.label, resetProfile.dialog.footer.label, resetProfile.dialog.acceptButton.label.
and you should have a different label for the button in about:support, like resetProfile.button.label.  Some locale may have specific needs for OK/CANCEL buttons and reusing the same string id doesn't look sane (you are obviously free to use the same string value though for en-US).

@@ +7,5 @@
> +<!ENTITY resetProfile.items.label        "The following will be preserved:">
> +<!ENTITY resetProfile.footer.label       "&brandShortName; will restart and everything else will be removed.">
> +<!ENTITY resetProfile.button.label       "Reset &brandShortName;">
> +<!ENTITY resetProfile.title              "Reset &brandShortName; to its default state">
> +<!ENTITY resetProfile.description        "If you're having major problems which you can't resolve, start fresh with only your essential information.">

I wonder if we should first suggest to ask the Mozilla Support community, users may be tempted to try this "destructive" way before asking for help. Maybe "if you're having major problems that you can't solve with the help of the Mozilla Support community, start fresh..."
Comment 27 Verdi [:verdi] 2012-03-08 10:05:11 PST
(In reply to Marco Bonardo [:mak] from comment #26)

> 
> I wonder if we should first suggest to ask the Mozilla Support community,
> users may be tempted to try this "destructive" way before asking for help.
> Maybe "if you're having major problems that you can't solve with the help of
> the Mozilla Support community, start fresh..."

No. The idea of the feature is be easy for users to discover and to give them an easy way to fix things without contacting support and/or performing the complicated and time-consuming troubshooting steps required to fix many of the issues that this will address.
Comment 28 Marco Bonardo [::mak] 2012-03-08 10:38:13 PST
ok, thanks for clarifying that.
Comment 31 Marco Bonardo [::mak] 2012-03-09 13:53:21 PST
In the review I missed a wrongly hardcoded Firefox in the dtd, thanks to Callek for catching that.

https://hg.mozilla.org/mozilla-central/rev/b9a624c9fdbe
Comment 32 Serge Gautherie (:sgautherie) 2012-03-09 15:43:51 PST
http://hg.mozilla.org/comm-central/rev/d32f2001cc1f
"Bugstage fix due to fallout from Bug 717070 landing"
Comment 33 Serge Gautherie (:sgautherie) 2012-03-09 15:46:32 PST
After Thunderbird, SeaMonkey:
http://hg.mozilla.org/comm-central/rev/1fcc5f6914e9
"Bugstage fix due to fallout from Bug 717070 landing"
Comment 34 Larry Snow 2012-03-09 23:33:49 PST
I know I probably shouldn't ask this here, but no one seems to know over on the forums.  Why does this not work with Firefox Portable?  Works fine with a regular installation, but not with my portable one.
Comment 35 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-03-10 01:56:30 PST
(In reply to Larry Snow from comment #34)
Larry, this feature is only supported on the "default" profile in an application and I suspect that Firefox portable doesn't use a "profiles.ini" file to keep track of profiles. You can still use the feature on other profiles in profiles.ini by temporarily setting them as the default.

(Quoting Mano from comment #6)
> 1. Why is the "profile" environment variable needed? It would probably be
> better to support recovering just from the default profile (If you use
> multiple profiles, you most likely do not need recovery tools like this).
Comment 36 Ioana (away) 2012-03-20 03:01:04 PDT
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20120318 Firefox/13.0a2
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120319 Firefox/13.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20120318 Firefox/13.0a2

Note You need to log in before you can comment on or make changes to this bug.