Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20120611 Firefox/15.0a2 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20120611 Firefox/15.0a2 Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20120611 Firefox/15.0a2 BuildID: 20120611042006 Mozilla/5.0 (Windows NT 6.1; rv:16.0) Gecko/16.0 Firefox/16.0a1 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/16.0 Firefox/16.0a1 Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/16.0 Firefox/16.0a1 BuildID: 20120611030526 Actual behavior: With the fix for bug 731047, when a user resets Firefox, the files and folders in his old profile are moved to the newly created "Old Firefox Data" folder (located on the Desktop). When the user resets Firefox a second time, a new folder is created "Old Firefox Data-1". When the user resets Firefox for the N-th time, a new folder is created "Old Firefox Data-N-1". The profile names are not recorded anywhere. Expected behavior: When a user resets Firefox, the folder with his old profile is moved to the newly created "Old Firefox Data" folder (located on the Desktop), not only the folder's content. This way, the profile name is kept too. When the user resets Firefox a second time, the folder with the profile he resets from is added to the "Old Firefox Data" folder. This way, all the old data is added to the same folder creating less mess.

This should do the trick, I think...

Comment on attachment 733812 [details] [diff] [review] Keep only one folder with per-profile subfolders + // It's OK if it already exists, IFF it is a directory spelling? Do we have tests for profile reset?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2) > Do we have tests for profile reset? Bug 700898 is on file for the migration portion and there aren't any compiled code tests.

This is a first attempt at a test for this feature. It wasn't as straightforward as I'd hoped, so there are some (possibly important?) questions here: GTest doesn't do xpcom yet (bug 855462). So I'm manually starting it (should I shut it down at the end or does that not matter?). A secondary problem was the progress window that the migration code opens. This is not possible before we start native app support (as far as I can tell from the code, at least - Benjamin, please correct me if I'm mistaken!). Starting native app support seems to require doing a bunch of platform-specific stuff that's holed up in XRE_mainStartup: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp?rev=128ad38ba5d5&mark=3274-3431#3438 . I didn't think copying this code out to the test was the right approach, and refactoring nsAppRunner.cpp seems way out of scope for this bug (I'm not even sure if that's easily possible as I'm not sure what all that code does). So as a workaround, I've commented out the NS_FAILED check there, and null-check progressWindow when trying to close it (this seems to be enough on OSX - I'm not familiar enough with nsCOMPtr/nsIWindowWatcher/C++ to know for sure whether the out-param is guaranteed to be null (x-platform) unless the function succeeded?). I also personally don't think that we should fail to migrate just because we can't open a window, but on the other hand, it's pretty serious if we can't... so while this is a hack of sorts, I am not sure whether it'd be perfectly fine to check in like this, or if we should hold off until we have a better solution for this problem. Separately, IMHO these complications mean we should just land the code changes now, and if we're uncomfortable landing this test as-is at the moment, I can do my best to chase it once bug 855462 gets fixed and I have some more info about how we feel about the progress window.

The previous version of the test left remnants of the test profile. Icky. Fixed now.

As far as I know, it should not be necessary to start native app support to open a window (and indeed, we shouldn't be starting native app support. How much work do you want to do on this? It would be great to have a common mechanism for gtest to start XPCOM, do stuff, and shut it down cleanly. That might be substantially similar or even become shared code with ScopedXPCOM in http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestHarness.h#99 Although it would also be nice to use XRE_InitEmbedding instead of NS_InitXPCOM, because you get a lot of the standard toolkit behaviors for free (most or all of nsXREDirProvider). I don't think that the test is required to land the patch, given that it didn't have tests to begin with, so you're welcome to spin it off.

Comment on attachment 741237 [details] [diff] [review] Test v2 >diff --git a/toolkit/xre/ProfileReset.cpp b/toolkit/xre/ProfileReset.cpp >--- a/toolkit/xre/ProfileReset.cpp >+++ b/toolkit/xre/ProfileReset.cpp >@@ -79,16 +79,18 @@ ProfileResetCleanup(nsIToolkitProfile* a > const PRUnichar* params[] = {appName.get(), appName.get()}; > > nsXPIDLString resetBackupDirectoryName; > > static const PRUnichar* kResetBackupDirectory = NS_LITERAL_STRING("resetBackupDirectory").get(); > rv = sb->FormatStringFromName(kResetBackupDirectory, params, 2, > getter_Copies(resetBackupDirectoryName)); > >+ if (NS_FAILED(rv)) return rv; Please put the return on the next line. No braces required. >@@ -136,17 +138,17 @@ ProfileResetCleanup(nsIToolkitProfile* a > > nsCOMPtr<nsIDOMWindow> progressWindow; > rv = windowWatcher->OpenWindow(nullptr, > kResetProgressURL, > "_blank", > "centerscreen,chrome,titlebar", > NULL, > getter_AddRefs(progressWindow)); >- if (NS_FAILED(rv)) return rv; >+ //if (NS_FAILED(rv)) return rv; I'm going to mark r- mainly for this. This shouldn't be necessary and we should figure out how to make the test work without making this change.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #6) > As far as I know, it should not be necessary to start native app support to > open a window (and indeed, we shouldn't be starting native app support. > > How much work do you want to do on this? It would be great to have a common > mechanism for gtest to start XPCOM, do stuff, and shut it down cleanly. That > might be substantially similar or even become shared code with ScopedXPCOM > in http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestHarness.h#99 > > Although it would also be nice to use XRE_InitEmbedding instead of > NS_InitXPCOM, because you get a lot of the standard toolkit behaviors for > free (most or all of nsXREDirProvider). > > I don't think that the test is required to land the patch, given that it > didn't have tests to begin with, so you're welcome to spin it off. Sent to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/824fccf23d6f I'll file a followup bug on the test for this particular issue. There's already a bug on file about GTest and XPCOM, bug 855462.