674168 - support chrome multiple profile migration

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement)

Description

[Makoto Kato [:m_kato]]

This is a work item after landing bug 505192.

The latest Chrome supports multiple profiles using "Local State" JSON file.  Previous version used command line option only.

Comment 1

[Matthew N. [:MattN]]

Attachment: 0.1 WIP Multi-profile support

I didn't realize this bug was already filed and assigned when I was working on this. This applies on top of the patch from bug 505192.

This is more important now that the creation and switching of user profiles is exposed in the UI.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 576719 [details] [diff] [review]
0.1 WIP Multi-profile support

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

::: browser/components/migration/public/nsIBrowserProfileMigrator.idl
@@ +76,5 @@
> +   * Get a friendly name for a profile
> +   * @param   aProfile the profile that we are looking for the name of
> +   * @returns string of the profile name
> +   */
> +  wstring getSourceProfileName(in wstring aProfile);

ugh, would be better if sourceProfiles would return an object with "name" and "path" properties, than having to get the path and pass it into again to get a name...
Shouldn't be too hard to return an array of jsval objects with those 2 properties, and the only user of this interface is migration.js (I also took a look at addons mxr and nobody uses it, so we can improve it where possible).
Afaict you would just have to fix the opera migrator, the chrome migrator would be trivial.

