848533 - Split migration to a browser and toolkit part

Status: NEW

(Toolkit :: General, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

From the Firefox Immersive/Metro browser for Windows 8, we'd like to be able to import preferences/history/cookies/etc from other browsers that are available on the machine [1].

To be able to do that, we'll have to use nsIBrowserProfileMigrator, MigrationUtils.jsm, and other items that currently live in browser/components/migration.  The Firefox Immersive/Metro browser can't use items in browser/components, so this bug is about moving the "migration" directory from browser/components to toolkit/components.

Attached is a patch that accomplishes the move and makes migration classes available from Firefox Immersive/Metro.  I haven't yet verified that this patch doesn't break browser data import for Firefox desktop.

Gavin, could you take a look at this patch (or delegate) and let me know if it seems reasonable?  I'm not sure who else to request review from but I imagine that this move will require a couple of approvals.

[1] See bug 831610

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

Removing one change that snuck in from a different patch.

Comment 2

[Marco Bonardo [:mak]]

why does the patch rewrite nsIEHistoryEnumerator.cpp, it should just move it...

Comment 3

[Marco Bonardo [:mak]]

apart this, the migrators as written depends on Places, until they are in browser is not a big deal cause we always build browser with Places, moving to toolkit the whole component should probably be #ifdef MOZ_PLACES, since wouldn't make any sense for Android or other toolkit consumers not using Places.

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3 (still need to #ifdef MOZ_PLACES)

(In reply to Marco Bonardo [:mak] from comment #2)
> why does the patch rewrite nsIEHistoryEnumerator.cpp, it should just move
> it...

hg fail on my part.  Updated the patch to use 'hg mv' instead of rewriting nsIEHistoryEnumerator.cpp.


(In reply to Marco Bonardo [:mak] from comment #3)
> apart this, the migrators as written depends on Places, until they are in
> browser is not a big deal cause we always build browser with Places, moving
> to toolkit the whole component should probably be #ifdef MOZ_PLACES, since
> wouldn't make any sense for Android or other toolkit consumers not using
> Places.

The migrators definitely depend on Places, but it should be possible to remove the dependency of the rest of the component on Places by modifying/removing/#ifdef'ing this function [1].  Do you think it's worthwhile to '#ifdef MOZ_PLACES' _only_ the migrators, allowing the rest of the component to be used without Places?

[1] https://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/MigrationUtils.jsm?rev=5273eab26e50#418

Comment 5

[Marco Bonardo [:mak]]

I think there's more than just that function depending on Places, basically all history and bookmarks imports are written for it, and since it's the most part of that the migrators do... Plus there is no alternative implementation atm.
I think the whole component should just not be built at all if MOZ_PLACES is not defined.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v4

OK I think this patch addresses all concerns so far.

It has made it through tryserver [1] but I haven't yet tried running the builds that try has produced.

Marco, are you an appropriate reviewer for this patch?

[1] https://tbpl.mozilla.org/?tree=Try&rev=005f642dfa16

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Also, before this gets mentioned again; the reason that nsIEHistoryEnumerator.cpp seems to be rewritten is that the patch removes the line '#include "nsStringAPI.h"' - the patch fails to compile otherwise.

Comment 8

[Marco Bonardo [:mak]]

Ah I see, the problem is that line endings in the file are wrong and your editor is converting them to unix style. That's ok, these should have been correct from the beginning, likely my fault.
Yes, I can review this patch.

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v5

Updating patch to apply cleanly to current tip

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v6

The last patch didn't list nsIBrowserProfileMigrator.idl in the moz.build for migration.  Re-adding.  I'm building this patch locally and will run it through try if/when that succeeds.

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Local build succeeded, here's the try run:
  https://tbpl.mozilla.org/?tree=Try&rev=032caf26fa8a

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

:mak - ping :)

Comment 13

[Marco Bonardo [:mak]]

So, I briefly discussed this today with Mano, and we'd first of all like to figure the alternatives we may have here.

Moving this code to toolkit is wrong cause it's built strictly with Firefox Desktop UI in mind, that means it may import/change settings or UI pieces (like fonts) specifically for Firefox Desktop, and that may not really be what you want.

Are you also unable to access browser js modules or is the problem just components? we may avoid components and just use modules here.
Or, it may be worth to just copy the needed migrators code to metro folder.
Btw, before any decision we'd like some more details about what you need and what is blocking you from using the desktop code.

Comment 14

[Matt Brubeck (:mbrubeck)]

(In reply to Marco Bonardo [:mak] from comment #13)
> Are you also unable to access browser js modules or is the problem just
> components?

We cannot access any /browser code except /browser/metro.  For example in Metro Firefox, paths like resource:/// and chrome://browser/ point to the Metro app directory instead of the desktop Firefox app directory.  Metro is currently like Fennec -- a separate toolkit app that does not depend on Firefox code.

However, since Metro Firefox is bundled with desktop Firefox, maybe we can add a way to load code from the desktop Firefox app directory.  That would work for content, but maybe not for components...

> Or, it may be worth to just copy the needed migrators code to metro folder.
> Btw, before any decision we'd like some more details about what you need and
> what is blocking you from using the desktop code.

Copying files at build time would be fine with me, as long as we are not actually forking the source code.  Or perhaps we can move just the back-end components to toolkit and keep the UI parts separate.

See also the related discussion in bug 811548 comment 23 and 24.

Comment 15

[Asaf Romano (gone)]

There's no "back-end" part. The migrators assume they're migrating to Desktop Firefox. In practice, it may or may not be an issue depending on the set of preferences that we import from IE and the set of preferences that Metro Firefox support (compared to Desktop Firefox). 

Given that, and given that we rarely change migrators, I think it would be wise to go ahead fork the code. Also note that MigrationUtils has to be forked either way, because it's the entry point for the current migration UI, which I assume metro doesn't want (and you cannot get rid of MigrationUtils as a whole, because it also forms the base for all migrators).

In any case, nothing should be moved to toolkit.

Comment 16

[Asaf Romano (gone)]

FYI, the current setup works like this:
1) When there's no profile (or when --migration is used), XRE startup creates the "profile migrator" component, and calls its migrate method. Desktop Firefox implements it in ProfileMigator.js:
http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/ProfileMigrator.js
2) |migrate| is bound to MigrationUtils.startupMigration
3) startupMigration checks if there are any browser from which we can migrate (and also checks if there's a default browser set).
4) If there are, it opens the migration wizard (migration.xul).
5) The migration wizard gets the available migrators from MigrationUtils (getMigrators())
6) It's the migration wizard code (migration.js) that interacts directly with the migrators (I assume Metro Firefox would implement something much more simple, likely silent migration).
7) Migrators share a lot of their implementation by some common prototype and helper methods provided by MigrationUtils.

Comment 17

[Asaf Romano (gone)]

With all that said, we could move MigrationUtils.js to toolkit if startupMigration is moved out of it to ProfileMigrator.js - and anything that's browser-specific. So it'll just consist as a helper module for implementing migrators (for your browser or your mail client and so on).

We'd also have to move (and rename) nsIBrowserProfileMigrator to Toolkit (or remove it. There's a patch somewhere to deCOM migrators)

So, if we do that:
1) You'd need to implement the "profile migrator" component in Firefox Metro. This is where Firefox Metro would implement its migration UI entry point (no UI, simple dialog, horrible wizard as in Desktop Firefox, etc).
2) You'd need to copy over IEMigrator.js (and I guess FirefoxMigrator.js) to Firefox Metro. This isn't as bad as it sounds. This code is rarely changed, and it's quite small and easy to maintain. And, as I noted above, Metro Firefox may want to alter it a bit to support its own set of preferences etc.
3) The IE migrator also has a little c++ component for accessing IE history. This code rarely changes either, so you could copy it over too. Unlike IEMigrator.js, it's probably valid to copy it during build time.

Comment 18

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

(In reply to Mano from comment #17)
> 2) You'd need to copy over IEMigrator.js (and I guess FirefoxMigrator.js) to
> Firefox Metro.

