Closed Bug 705927 Opened 13 years ago Closed 10 years ago

Support friendly names for profile selection during migration

Categories

(Firefox :: Migration, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: MattN, Assigned: MattN)

Details

(Whiteboard: p=2)

Attachments

(1 file, 1 obsolete file)

Migration code currently passes a profile directory name around as a pseudo identifier for migrators which support multiple profiles.  The profile directory may be unfriendly and non-descriptive which makes it hard for a user to distinguish their profiles on the migrator profile selection page.

For example, Google Chrome creates profile directories before letting the user choose a name.  It names the n-th profile directory "Profile n" (where n > 1) which leads to profile selection UI of:

* Default
* Profile 2
* Profile 3
...

Where applicable, we should read the friendly, user-facing name out of the existing profiles and use them on the profile selection screen.
Blocks: 706973
Mathew, this should be very easy to do now. The array is already returned in a jsval form, so you can put whatever you want there.

Try this in your scrachpad:

Components.utils.import("resource://gre/modules/FileUtils.jsm");

let file = new FileUtils.File("~/");
let fileWrapped = Object.create(file, { "toString": { value: function() "MyFile" } });
alert(fileWrapped);

Of course, you could just set toString on |file|, but i wouldn't rely on xpconnect to be nice and gentle. This proto-option should be safer.
Assignee: nobody → MattN+bmo
Attachment #577415 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8432186 - Flags: review?(mano)
No longer blocks: 706973
Comment on attachment 8432186 [details] [diff] [review]
v.2 Make sourceProfiles an array of profile objects

My main concern is that's it'd be confusing to call Chromium "Chrome". If the user has both browsers installed, he'd get a combined-profiles list even though he wasn't introduced to profiles in Chrome at all. Even worse, "default", or any other profile name, could be used in both browser, and the profile selection list in the migration wizard would not reflect what's this "default" and what's the other. Sure, we could include this info in the profile object, but I don't think it's nice to call Chromium Chrome in Wizard Page A and Chromium in Wizard Page B.

I think that a better solution is to define multiple migrators, and reuse the migrator prototype (IIRC, it's the component's constructor that does the path checks, so it's going to shape nicely - shared prototype, multiple constructors. Of course, it would be somewhat easier if migrators were deCOMed by now, but you can tweak chrome.manifest to collect multiple components from one file.

By the way, nitty-nit: please always prefer "let" over "var" in new code.
Attachment #8432186 - Flags: review?(mano) → review-
Oh well, I should have made this comment in bug 706973.
Comment on attachment 8432186 [details] [diff] [review]
v.2 Make sourceProfiles an array of profile objects

I discussed with Mano on IRC.

We still want this fix to show the friendly name for profiles of a single migrator. See paragraph 2 about how the directory name and friendly name differ for Chrome.
Attachment #8432186 - Flags: review- → review?(mano)
Comment on attachment 8432186 [details] [diff] [review]
v.2 Make sourceProfiles an array of profile objects


>       var sourceProfiles = this._migrator.sourceProfiles;
>-      for (var i = 0; i < sourceProfiles.length; ++i) {
>+
>+      for (var profile of sourceProfiles) {

let


>   onSelectProfilePageRewound: function ()
>   {
>     var profiles = document.getElementById("profiles");
>-    this._selectedProfile = profiles.selectedItem.id;
>+    let matchingProfiles = this._migrator.sourceProfiles.filter(
>+      (profile) => profile.id == profiles.selectedItem.id

just |profile =>|.

>   onSelectProfilePageAdvanced: function ()
>   {
>     var profiles = document.getElementById("profiles");
>-    this._selectedProfile = profiles.selectedItem.id;
>-    
>+    let matchingProfiles = this._migrator.sourceProfiles.filter(
>+      (profile) => profile.id == profiles.selectedItem.id

ditto.

>+    );
>+    this._selectedProfile = matchingProfiles.length ? matchingProfiles[0] : null;
>+

This seems like a good early adopter of Array.find. In any case, Array.filter is wrong.


>diff --git a/browser/components/migration/public/nsIBrowserProfileMigrator.idl b/browser/components/migration/public/nsIBrowserProfileMigrator.idl
>--- a/browser/components/migration/public/nsIBrowserProfileMigrator.idl
>+++ b/browser/components/migration/public/nsIBrowserProfileMigrator.idl

rev uuid.

>diff --git a/browser/components/migration/src/ChromeProfileMigrator.js b/browser/components/migration/src/ChromeProfileMigrator.js
>--- a/browser/components/migration/src/ChromeProfileMigrator.js
>+++ b/browser/components/migration/src/ChromeProfileMigrator.js

>-    let profiles;
>+    let profiles = [];
>     try {
>       // Local State is a JSON file that contains profile info.
>       let localState = this._chromeUserDataFolder.clone();
>       localState.append("Local State");
>       if (!localState.exists())
>         throw new Error("Chrome's 'Local State' file does not exist.");
>       if (!localState.isReadable())
>         throw new Error("Chrome's 'Local State' file could not be read.");
> 
>       let fstream = Cc[FILE_INPUT_STREAM_CID].createInstance(Ci.nsIFileInputStream);
>       fstream.init(localState, -1, 0, 0);
>       let inputStream = NetUtil.readInputStreamToString(fstream, fstream.available(),
>                                                         { charset: "UTF-8" });
>       let info_cache = JSON.parse(inputStream).profile.info_cache;
>-      if (info_cache)
>-        profiles = Object.keys(info_cache);
>+      for (let profile in info_cache) {

s/profile/profileFolderName/

>+        let profileFolder = this._chromeUserDataFolder.clone();
>+        profileFolder.append(profile);
>+        let p = {
>+          id: profile,
>+          name: info_cache[profile].name || profile,
>+        }
>+        profiles.push(p);

profiles.push({ id: profileFolderName
              , info_cache[profileFolderName].name || profile });


>+      }
>     } catch (e) {
>       Cu.reportError("Error detecting Chrome profiles: " + e);
>       // If we weren't able to detect any profiles above, fallback to the Default profile.
>       let defaultProfileFolder = this._chromeUserDataFolder.clone();
>       defaultProfileFolder.append("Default");
>       if (defaultProfileFolder.exists())
>-        profiles = ["Default"];
>+        profiles = [{
>+          id: "Default",
>+          name: "Default",
>+        }];

Wrap the block or revert to a single line (I prefer the later). Actually, I'd just |push|.
Attachment #8432186 - Flags: review?(mano) → review+
(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #6)
> >       if (defaultProfileFolder.exists())
> >-        profiles = ["Default"];
> >+        profiles = [{
> >+          id: "Default",
> >+          name: "Default",
> >+        }];
> 
> Wrap the block or revert to a single line (I prefer the later). Actually,
> I'd just |push|.

I left this replacing profiles since the exception could have happened after we already pushed profiles and it's probably better to just show the default in that case (especially so we don't have duplicate "Default"s). I kept the profile objects multi-line so readability and so blame won't change when a not property is added.

Pushed: https://hg.mozilla.org/integration/fx-team/rev/875df781fa8b
Whiteboard: p=2
https://hg.mozilla.org/mozilla-central/rev/875df781fa8b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: