1100118 - Allow migration from Firefox to Firefox Developer Edition

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[[e-mail]]

I think it's possible to tweak the Firefox importer so that Developer Edition will offer to import from Firefox without breaking existing Firefox Refresh functionality.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8523526 [details] [diff] [review]
Proposed patch

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

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +14,5 @@
>   */
>  
> +const Cu = Components.utils;
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;

Nit: use the newer object destructuring syntax

@@ +60,5 @@
> +    return null;
> +
> +  let iniParser = Cc["@mozilla.org/xpcom/ini-parser-factory;1"].
> +                  getService(Ci.nsIINIParserFactory).
> +                  createINIParser(profileIni);

Why are you re-implementing parts of nsToolkitProfileService when you have access to a profiles enumerator on nsIToolkitProfileService?

Comment 3

[[e-mail]]

(In reply to Matthew from comment #2)
> Why are you re-implementing parts of nsToolkitProfileService when you have
> access to a profiles enumerator on nsIToolkitProfileService?

I didn't want to depend on that.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to [e-mail] from comment #3)
> (In reply to Matthew from comment #2)
> > Why are you re-implementing parts of nsToolkitProfileService when you have
> > access to a profiles enumerator on nsIToolkitProfileService?
> 
> I didn't want to depend on that.

Why not?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8523526 [details] [diff] [review]
Proposed patch

I don't see this as a particularly compelling feature (low discoverability), so if it's going to significantly complicate the code I think we shouldn't do it. I'll let Matt decide how complicated this is.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8523526 [details] [diff] [review]
Proposed patch

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

I would consider this patch if it used the Toolkit Profile Service as that's where most of the complexity is.

::: browser/components/migration/content/migration.js
@@ +100,5 @@
>  
>      // Advance to the next page if the caller told us to.
>      if (this._migrator && this._skipImportSourcePage) {
> +      this._selectedProfile = "";
> +      this._wiz.currentPage.next = "homePageImport";

It's been a while since I looked at this code so could you explain why we need to set `this._wiz.currentPage.next` here for this patch? We skip the homePageImport step in some builds so I'm not sure if this is correct.

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +113,4 @@
>      Components.classes["@mozilla.org/toolkit/profile-service;1"]
>                .getService(Components.interfaces.nsIToolkitProfileService)
> +              .selectedProfile.rootDir;
> +  if (!sourceProfileDir)

It was possible to not have a selectedProfile before. Are you sure that's no longer the case?

Comment 7

[[e-mail]]

(In reply to Matthew from comment #6)
> (From update of attachment 8523526 [details] [diff] [review])

> >      // Advance to the next page if the caller told us to.
> >      if (this._migrator && this._skipImportSourcePage) {
> > +      this._selectedProfile = "";
> > +      this._wiz.currentPage.next = "homePageImport";
> 
> It's been a while since I looked at this code so could you explain why we
> need to set `this._wiz.currentPage.next` here for this patch? We skip the
> homePageImport step in some builds so I'm not sure if this is correct.

> > +  if (!sourceProfileDir)
> 
> It was possible to not have a selectedProfile before. Are you sure that's no
> longer the case?

Both changes are needed to keep Profile Reset or Refresh or whatever it is you're calling it these days working correctly.

Comment 8

[[e-mail]]

Attachment: Revised patch

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8542695 [details] [diff] [review]
Revised patch

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

I didn't manually test this yet to see the experience.

::: browser/components/migration/FirefoxProfileMigrator.js
@@ +14,5 @@
>   */
>  
> +const Cu = Components.utils;
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;

Nit: use object destructuring

@@ +84,3 @@
>      Components.classes["@mozilla.org/toolkit/profile-service;1"]
>                .getService(Components.interfaces.nsIToolkitProfileService)
> +              .selectedProfile.rootDir;

You still didn't reply to this question:
"It was possible to not have a selectedProfile before. Are you sure that's no longer the case?"

::: browser/components/migration/moz.build
@@ +31,5 @@
>  
>  EXTRA_PP_COMPONENTS += [
>      'BrowserProfileMigrators.manifest',
>      'ChromeProfileMigrator.js',
> +    'FirefoxProfileMigrator.js',

This file can now be reverted

Comment 10

[[e-mail]]

(In reply to Matthew N. from comment #9)
> (From update of attachment 8542695 [details] [diff] [review])
> "It was possible to not have a selectedProfile before. Are you sure that's
> no longer the case?"
Actually I don't think it was the case before; the migrator was only available for a profile reset of the selected default profile. If there's some other scenario then please let me know the STR and I'll test it.

Comment 11

[[e-mail]]

Attachment: Revised patch

I've updated the patch for the other two changes as requested, but I haven't put the null check back in.
Obviously in the migration case we'll be choosing a source profile.
In the Refresh case, this is only permitted for the default profile:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2186

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8551314 [details] [diff] [review]
Revised patch

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

r- since it doesn't work due to the improper `sourceProfiles` return value.

(In reply to [e-mail] from comment #10)
> (In reply to Matthew N. from comment #9)
> > (From update of attachment 8542695 [details] [diff] [review])
> > "It was possible to not have a selectedProfile before. Are you sure that's
> > no longer the case?"
> Actually I don't think it was the case before; the migrator was only
> available for a profile reset of the selected default profile. If there's
> some other scenario then please let me know the STR and I'll test it.

OK, I forgot that that check was also in nsAppRunner.cpp I thought it was only in the about:support JS and that the command line argument could still invoke for a non-default profile.

::: browser/components/migration/FirefoxProfileMigrator.js
@@ +48,5 @@
> +    let rootDir = profile.rootDir;
> +
> +    if (rootDir.exists() && rootDir.isReadable() &&
> +        !rootDir.equals(MigrationUtils.profileStartup.directory))
> +      allProfiles.set(profile.name, rootDir);

Please include the braces for new code.

@@ +54,5 @@
> +  return allProfiles;
> +};
> +
> +function sorter(a, b) {
> +  return a.toLocaleLowerCase().localeCompare(b.toLocaleLowerCase());

Using toLocaleLowerCase here feels weird to me since I think localeCompare is supposed to handle case differences for you based on the locale but I haven't used these APIs before.

@@ +61,5 @@
> +Object.defineProperty(FirefoxProfileMigrator.prototype, "sourceProfiles", {
> +  get: function() {
> +    let allProfiles = this._getAllProfiles();
> +    if (allProfiles && allProfiles.size)
> +      return [...allProfiles.keys()].sort(sorter);

This doesn't seem to work and shows `undefined` for all profiles names when I run firefox with the --migration argument. My understanding is that this getter is supposed to return an array of objects having keys `id` and `name` but this is just returning an array of names which leads to `undefined` being displayed.

Nit: Braces here too.

Comment 13

[[e-mail]]

(In reply to Matthew N. from comment #12)
> This doesn't seem to work and shows `undefined` for all profiles names when
> I run firefox with the --migration argument. My understanding is that this
> getter is supposed to return an array of objects having keys `id` and `name`
> but this is just returning an array of names which leads to `undefined`
> being displayed.

Sorry the patch was originally written against Firefox 31 and I hadn't notice the change in meaning introduced by bug 705927.

Comment 14

[[e-mail]]

(In reply to Matthew N. from comment #12)
> > +function sorter(a, b) {
> > +  return a.toLocaleLowerCase().localeCompare(b.toLocaleLowerCase());
> 
> Using toLocaleLowerCase here feels weird to me since I think localeCompare
> is supposed to handle case differences for you based on the locale but I
> haven't used these APIs before.

I've seen it used without any lowercasing, with just regular toLowerCase, and UITour.jsm has it with toLocaleLowerCase too. As I recall, Windows doesn't care, but Linux does.

Comment 15

[[e-mail]]

Attachment: Updated patch

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8555289 [details] [diff] [review]
Updated patch

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

Thanks and sorry for the slow reviews.

::: browser/components/migration/FirefoxProfileMigrator.js
@@ +84,3 @@
>      Components.classes["@mozilla.org/toolkit/profile-service;1"]
>                .getService(Components.interfaces.nsIToolkitProfileService)
> +              .selectedProfile.rootDir;

You can hit the following in the case of (no profiles.ini OR no Default=1) AND launching with --migration and --profile …:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIToolkitProfileService.selectedProfile]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///Users/matthew/repos/fx-team/obj-fx-opt/dist/Nightly.app/Contents/Resources/browser/components/FirefoxProfileMigrator.js :: FirefoxProfileMigrator.prototype.getResources :: line 84"  data: no]

It may not be common in practice but bailing via an Exception isn't ideal in case we need to run cleanup code. Can you put the null check back?

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8555289 [details] [diff] [review]
Updated patch

Actually, you should also fix this so it doesn't load about:welcomeback after an import. You probably want to skip the 3 prefs getting set and migration the whole sessionstore.js file: https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/FirefoxProfileMigrator.js?rev=379e016a9f05&mark=120-125#116

Comment 18

[[e-mail]]

(In reply to Matthew N. from comment #16)
> You can hit the following in the case of (no profiles.ini OR no Default=1)
> AND launching with --migration and --profile …:
...
> Can you put the null check back?

First I'd like to find out how we get into that code path; if you have no profiles we shouldn't be trying to migrate at all, and if we're migrating the null profile then that should only be because of Firefox Refresh which only works on the default profile.

(In reply to Matthew N. from comment #17)
> Actually, you should also fix this so it doesn't load about:welcomeback
> after an import. You probably want to skip the 3 prefs getting set and
> migration the whole sessionstore.js file:
> https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/
> FirefoxProfileMigrator.js?rev=379e016a9f05&mark=120-125#116

Thanks, I'll look into that.

Comment 19

[[e-mail]]

(In reply to Matthew N. from comment #16)
> You can hit the following in the case of (no profiles.ini OR no Default=1)
> AND launching with --migration and --profile …:
> 
> [Exception... "Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsIToolkitProfileService.selectedProfile]"  nsresult:
> "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
> file:///Users/matthew/repos/fx-team/obj-fx-opt/dist/Nightly.app/Contents/
> Resources/browser/components/FirefoxProfileMigrator.js ::
> FirefoxProfileMigrator.prototype.getResources :: line 84"  data: no]
> 
> It may not be common in practice but bailing via an Exception isn't ideal in
> case we need to run cleanup code. Can you put the null check back?

The null check isn't relevant; something went wrong with my Firefox Refresh detection.

Comment 20

[[e-mail]]

Attachment: Fixed patch

* Fixed Refresh when other profiles exist
* Except for Refresh, only migrate sessionstore.js
* Return an empty array when there are no available source profiles
(null was incorrect behaviour in this case and triggered that exception)

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8557995 [details] [diff] [review]
Fixed patch

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

Thanks. We'll want to make sure QE verifies that this feature works without regressing Reset with one or more than one profile.

Comment 22

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77537f14b736

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77537f14b736

Comment 24

[Petruta Horea [:phorea], Desktop QA]

Could you please provide a few details on how to verify this?

I tried under Win 8.1 64-bit with Developer Edition 39.0a2:
 - when no profile is created, Dev Edition opens without the option to import from any browser
 - "Import Data from Another Browser" option from Library window has only IE and Chrome to import from
 - "Import Bookmarks from HTML" (html file added from Firefox 38 beta 8) only adds bookmarks; there is no history and no cookies

Comment 25

[Matthew N. [:MattN]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #24)
>  - when no profile is created, Dev Edition opens without the option to
> import from any browser

Did you try removing the profiles.ini file?

Comment 26

[[e-mail]]

(In reply to Petruta Rasa from comment #24)
> Could you please provide a few details on how to verify this?

There's no new UI for the feature; you have to launch Firefox with the -migration option.

(In reply to Matthew N. from comment #25)
> Did you try removing the profiles.ini file?

Wouldn't that defeat the object?

Comment 27

[Matthew N. [:MattN]]

(In reply to [e-mail] from comment #26)
> (In reply to Petruta Rasa from comment #24)
> > Could you please provide a few details on how to verify this?
> 
> There's no new UI for the feature; you have to launch Firefox with the
> -migration option.

I could have sworn the UI worked when I tested.

> (In reply to Matthew N. from comment #25)
> > Did you try removing the profiles.ini file?
> 
> Wouldn't that defeat the object?

Oops, of course.

Comment 28

[Petruta Horea [:phorea], Desktop QA]

(In reply to [e-mail] from comment #26)
> (In reply to Petruta Rasa from comment #24)
> > Could you please provide a few details on how to verify this?
> 
> There's no new UI for the feature; you have to launch Firefox with the
> -migration option.

Filled bug 1159732 for Developer Edition not showing the import dialog at start-up. 

Launching Developer Edition (or Nightly) with -migration option offers to import from Firefox and shows all the existing profiles so that the user can choose from which profile to import data.

Was this implemented to also work with the option from Library? I think I saw it once, but I don't seem to reproduce anymore.

Comment 29

[Matthew N. [:MattN]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #28)
> Was this implemented to also work with the option from Library? I think I
> saw it once, but I don't seem to reproduce anymore.

No, it should never show there since the Firefox migrator is a startup-only migrator since it works by replacing whole files instead of just inserting records in databases. We can replace some of those files while Firefox is already running.

Comment 30

[Petruta Horea [:phorea], Desktop QA]

Thanks, Matthew!

Marking as verified as per above comments.