> 3) The IE migrator also has a little c++ component for accessing IE history.
> This code rarely changes either, so you could copy it over too. Unlike
> IEMigrator.js, it's probably valid to copy it during build time.

We should avoid copying things wholesale if possible. Can't we refactor such that e.g. IEMigrator (and whatever parts of MigrationUtils it depends on) can be moved to toolkit?

Comment 19

[Marco Bonardo [:mak]]

After discussing the issue, the current decision is to split the current migration component across browser and toolkit so that Metro can use the toolkit utils, but it will have to implement its specific migrator.
Mano is going to work on the patch and I'm going to review it.
You will need to adapt your current patches to that, we'll try to make the change shortly, if you have specific deadlines please let us know.

Comment 20

[Marco Bonardo [:mak]]

(In reply to Mano from comment #17)
> 3) The IE migrator also has a little c++ component for accessing IE history.
> This code rarely changes either, so you could copy it over too. Unlike
> IEMigrator.js, it's probably valid to copy it during build time.

This component doesn't have any dependency on browser, we could move it to toolkit.

Comment 21

[Asaf Romano (gone)]

I would rather copy over on build time, if at all possible. Moving it to toolkit means either placing it in the places component or creating a migration toolkit component (which I don't need there if it's only MigrationUtils that is moved over).

Comment 22

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

If I understand the discussion thus far, we're aiming to separate things roughly like the following.

Browser
  BrowserProfileMigrationUtils.js [1]
  Firefox-desktop migrators
    IEProfileMigrator.js
    ChromeProfileMigrator.js
    SafariProfileMigrator.js
  Import wizard
    migration.xul
    migration.js
    nsIProfileMigrator.idl [2]
    ProfileMigrator.js [2]

Metro
  MetroBrowserProfileMigrationUtils.js [1]
  Firefox-metro migrators
    IEProfileMigrator.js
    ChromeProfileMigrator.js
    SafariProfileMigrator.js

Toolkit
  ProfileMigrationUtils.js [1]
  nsIEHistoryEnumerator.idl/h/cpp [3]


[1] It seems that MigrationUtils.jsm needs to be split across toolkit, browser, and metro (at a minimum these would contain the resource types available for import).  I've chosen names that might make sense, but this isn't a suggestion to use these names specifically.

[2] nsIProfileMigrator is only ever created in nsAppRunner, when potentially showing the import wizard.  This is specific to Firefox-desktop and so does not need to be moved to toolkit or replicated in metro.

[3] Alternatively, as Mano suggested in comment 21, we could keep the IE History Enumerator in browser and copy it to metro at build time.

Some thoughts that come to mind:
  1) It seems like we can get rid of the nsIBrowserProfileMigrator (note: not nsIProfileMigrator) interface entirely, but I might be missing something.
  2) If we don't move the string resources (migration.dtd, migration.properties) from browser to toolkit, we'll have to duplicate some strings across from browser to metro.  I think that's probably the right way to organize things, but it's worth noting that it might be more of a burden on localizers.

Comment 23

[Asaf Romano (gone)]

It seems to me we've much more severe issues to figure out than deciding where to place the implementation.

I'm told Metro Firefox and Desktop Firefox don't currently share the user profile because of some fundamental issues in Window 8, but that in some other way the two profiles are going to be in sync. Depending on how this profile-sharing/syncing is implemented, and more importantly, how early during the startup path one Firefox variant can *at least tell there's the other profile to sync with*, the migration story *for both Firefox variants* may vary dramatically.
- If during its first launch one variant cannot tell that rather than migrating data from "the default browser", it should migrate from the other variant, then we're going to migrate twice, with the second migation being overridden by profile-sync/share/profile-in-the-cloud at some point later. If either variants would still expose some migration UI (desktop does and I assume Metro isn't going to), the UE is going to be quite strange.
- If, on the other hand, we can tell early enough that migration has already happened in the other Firefox variant, we must not introduce any meaningful differences between the two migration paths, as it doesn't make sense to import a different set of data depending on which Firefox variant was opened first. In particular, it means that if Metro Firefox is doing silent migration (not offering profile or homepage selection), Desktop Firefox should also implement that. 

