I have a rough patch already. It works on linux, not sure about other platforms. While this might not seem necessary at first glance, this is for a browser that will be built off of Firefox. Normal Firefox builds will never see an option to import a Firefox profile because if they already have one they won't see the import dialog.

oh, and I need to look up the location of the profiles on the other platforms.

Comment on attachment 168295 [details] [diff] [review] 273874-1.diff Ben, could you please glance over this patch and tell me if I'm doing anything majorly wrong? I'm not actually requesting r= yet, I'll wait until I can test on some other platforms first. Thanks.

it's pretty much finished, I just need to get it working on all the other platforms.

Comment on attachment 170335 [details] [diff] [review] 273874-2.diff there just isn't a valid argument why Firefox would want this code. If another browser based on Firefox wants this, they can use it for their own builds. This is no different than adding code to /browser for any other functionality that Firefox neither wants or needs.

(In reply to comment #0) > ...Normal Firefox builds will never see an option to > import a Firefox profile because if they already have one they won't see the > import dialog. > That ability would be useful for when something goes wrong, and you have a profile backed up somewhere else and you want to grab just bookmarks and cookies for instance. No?

What about when you start Firefox and it creates a profile, then you want to import your settings from another profile (backup stored on USB for example)? This would be far easier than find/replace entire profiles, especially when there are multiple profiles existing. Think of nightly testers using this.

I'll be working on this as part of the profile cleanup feature[1]. It will create a new profile for a user and migrate their data over. This will also be useful for users to migrate data between profiles for different channels[2]. I have a JS implementation with support for bookmarks that I will attach shortly. [1] https://wiki.mozilla.org/Support/Firefox_Features/Clean_up_user_profile [2] https://wiki.mozilla.org/Features/Desktop/Ability_to_run_concurrent_channels

Here is a working version that uses the toolkit profile service which is planned to be removed in bug 214675. I'm flagging for feedback since I'd like to hear from bsmedberg on how he'd rather me get a list of profiles? Should I just parse profiles.ini myself? I personally think that as long as we have profiles.ini, we should have a common API to manipulate it so I'd rather this functionality not be removed in bug 214675. In another bug for the reset profile feature[1] I'll have to create a new profile so I'd be using toolkit profile service there too if it wasn't going away. mak, ignoring the above, could you do a pass on this? It supports bookmarks using the JSON backups and history, passwords, cookies by just copying the related files. This will be the basis for the clean up feature[1] and the plan is to iterate on the individual data types to improve the error handling in follow-up bugs. For example, instead of just copying cookies.sqlite, a follow-up would attempt to query the database and insert the valid results into a new cookie database. This would attempt to catch cases of database corruption. [1] https://wiki.mozilla.org/Support/Firefox_Features/Clean_up_user_profile

well, if you copy places.sqlite there is poor advantage in copying bookmarks, since they are inside places.sqlite. Maybe I'd just copy the database for now, and in future, when we'll have an history backup, we can re-evaluate that.

Export bookmarks to json, copy places.sqlite, import bookmarks from json.

(In reply to Igor Velkov from comment #10) > Export bookmarks to json, copy places.sqlite, import bookmarks from json. what's the scope, places.sqlite includes bookmarks.

Comment on attachment 582183 [details] [diff] [review] v.1 Support for bookmarks & file copies of some others Most of the point of removing the profile manager and profile service was that there won't *be* a list of named profiles. There will just be a single (default) profile directory, and if you want to use a different directory you can use the -profile flag (or a custom UI a-la the "new" standalone profile manager). So I really don't want to code in anything which assumes that there will be multiple named profiles and you have to choose one. It is ok for now to use the profile service to get the location of the "default" profile. If we eventually have different profiles for Nightly/Aurora/Beta, we should treat those as different products, not different named profiles as in the profile manager.

MattN, please clarify if you want a review on the remaining parts, (excluding bookmarks) or you are going to make a new revision addressing bookmarks and multiple profiles concerns rised in previous comments.

This patch now only uses the toolkit profile service to get the default profile and import from it if it's not the currently running profile. Instead of importing bookmarks from the json backups, I just copy the bookmark backup directory to the new profile so that it can be used to restore from in case there is a problem with places.sqlite.

(In reply to Matthew N. [:MattN] from comment #14) > I just copy the > bookmark backup directory to the new profile so that it can be used to > restore from in case there is a problem with places.sqlite. this is a really great idea!

Comment on attachment 585650 [details] [diff] [review] v.2 Only look for default profile & get bookmarks from places.sqlite Review of attachment 585650 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/migration/src/ChromeProfileMigrator.js @@ +16,4 @@ > * The Original Code is the Browser Profile Migrator. > * > * The Initial Developer of the Original Code is the Mozilla Foundation. > * Portions created by the Initial Developer are Copyright (C) 2011 2012... btw actually could you may just use MPL2? http://www.mozilla.org/MPL/headers/ @@ +43,5 @@ > const Cr = Components.results; > const MIGRATOR = Ci.nsIBrowserProfileMigrator; > > const LOCAL_FILE_CID = "@mozilla.org/file/local;1"; > +const TOOLKIT_PROFILE_SVC_CID = "@mozilla.org/toolkit/profile-service;1"; this is used just once, I really don't care it being a const. @@ +122,5 @@ > + let bookmarksBackupDir = this._sourceProfile.clone(); > + bookmarksBackupDir.append("bookmarkbackups"); > + if (!bookmarksBackupDir.exists()) { > + throw("Bookmarks backup folder does not exist." + bookmarksBackupDir.path); > + } Actually, this is not a strong error condition, it may happen that the user has bookmarks but not yet a backup, or he may have disabled backups for whatever reason (it's feasible through about:config). @@ +134,5 @@ > { > this._notifyStart(MIGRATOR.BOOKMARKS); > > try { > + let srcBackupsDir = this._bookmarks_backup_folder; As I said, failing here is sign there are no backups, but not that importing bookmarks failed. we should probably ignore error and always report success, after all bookmarks are imported with places.sqlite. @@ +268,5 @@ > * @param aDoingStartup > * non-null if called during startup. > * @return supported migration types > + * > + * @todo make sure source databases are not in-use is this something you have to fix here yet, or willing to follow-up? if the latter this needs a bug number. @@ +292,5 @@ > } catch (e) { > Cu.reportError(e); > } > > + // The following migrations may cause damage if done while the desination profile is already typo: desination @@ +293,5 @@ > Cu.reportError(e); > } > > + // The following migrations may cause damage if done while the desination profile is already > + // running so only allow them when migrating during startup. Actually, in the backups migrator, you migrate backups only if the dest profile doesn't have a backups folder... if the destination profile is already running it likely has backups, so just conditioning all migrators on aDoingStartup would not change much and avoid differences that would only hit edge cases. if we condition everything on aDoingStartup we may check this in sourceExist and make this a migrator that can only be used on startup. @@ +315,3 @@ > try { > + let file = this._sourceProfile.clone(); > + file.append("cookies.sqlite"); I think you should also copy permissions.sqlite for cookies. @@ +363,5 @@ > + for (let i = 0; i < profiles.length; i++) { > + result = this.getMigrateData(profiles.queryElementAt(i, Ci.nsISupportsString), false); > + if (result) > + break; > + } shouldn't this check the profile we try to import in sourceProfiles (the selectedProfile), rather than any profile? You may move that code to an helper, included the check that we are not trying to import the current profile @@ +402,5 @@ > * Return home page URL > * > * @return home page URL > */ > get sourceHomePageURL() this should be a todo, I suppose?

Comment on attachment 585650 [details] [diff] [review] v.2 Only look for default profile & get bookmarks from places.sqlite Will this non-Firefox browser have the same appname (in application.ini) as Firefox? If not, the profile service will look in the wrong place for profiles.ini and you wouldn't find the data anyway, right? I feel like this is a lot of change in code which is already poorly tested and regression-prone, but that's really for mak or some other peer to decide overall.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #17) > Comment on attachment 585650 [details] [diff] [review] > v.2 Only look for default profile & get bookmarks from places.sqlite > > Will this non-Firefox browser have the same appname (in application.ini) as > Firefox? If not, the profile service will look in the wrong place for > profiles.ini and you wouldn't find the data anyway, right? The main motivation is for this to be able to import data to a new Firefox profile from the default Firefox profile in order to clean up and fix problems [1]. The idea is to automate the manual processes that support directs people to when they are having problems with their profile[2][3] some of which will become very difficult when the profile manager UI is removed. This patch currently replaces a large portion of the steps on [2] and will save users a lot of time. > I feel like this is a lot of change in code which is already poorly tested > and regression-prone, but that's really for mak or some other peer to decide > overall. The changes are basically the same as was done for the Chrome migrator in bug 505192 and a testing harness is in progress in bug 700898 and bug 706017. [1] https://wiki.mozilla.org/Support/Firefox_Features/Clean_up_user_profile [2] https://support.mozilla.com/en-US/kb/Recovering%20important%20data%20from%20an%20old%20profile [3] https://support.mozilla.com/en-US/kb/Managing%20profiles

actually splinter review doesn't show correctly that this adds a new file implementing the migrator, doesn't modify lots of code (it shows it as modifying an existing file cause you used hg cp). Still, the lack of tests in Migration can't surely be denied!

(In reply to Marco Bonardo [:mak] from comment #16) > Comment on attachment 585650 [details] [diff] [review] > v.2 Only look for default profile & get bookmarks from places.sqlite > > Review of attachment 585650 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/migration/src/ChromeProfileMigrator.js > @@ +16,4 @@ > > * The Original Code is the Browser Profile Migrator. > > * > > * The Initial Developer of the Original Code is the Mozilla Foundation. > > * Portions created by the Initial Developer are Copyright (C) 2011 > > 2012... btw actually could you may just use MPL2? > http://www.mozilla.org/MPL/headers/ My plan was to switch to MPL2 since I saw your comment in another bug but I was trying to minimize the diff from the Chrome migrator. > @@ +315,3 @@ > > try { > > + let file = this._sourceProfile.clone(); > > + file.append("cookies.sqlite"); > > I think you should also copy permissions.sqlite for cookies. I don't think that cookie permissions are in the 80% case and blocked cookies preventing logins could be the reason a user is migrating to a new profile so I'd rather hold off on this for now. If it is deemed necessary then I'd rather do selective migration instead of a DB copy since I've seen the default add-on permissions get lost on profiles (eg. bug 506446) so I'd rather start with good defaults. > @@ +363,5 @@ > > + for (let i = 0; i < profiles.length; i++) { > > + result = this.getMigrateData(profiles.queryElementAt(i, Ci.nsISupportsString), false); > > + if (result) > > + break; > > + } > > shouldn't this check the profile we try to import in sourceProfiles (the > selectedProfile), rather than any profile? > You may move that code to an helper, included the check that we are not > trying to import the current profile The problem is that sourceExists is called before a profile is selected and I was avoiding coding to the assumption that sourceProfiles will only return one profile. > @@ +402,5 @@ > > * Return home page URL > > * > > * @return home page URL > > */ > > get sourceHomePageURL() > > this should be a todo, I suppose? I considered this a preference and I'll migrate them in a follow-up (bug 715348). I added a comment about that.

(In reply to Matthew N. [:MattN] from comment #20) > I don't think that cookie permissions are in the 80% case and blocked > cookies preventing logins could be the reason a user is migrating to a new > profile so I'd rather hold off on this for now. Ok, though it's worth to add a comment saying this to the cookie migrator. > The problem is that sourceExists is called before a profile is selected Add a comment on this, as well.

Comment on attachment 585906 [details] [diff] [review] v.3 don't support migration via UI Review of attachment 585906 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/migration/src/ChromeProfileMigrator.js @@ +7,2 @@ > > const Cc = Components.classes; Since this migrator is "special" in the sense it's migrating Firefox to itself, it's worth to add a brief multiline comment block at the top explaining the reasoning for it. @@ +86,5 @@ > + */ > + get _bookmarks_backup_folder() > + { > + let bookmarksBackupDir = this._sourceProfile.clone(); > + bookmarksBackupDir.append("bookmarkbackups"); Would be healthier to add a folderRelativePath (or something similar) to PlacesUtils.backups to help maintenability in case backups should be moved in future (most likely there will be a separate module to handle those, and doing a search would then easily find this point to be fixed). @@ +252,1 @@ > return result; Just to be sure, this means we will show the option to Migrate Firefox, but show no migrator available, or the option is not shown if no migrator is available but sourceExists returns true? If possible we should ensure Firefox is not a selectable option when migrating from the UI, otherwise would be a dead one. @@ +261,5 @@ > } catch (e) { > Cu.reportError(e); > } > > + // places I prefer if you refer to it as // Bookmarks, history and favicons. @@ +347,5 @@ > + if (!this._profilesCache) > + { > + this._profilesCache = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray); > + let profileService = Cc["@mozilla.org/toolkit/profile-service;1"].getService(Ci.nsIToolkitProfileService); > + var profile = profileService.selectedProfile; Add a comment on the reasoning we only support the selected profile. @@ +366,5 @@ > * Return home page URL > * > * @return home page URL > + * > + * @todo bug 715348 add also a brief comment on what is that bug supposed to handle

(In reply to Marco Bonardo [:mak] from comment #22) > @@ +252,1 @@ > > return result; > > Just to be sure, this means we will show the option to Migrate Firefox, but > show no migrator available, or the option is not shown if no migrator is > available but sourceExists returns true? > If possible we should ensure Firefox is not a selectable option when > migrating from the UI, otherwise would be a dead one. If sourceExists returns false, Firefox is not shown in the list of migrators available, therefore Firefox will never be shown when migrating from the UI.

I mean, when launched from the UI, I imagine sourceExists returns true, so the Firefox entry should appear. but there is no data we can migrate because we are not in the startup path, so ideally the entry should not appear. Otherwise the user can choice Firefox but then he can't migrate anything. What happens really?

Comment on attachment 586736 [details] [diff] [review] v.4 address review comments Review of attachment 586736 [details] [diff] [review]: ----------------------------------------------------------------- It looks good enough to land imo. I would just like to figure out comment 24, and a better name for the backups folder path getter, but those are minor changes. ::: browser/components/migration/src/ChromeProfileMigrator.js @@ +2,5 @@ > * vim: sw=2 ts=2 sts=2 et > + * This Source Code is subject to the terms of the Mozilla Public License > + * version 2.0 (the "License"). You can obtain a copy of the License at > + * http://mozilla.org/MPL/2.0/. > + */ nit: the boilerplate has been upgraded to have /* and /* in line with the licence @@ +332,5 @@ > let profiles = this.sourceProfiles; > if (profiles.length < 1) > return false; > + // check that we can get data from at least one profile since profile selection has not > + // happened yet global-really-nit: proper punctuation in comments (ucfirst, end with perios, use commas) ::: toolkit/components/places/PlacesUtils.jsm @@ +1827,5 @@ > delete this.folder; > return this.folder = bookmarksBackupDir; > }, > > + get profileRelativeFolder() { clarify in the name that this is a path (even just by a Path suffix), otherwise it looks like an nsILocalFile

(In reply to Marco Bonardo [:mak] from comment #24) > I mean, when launched from the UI, I imagine sourceExists returns true, so > the Firefox entry should appear. but there is no data we can migrate because > we are not in the startup path, so ideally the entry should not appear. > Otherwise the user can choice Firefox but then he can't migrate anything. > What happens really? sourceExists only returns true when getMigrateData(..) > 0 so the user won't get to the data selection screen if there is no data to migrate. (In reply to Marco Bonardo [:mak] from comment #22) > ::: browser/components/migration/src/FirefoxProfileMigrator.js > @@ +86,5 @@ > > + */ > > + get _bookmarks_backup_folder() > > + { > > + let bookmarksBackupDir = this._sourceProfile.clone(); > > + bookmarksBackupDir.append("bookmarkbackups"); > > Would be healthier to add a folderRelativePath (or something similar) to > PlacesUtils.backups to help maintenability in case backups should be moved > in future (most likely there will be a separate module to handle those, and > doing a search would then easily find this point to be fixed). After testing patch v4 more I realized why I wasn't using PlacesUtils.backups.folder before: That code uses "profD" but when the migrator is run during startup it must use "profDS" since "profD" is not setup yet. I changed the approach to accommodate this. Please re-review.

Comment on attachment 587256 [details] [diff] [review] v.5 bookmark backup folder lookup Review of attachment 587256 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/PlacesUtils.jsm @@ +1830,5 @@ > > + get profileRelativeFolderPath() { > + delete this.profileRelativeFolderPath; > + return this.profileRelativeFolderPath = "bookmarkbackups"; > + }, so, if this is trying to make the property readonly, seems wrong, since it's replacing the getter with a simple property, otherwise this may just be get profileRelativeFolderPath() "bookmarkbackups", Or I miss anything?

(In reply to Marco Bonardo [:mak] from comment #27) > get profileRelativeFolderPath() "bookmarkbackups", I replaced it with your version https://hg.mozilla.org/integration/mozilla-inbound/rev/c550ffd3ba74

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