Btw, you should have revved the uuid!

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +484,5 @@
> +  get sourceHasMultipleProfiles()
> +  {
> +    let profiles = this.sourceProfiles;
> +    if (profiles && profiles.length > 1)
> +      return true;

the first check seems useless, sourceProfiles in the worse case returns an empty array, so you may just return this.sourceProfiles.length > 1;

::: browser/components/migration/src/nsOperaProfileMigrator.cpp
@@ +280,5 @@
>  
>  NS_IMETHODIMP
> +nsOperaProfileMigrator::GetSourceProfileName(const PRUnichar* aProfile, PRUnichar** aResult)
> +{
> +  *aResult = nsnull;

afaik opera migrator supports multiple profiles, so this is wrong.

Comment 3

[Matthew N. [:MattN]]

(In reply to Marco Bonardo [:mak] from comment #2)
> Comment on attachment 576719 [details] [diff] [review] [diff] [details] [review]
> 0.1 WIP Multi-profile support
> 
> Review of attachment 576719 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/migration/public/nsIBrowserProfileMigrator.idl
> @@ +76,5 @@
> > +   * Get a friendly name for a profile
> > +   * @param   aProfile the profile that we are looking for the name of
> > +   * @returns string of the profile name
> > +   */
> > +  wstring getSourceProfileName(in wstring aProfile);
> 
> ugh, would be better if sourceProfiles would return an object with "name"
> and "path" properties, than having to get the path and pass it into again to
> get a name...
> Shouldn't be too hard to return an array of jsval objects with those 2
> properties, and the only user of this interface is migration.js (I also took
> a look at addons mxr and nobody uses it, so we can improve it where
> possible).
> Afaict you would just have to fix the opera migrator, the chrome migrator
> would be trivial.

I considered this but didn't know if passing jsval objects was acceptable practice for XPCOM APIs.  Note that aProfile is not the path, it's just the folder name and we currently pass that to all other functions as an argument. Therefore it's not making things worse.  Ideally a  migrator and a browser profile would be separate classes and we would pass an instance of the profile to the migrator instead of the name everywhere.  I'm glad you checked addons mxr because that was another reason I didn't want to break the API unnecessarily.

So do you want me to return an array of jsval objects still?

> Btw, you should have revved the uuid!

Yep, I forgot to rev it again in this patch.

> ::: browser/components/migration/src/ChromeProfileMigrator.js
> @@ +484,5 @@
> > +  get sourceHasMultipleProfiles()
> > +  {
> > +    let profiles = this.sourceProfiles;
> > +    if (profiles && profiles.length > 1)
> > +      return true;
> 
> the first check seems useless, sourceProfiles in the worse case returns an
> empty array, so you may just return this.sourceProfiles.length > 1;

OK

> ::: browser/components/migration/src/nsOperaProfileMigrator.cpp
> @@ +280,5 @@
> >  
> >  NS_IMETHODIMP
> > +nsOperaProfileMigrator::GetSourceProfileName(const PRUnichar* aProfile, PRUnichar** aResult)
> > +{
> > +  *aResult = nsnull;
> 
> afaik opera migrator supports multiple profiles, so this is wrong.

Actually, in migration.js I check if there is a friendly name and fallback to what was shown before so the result is the same.  I don't think there is a friendly name for Opera profiles.
+ let name = this._migrator.getSourceProfileName(str);
+ item.setAttribute("label", name ? name : str.data);

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Matthew N. [:MattN] from comment #3)
> I considered this but didn't know if passing jsval objects was acceptable
> practice for XPCOM APIs.

We do this in other interfaces, where the expected caller is js. It seems to me it would largely simplify the handling here. A super-reviewer may likely give hints here, maybe ping Gavin before going head-down into the change?

> Note that aProfile is not the path, it's just the
> folder name and we currently pass that to all other functions as an
> argument. Therefore it's not making things worse.

Right, it's like a "bogus" identifier. So in your object you may have
  id: the existing folder name identifier
  name: a better name to use in the label
  path: the actual profile path

> Ideally a  migrator and a
> browser profile would be separate classes and we would pass an instance of
> the profile to the migrator instead of the name everywhere.

I feel like all the migrators stuff is overly complicated for what it does. Moving most of it to js may clarify the way to go, and maybe also kill some xpcom layers.
But yeah, those should be different entities.

> > ::: browser/components/migration/src/nsOperaProfileMigrator.cpp
> > @@ +280,5 @@
> > >  
> > >  NS_IMETHODIMP
> > > +nsOperaProfileMigrator::GetSourceProfileName(const PRUnichar* aProfile, PRUnichar** aResult)
> > > +{
> > > +  *aResult = nsnull;
> > 
> > afaik opera migrator supports multiple profiles, so this is wrong.
> 
> Actually, in migration.js I check if there is a friendly name and fallback
> to what was shown before so the result is the same.  I don't think there is
> a friendly name for Opera profiles.

Well, then this behavior should at least be documented in the idl.
But still I prefer my jsval approach with name value being the same as the id when a better name is not available.
But as I said, I'd probably ping Gavin for a SR feedback.

Comment 5

[Matthew N. [:MattN]]

Attachment: v0.2 Multi-profile support with unfriendly names

I had a discussion with Gavin on IRC and he doesn't feel that friendly names should block this bug so I filed bug 705927 for that.

This patch addresses the one remaining review comment.

Comment 6

[Matthew N. [:MattN]]

Attachment: Address TODO and rebase for initial chrome support

Rebased on top of https://hg.mozilla.org/integration/mozilla-inbound/rev/07d4c438a18b and fixed the TODO by using .append(..) which also tidied the code.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 577525 [details] [diff] [review]
Address TODO and rebase for initial chrome support

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

Globally looks fine, a couple things could be clarified, thus I'd like to take a second really-quick look once those are addressed.

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +133,5 @@
>    },
>  
>    _homepageURL : null,
>    _replaceBookmarks : false,
> +  _chromeProfile: null,

This is a bit generic named, what about _migratingProfile?

@@ +134,5 @@
>  
>    _homepageURL : null,
>    _replaceBookmarks : false,
> +  _chromeProfile: null,
> +  _profileCache: null,

This will contain info on all profiles, so should probably be _profilesCache

@@ +501,5 @@
> +  {
> +    let profiles = this.sourceProfiles;
> +    if (profiles.length > 1)
> +      return true;
> +    return false;

return this.sourceProfiles.length > 1;

@@ +513,5 @@
> +        let localState = Cc[LOCAL_FILE_CID].createInstance(Ci.nsILocalFile);
> +        // Local State is a JSON file that contains profile info.
> +        localState.initWithPath(this._paths.userData + "Local State");
> +        if (!localState.exists() || !localState.isReadable())
> +          return profiles;

Shouldn't we try to fallback to Default like in the exception case?
Actually I'd probably just throw new Components.Exception and let the catch handle it

@@ +526,5 @@
> +        str.data = index;
> +        profiles.appendElement(str, false);
> +      }
> +
> +    } catch (e) {

nit: one newline before the for to separate it from the caching code, and no newline after it since it's useless

@@ +528,5 @@
> +      }
> +
> +    } catch (e) {
> +      dump("Could not detect Chrome profiles so assuming default: " + e + "\n");
> +      if (profiles.length < 1) {

I don't understand the scope of this check, < 1 means there is no profile, so in case it can extract some profiles but not all of them we return what we found? Could you add a small comment explaining the expected behavior?
Also, the above dump is debug code that should be removed (it's also saying the untrue since we fallback only in some case)

Comment 8

[Matthew N. [:MattN]]

Attachment: v0.4 Rebase, address comments, & improve error checking

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 578123 [details] [diff] [review]
v0.4 Rebase, address comments, & improve error checking

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

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +442,4 @@
>  
>      let result = 0;
> +    if (!chromeProfileDir.exists() || !chromeProfileDir.isReadable())
> +      return result;

it's interesting that nsIBrowserProfileMigrator.idl defines 0 as ALL, so returning 0 from getMigrateData sounds like saying "we can migrate everything", rather than "we can migrate nothing"

Could you please add a @note to nsIBrowserProfileMigrator::GetMigrateData to specify a 0 return value means NONE and not ALL.
Likely the idl should have used a different const for ALL, like 0xFFFF, and define NONE as 0x0000, may even be worth filing a bug to change that (gavin should approve that change before we do it).

@@ +518,5 @@
> +      if (profiles.length < 1)
> +        return false;
> +
> +      // check that we can actually get data from the first profile
> +      result = this.getMigrateData(profiles.queryElementAt(0, Components.interfaces.nsISupportsString), false);

s/Components.interfaces/Ci/

@@ +521,5 @@
> +      // check that we can actually get data from the first profile
> +      result = this.getMigrateData(profiles.queryElementAt(0, Components.interfaces.nsISupportsString), false);
> +    } finally {
> +      return result > 0;
> +    }

I'd prefer a catch that does a Cu.reportError and the return out of the finally, let's avoid hiding errors.

@@ +538,5 @@
> +        let localState = Cc[LOCAL_FILE_CID].createInstance(Ci.nsILocalFile);
> +        // Local State is a JSON file that contains profile info.
> +        localState.initWithPath(this._paths.userData + "Local State");
> +        if (!localState.exists() || !localState.isReadable())
> +          throw new Components.Exception("Chrome's 'Local State' file doesn't exist or could not be read.");

Add a proper Cr.NS_ERROR_FILE_ACCESS_DENIED as second argument to Components.Exception, we'll let the message clarify the reason for the failed access (otherwise you should split the if and return the proper Cr.NS_ERROR_FILE_ for each)

Comment 10

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeadcd101dcb

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/eeadcd101dcb