IMO, Any resolution here should also take into account future support for profile in the cloud.

*Once* this is figured out, we should first fix Desktop Firefox to implement the desired UE (silent migration, at least, I guess), and then either do build-time copies of the entire component, including the UI, or create the browser-shared directory I suggested last week and put everything there. Toolkit is also an option, I guess (de-facto browser-shared), but I'd really like to avoid that, given the defacto-API nature any file in toolkit gets over time. Unfortunately we're nowhere close to fixing that last thing.

Comment 24

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Mano from comment #23)
> It seems to me we've much more severe issues to figure out than deciding
> where to place the implementation.
> 
> I'm told Metro Firefox and Desktop Firefox don't currently share the user
> profile because of some fundamental issues in Window 8, 

I believe the fundamental issue is that Desktop Firefox and immersive/Metro Firefox can be run simultaneously, and the Mozilla platform doesn't support multiple processes using the same profile simultaneously.

> but that in some
> other way the two profiles are going to be in sync. Depending on how this
> profile-sharing/syncing is implemented, and more importantly, how early
> during the startup path one Firefox variant can *at least tell there's the
> other profile to sync with*, the migration story *for both Firefox variants*
> may vary dramatically.

The current plan is to let Firefox Sync do the job of syncing the two profiles.  For users that don't enable Firefox Sync, the two profiles will not be kept in sync.  For users that *do* enable Firefox Sync, syncing will happen much as it already does for profiles that are located on separate machines.

I understand our requirements for migration from other browsers to be "If I initiate a migration from the metro UI, then the metro profile will be populated.  If I initiate a migration from the desktop UI, then the desktop profile will be populated.  If sync is enabled, then at some later point it will notice that bookmarks/history/settings/etc have been created/modified, and do the appropriate syncing of those items."  The user story says that import should happen into both profiles simultaneously, but that requirement has been relaxed.

> - If during its first launch one variant cannot tell that rather than
> migrating data from "the default browser", it should migrate from the other
> variant, then we're going to migrate twice, with the second migation being
> overridden by profile-sync/share/profile-in-the-cloud at some point later.
> If either variants would still expose some migration UI (desktop does and I
> assume Metro isn't going to), the UE is going to be quite strange.
> - If, on the other hand, we can tell early enough that migration has already
> happened in the other Firefox variant, we must not introduce any meaningful
> differences between the two migration paths, as it doesn't make sense to
> import a different set of data depending on which Firefox variant was opened
> first. In particular, it means that if Metro Firefox is doing silent
> migration (not offering profile or homepage selection), Desktop Firefox
> should also implement that. 

Metro is (under the current projected path we're taking) going to expose a UI for migrating from other browsers on the machine, but that will not include migrating from desktop Firefox.  Desktop Firefox will continue to have the exact same browser migration functionality/offerings that it currently has.  In particular, startup migration (the migration wizard) will only happen when it currently does (on startup of desktop Firefox if there is no desktop Firefox profile available), and migration will only be available from non-Firefox browsers.
 
> IMO, Any resolution here should also take into account future support for
> profile in the cloud.

I'm not familiar with profile in the cloud, so I can't speak to this.

> *Once* this is figured out, we should first fix Desktop Firefox to implement
> the desired UE (silent migration, at least, I guess), and then either do

I believe that there are no changes necessary to the desktop Firefox UE based on the sync approach outlined above.

> build-time copies of the entire component, including the UI, or create the
> browser-shared directory I suggested last week and put everything there.
> Toolkit is also an option, I guess (de-facto browser-shared), but I'd really
> like to avoid that, given the defacto-API nature any file in toolkit gets
> over time. Unfortunately we're nowhere close to fixing that last thing.

I must have missed the browser-shared suggestion, but that sounds like a great idea to me!  I'm less of a fan of build-time copying from browser to metro, but if that turns out to be an easier/more robust/cleaner/better-smelling option then I'm not going to argue against it.

mbrubeck, jimm, bbondy, asa, or anyone else: If I've misrepresented the current state of affairs with regard to our plan for syncing profiles (or with regard to anything else) please jump in.

Comment 25

[Asaf Romano (gone)]

(In reply to Tim Abraldes (:tabraldes) (:TimAbraldes) from comment #24)

> The current plan is to let Firefox Sync do the job of syncing the two
> profiles.  For users that don't enable Firefox Sync, the two profiles will
> not be kept in sync.

I find this very hard to believe given the current user base of sync and the fact that it's somewhat deprecated.

The other problem is that the migration UE in Desktop Firefox doesn't interact with Firefox Sync at all. There's no "Firefox Sync" option in the migration wizard.

> For users that *do* enable Firefox Sync, syncing will
> happen much as it already does for profiles that are located on separate
> machines.

I didn't have a chance to play with Windows 8 metro<->desktop messy interaction myself yet, but this seems terrible.

> I understand our requirements for migration from other browsers to be "If I
> initiate a migration from the metro UI, then the metro profile will be
> populated.  If I initiate a migration from the desktop UI, then the desktop
> profile will be populated.  If sync is enabled, then at some later point it
> will notice that bookmarks/history/settings/etc have been created/modified,
> and do the appropriate syncing of those items."

Sync will do a very poor job syncing (well, merging) two migrated profiles.


So... this sounds like in Metro Firefox migration is done only on demand (and not during first startup as in Desktop Firefox)?
 
> Metro is (under the current projected path we're taking) going to expose a
> UI for migrating from other browsers on the machine, but that will not
> include migrating from desktop Firefox.  Desktop Firefox will continue to
> have the exact same browser migration functionality/offerings that it
> currently has.  In particular, startup migration (the migration wizard) will
> only happen when it currently does (on startup of desktop Firefox if there
> is no desktop Firefox profile available), and migration will only be
> available from non-Firefox browsers.

We just need to make sure that Desktop Firefox and Metro Firefox offer the same migration feature set. For example, Desktop Firefox can import from any profile in Chrome (and not just from the default profile). This won't work in Metro Firefox *iff* it migrates data silently.

> > IMO, Any resolution here should also take into account future support for
> > profile in the cloud.
> 
> I'm not familiar with profile in the cloud, so I can't speak to this.

That's, afaik, the replacement for Firefox Sync.

Comment 26

[Jim Mathies [:jimm]]

Tim sums things up well. Profile sharing is not going to happen due to various issues with two browsers trying to access the same profile at the same time. Sync service / client 2.0 (which is in the design phase about now or maybe a bit farther) is taking our needs into account such that we will (hopefully) be able to sync locally between two profiles by two independent browsers. In the meantime, we're using Sync 1.0 and syncing both over the network to our sync servers. We promote sign-up in both to try and get people connected up. This approach was a trade off due to the amount of work needed to get Sync doing things the way we would like it to.

The migration work here is really simple - we want to offer a migration option in the metro browser, just like we do in desktop. We'd also like to share as much migration code as possible between the two code bases.

I don't think the two migration wizards need to match feature for feature. The main reason we have this is that some users are never going to be over on the desktop. So we want to offer a basic migration option for these users in the immersive browser. If they use both and connect up sync in both, it's not a big deal. But if we have a user that spends 90% of his/her time in metroland, they may want to migrate 3rd party browser data. This is even more important given that we are going to be a little late to the whole metro party.