Asaf Romano (gone) Assignee
	Description • 13 years ago

In bug 710259, I started writing a shared module for migration. I'm moving the work on it to this bug.

Goals:
1. Unify migration notifications handling.
2. Do away with error-prone error-handling in each migrator (at least in sync cases).
3. Prepare for deCOMing migrators (bug 718280)
3.1. Prepare for broken and unused "profiles" support from migrators api.
4. Shrink migrators code.
5. General cleanup of Firefox and Chrome migrators.
6. Basic tests for the new module

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 1 • 13 years ago

Comment on attachment 589155 [details] [diff] [review]
wip

>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/migration/src/MigrationUtils.jsm	Tue Jan 17 14:22:49 2012 +0200
>+let MigrationUtils = Object.seal({
>+   * @note if multiple resource objects are set for a single migration type,

then what?

	Asaf Romano (gone) Assignee
	Comment 2 • 13 years ago

Thanks for the spot. I need to move over the documentation part. It's not completely in sync with the final design.

	Asaf Romano (gone) Assignee
	Comment 3 • 13 years ago

I still need to go over some comments and update the IE migrator.

	Marco Bonardo [:mak]
	Comment 4 • 13 years ago

Comment on attachment 589480 [details] [diff] [review]
ready for first pass

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

Not finished, I still have to check the module, and the re-check the Chrome migrator, though I can start with some comment/questions.

I'm mostly worried by the number of changes to the Chrome Migrator, we must ensure to not regress its functionality, that without tests is scary.

::: browser/components/migration/public/nsIBrowserProfileMigrator.idl
@@ +78,4 @@
>     * browser (i.e. whether or not it is installed, and there exists
>     * a user profile)
>     */
> +  boolean getSourceExists(in boolean aDoingStartup);

To be a bit more compatible, but mostly to avoid blind boolean arguments, what about adding a separate sourceExistsOnStartup or similar, instead?

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +12,4 @@
>  
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

likely may avoid lazygetter on services

@@ +42,5 @@
> +
> +  this._migrator = aMigrator;
> +  this._startupMigration = aStartupMigration;
> +
> +  let file = aMigrator._chromeUserDataFolder.clone();

we may expose a getter for the source folder... it's the same in the FF migrator isn't it?

@@ +150,5 @@
> +
> +// TODO: This will become a true migration source in bug 718189
> +function HomePage(aMigrator, aProfile, aStartupMigration) {
> +  FileResource.call(this, aMigrator, aProfile, aStartupMigration,
> +                    /* MigrationUtils.itemTypes.HOMEPAGE */ -1, "Preferences");

may we have a special itemTypes property for -1, like UNSUPPORTED or similar.
Btw, I can't find where this magic -1 is handled, if it's handled (that the comment makes me think is not).
This is sort of confusing, maybe better to just let it alone as it was till we can make it a proper source.

@@ +155,5 @@
> +}
> +HomePage.prototype = Object.create(FileResource.prototype);
> +
> +// Prototype for History & Cookies resources, which are stored in
> +// a sql3lite data base.

typo: sql3lite, should be SQLite

@@ +165,5 @@
> +StorageResource.prototype = Object.create(FileResource.prototype);
> +
> +StorageResource.prototype.migrate = function(aCallback) {
> +  this._dbConn = Services.storage.openUnsharedDatabase(this._file);
> +  this._success = true;

init to false and set in handleCompletion to aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED

@@ +168,5 @@
> +  this._dbConn = Services.storage.openUnsharedDatabase(this._file);
> +  this._success = true;
> +  this._callback = aCallback;
> +  let stmt = this._dbConn.createAsyncStatement(this._SQLStatement);
> +  stmt.executeAsync(this);

these 2 may throw, better catch, callback in such a case and finalize()+asyncClose() in a finally
Indeed you may asyncClose here instead of waiting completion.

@@ +190,5 @@
> +function History(aMigrator, aProfile, aStartupMigration) {
> +  StorageResource.call(this, aMigrator, aProfile, aStartupMigration,
> +                       MigrationUtils.itemTypes.HISTORY, "History",
> +                       "SELECT url, title, last_visit_time, typed_count FROM \
> +                        urls WHERE hidden = 0");

I think this generates queries with lots of whitespace in the Storage log, please just use the classic +

@@ +237,5 @@
> +function Cookies(aMigrator, aProfile, aStartupMigration) {
> +  StorageResource.call(this, aMigrator, aProfile, aStartupMigration,
> +                       MigrationUtils.itemTypes.COOKIES, "Cookies",
> +                       "SELECT host_key, path, name, value, secure,httponly, \
> +                        expires_utc FROM cookies");

ditto

@@ +297,4 @@
>    },
>  
> +  sourceProfiles: {
> +    get: function() {

is there a reason to define getters like this?

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +25,4 @@
>  
> +/**
> + * Prototype for copy-over-file/s migration resources.
> + */

more documentation please, the params especially

@@ +26,5 @@
> +/**
> + * Prototype for copy-over-file/s migration resources.
> + */
> +function FilesResource(aMigrator, aProfile, aStartupMigration, aType, aFileNames) {
> +  this.type = aType;

"type" is generic, fileType? migratorType? resourceType?

@@ +33,3 @@
>  
> +  if (aFileNames !== undefined) {
> +    let sourceProfileFolder = aMigrator._sourceProfileFolder;

should we rather expose a public getter?

Is there any way to reach this point with a null _sourceProfileFolder? That may happen if the profile is not readable.

@@ +39,5 @@
> +      return file;
> +    });
> +    
> +    this._files = files.every(function(f) f.exists()) ? files : null;
> +  }

I'd prefer an early return

@@ +44,4 @@
>  }
>  
> +FilesResource.prototype = {
> +  get exists() /* this._startupMigration && */ this._files != null,

??

@@ +81,5 @@
> +  // both considered "History" (the localization string mentions both).
> +  FilesResource.call(this, aMigrator, aProfile, aStartupMigration,
> +                     MigrationUtils.itemTypes.HISTORY);
> +
> +  let backups = aMigrator._sourceProfileFolder.clone();

ditto, may _sourceProfileFolder be null?

@@ +86,5 @@
> +  backups.QueryInterface(Ci.nsILocalFile);
> +  backups.appendRelativePath(PlacesUtils.backups.profileRelativeFolderPath);
> +  this._files = backups.exists() ? [backups] : null;
> +}
> +BookmarkBackups.prototype = FilesResource.prototype;

Any reason to not just put everything into the Places migrator and make a unique array of files? just for keeping things separated?

@@ +115,5 @@
> +
> +Object.defineProperties(FirefoxProfileMigrator.prototype, {
> +  getSourceExists: {
> +    value: function(aDoingStartup)
> +      /*aDoingStartup && */ this._sourceProfileFolder != null,

??

::: browser/components/migration/src/Makefile.in
@@ +73,5 @@
>  	$(NULL)
>  
> +EXTRA_JS_MODULES = \
> +  MigrationUtils.jsm \
> +	$(NULL)

some indentation issue here

::: browser/components/migration/src/ProfileMigrator.js
@@ +19,2 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/FileUtils.jsm");

I suppose you either lazyget or get them, not both. Not sure if lazygetting Services.jsm is really useful, it's simple and likely used by a bunch of components already (similarly to XPCOMUtils)

@@ +30,2 @@
>      let [key, migrator] = this._getDefaultMigrator();
> +        Cu.reportError("key: " + key);

debug to be removed

::: browser/components/migration/src/nsIEProfileMigrator.cpp
@@ +428,1 @@
>    NOTIFY_OBSERVERS(MIGRATION_STARTED, nsnull);

The doStartup change here doesn't seem to be part of the scope of this bug, should likely be moved to a separate patch in bug 718586, but not be done here.

	Asaf Romano (gone) Assignee
	Comment 5 • 13 years ago

(In reply to Marco Bonardo [:mak] from comment #4)

> I'm mostly worried by the number of changes to the Chrome Migrator, we must
> ensure to not regress its functionality, that without tests is scary.

FWIW, I'm testing this a lot. And it's better to do it now when the migrator is so small (no preferences support etc).
 
> ::: browser/components/migration/public/nsIBrowserProfileMigrator.idl
> @@ +78,4 @@
> >     * browser (i.e. whether or not it is installed, and there exists
> >     * a user profile)
> >     */
> > +  boolean getSourceExists(in boolean aDoingStartup);
> 
> To be a bit more compatible, but mostly to avoid blind boolean arguments,
> what about adding a separate sourceExistsOnStartup or similar, instead?
> 

Given that it's trunk material, and given that the next step after this bug is to remove the interface altogether, I don't think this is necessary. In fact, I'm pretty sure no one else uses this interface.


> 
> @@ +42,5 @@
> > +
> > +  this._migrator = aMigrator;
> > +  this._startupMigration = aStartupMigration;
> > +
> > +  let file = aMigrator._chromeUserDataFolder.clone();
> 
> we may expose a getter for the source folder... it's the same in the FF
> migrator isn't it?

Well, getter or field, It's "private" in the sense that it's not part of the api.

> 
> @@ +150,5 @@
> > +
> > +// TODO: This will become a true migration source in bug 718189
> > +function HomePage(aMigrator, aProfile, aStartupMigration) {
> > +  FileResource.call(this, aMigrator, aProfile, aStartupMigration,
> > +                    /* MigrationUtils.itemTypes.HOMEPAGE */ -1, "Preferences");
> 
> may we have a special itemTypes property for -1, like UNSUPPORTED or similar.
> Btw, I can't find where this magic -1 is handled, if it's handled (that the
> comment makes me think is not).

No special handling, I'm just not passing this migrator to generateMigrator.



> 
> @@ +168,5 @@
> > +  this._dbConn = Services.storage.openUnsharedDatabase(this._file);
> > +  this._success = true;
> > +  this._callback = aCallback;
> > +  let stmt = this._dbConn.createAsyncStatement(this._SQLStatement);
> > +  stmt.executeAsync(this);
> 
> these 2 may throw, better catch, callback in such a case and
> finalize()+asyncClose() in a finally

The caller of migrate takes care of catching such exceptions (I documented that in the module).

> Indeed you may asyncClose here instead of waiting completion.

Hrm?


> 
> @@ +297,4 @@
> >    },
> >  
> > +  sourceProfiles: {
> > +    get: function() {
> 
> is there a reason to define getters like this?
> 

What's the alternative? __defineGetter__ is deprecated, and defineProperty uses the same verbose syntax.



> 
> @@ +33,3 @@
> >  
> > +  if (aFileNames !== undefined) {
> > +    let sourceProfileFolder = aMigrator._sourceProfileFolder;
> 
> should we rather expose a public getter?
> 

Given that the IDL is soon going away, exposing a public getter will make it impossible to 
distinguish this kind of public from "to be used by users of this component". Too bad there's no "protected" prefix...

> Is there any way to reach this point with a null _sourceProfileFolder? That
> may happen if the profile is not readable.
> 

That will makes it throw early, and that's handled in the module, unless I'm missing something.


> 
> @@ +86,5 @@
> > +  backups.QueryInterface(Ci.nsILocalFile);
> > +  backups.appendRelativePath(PlacesUtils.backups.profileRelativeFolderPath);
> > +  this._files = backups.exists() ? [backups] : null;
> > +}
> > +BookmarkBackups.prototype = FilesResource.prototype;
> 
> Any reason to not just put everything into the Places migrator and make a
> unique array of files? just for keeping things separated?

The idea is that even if only one of the resources exists, we still support import for that migration-type (unlike passwords, which uses one resource for two files, because both are needed for working-import).

> > +Object.defineProperties(FirefoxProfileMigrator.prototype, {
> > +  getSourceExists: {
> > +    value: function(aDoingStartup)
> > +      /*aDoingStartup && */ this._sourceProfileFolder != null,
> 
> ??

Debugging material.
 
> The doStartup change here doesn't seem to be part of the scope of this bug,
> should likely be moved to a separate patch in bug 718586, but not be done
> here.


Indeed.

	Marco Bonardo [:mak]
	Comment 6 • 13 years ago

(In reply to Mano from comment #5)
> (In reply to Marco Bonardo [:mak] from comment #4)
> 
> > I'm mostly worried by the number of changes to the Chrome Migrator, we must
> > ensure to not regress its functionality, that without tests is scary.
> 
> FWIW, I'm testing this a lot. And it's better to do it now when the migrator
> is so small (no preferences support etc).

Ok, we should setup some time with a person in the QA team to plan testing of the migrators. Please keep up with good testing, sorry if this has to be manual for now :(

> > ::: browser/components/migration/public/nsIBrowserProfileMigrator.idl
> > @@ +78,4 @@
> > >     * browser (i.e. whether or not it is installed, and there exists
> > >     * a user profile)
> > >     */
> > > +  boolean getSourceExists(in boolean aDoingStartup);
> > 
> > To be a bit more compatible, but mostly to avoid blind boolean arguments,
> > what about adding a separate sourceExistsOnStartup or similar, instead?
> > 
> 
> Given that it's trunk material, and given that the next step after this bug
> is to remove the interface altogether, I don't think this is necessary. In
> fact, I'm pretty sure no one else uses this interface.

Sure, still blind boolean arguments are not so good in js world too :(

> > Is there any way to reach this point with a null _sourceProfileFolder? That
> > may happen if the profile is not readable.
> > 
> 
> That will makes it throw early, and that's handled in the module, unless I'm
> missing something.

I couldn't find that check, unless you mean it's handled by catching any flying around exceptions?

Btw, just after dinner I'll complete this review.

	Marco Bonardo [:mak]
	Comment 7 • 13 years ago

Comment on attachment 589480 [details] [diff] [review]
ready for first pass

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

Enough for now, I'll finally review the next iteration, globally looks like a win, maybe it's just a bit harder to begin with but the documentation is ok and there are examples.

::: browser/components/migration/public/nsIBrowserProfileMigrator.idl
@@ +78,4 @@
>     * browser (i.e. whether or not it is installed, and there exists
>     * a user profile)
>     */
> +  boolean getSourceExists(in boolean aDoingStartup);

To be a bit more compatible, but mostly to avoid blind boolean arguments, what about adding a separate sourceExistsOnStartup or similar, instead?

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +12,4 @@
>  
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

likely may avoid lazygetter on services

@@ +42,5 @@
> +
> +  this._migrator = aMigrator;
> +  this._startupMigration = aStartupMigration;
> +
> +  let file = aMigrator._chromeUserDataFolder.clone();

we may expose a getter for the source folder... it's the same in the FF migrator isn't it?

@@ +150,5 @@
> +
> +// TODO: This will become a true migration source in bug 718189
> +function HomePage(aMigrator, aProfile, aStartupMigration) {
> +  FileResource.call(this, aMigrator, aProfile, aStartupMigration,
> +                    /* MigrationUtils.itemTypes.HOMEPAGE */ -1, "Preferences");

may we have a special itemTypes property for -1, like UNSUPPORTED or similar.
Btw, I can't find where this magic -1 is handled, if it's handled (that the comment makes me think is not).
This is sort of confusing, maybe better to just let it alone as it was till we can make it a proper source.

@@ +155,5 @@
> +}
> +HomePage.prototype = Object.create(FileResource.prototype);
> +
> +// Prototype for History & Cookies resources, which are stored in
> +// a sql3lite data base.

typo: sql3lite, should be SQLite

@@ +165,5 @@
> +StorageResource.prototype = Object.create(FileResource.prototype);
> +
> +StorageResource.prototype.migrate = function(aCallback) {
> +  this._dbConn = Services.storage.openUnsharedDatabase(this._file);
> +  this._success = true;

init to false and set in handleCompletion to aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED

@@ +168,5 @@
> +  this._dbConn = Services.storage.openUnsharedDatabase(this._file);
> +  this._success = true;
> +  this._callback = aCallback;
> +  let stmt = this._dbConn.createAsyncStatement(this._SQLStatement);
> +  stmt.executeAsync(this);

these 2 may throw, better catch, callback in such a case and finalize()+asyncClose() in a finally
Indeed you may asyncClose here instead of waiting completion.

@@ +190,5 @@
> +function History(aMigrator, aProfile, aStartupMigration) {
> +  StorageResource.call(this, aMigrator, aProfile, aStartupMigration,
> +                       MigrationUtils.itemTypes.HISTORY, "History",
> +                       "SELECT url, title, last_visit_time, typed_count FROM \
> +                        urls WHERE hidden = 0");

I think this generates queries with lots of whitespace in the Storage log, please just use the classic +

@@ +237,5 @@
> +function Cookies(aMigrator, aProfile, aStartupMigration) {
> +  StorageResource.call(this, aMigrator, aProfile, aStartupMigration,
> +                       MigrationUtils.itemTypes.COOKIES, "Cookies",
> +                       "SELECT host_key, path, name, value, secure,httponly, \
> +                        expires_utc FROM cookies");

ditto

@@ +297,4 @@
>    },
>  
> +  sourceProfiles: {
> +    get: function() {

is there a reason to define getters like this?

::: browser/components/migration/src/FirefoxProfileMigrator.js
@@ +25,4 @@
>  
> +/**
> + * Prototype for copy-over-file/s migration resources.
> + */

more documentation please, the params especially

@@ +26,5 @@
> +/**
> + * Prototype for copy-over-file/s migration resources.
> + */
> +function FilesResource(aMigrator, aProfile, aStartupMigration, aType, aFileNames) {
> +  this.type = aType;

"type" is generic, fileType? migratorType? resourceType?

@@ +33,3 @@
>  
> +  if (aFileNames !== undefined) {
> +    let sourceProfileFolder = aMigrator._sourceProfileFolder;

should we rather expose a public getter?

Is there any way to reach this point with a null _sourceProfileFolder? That may happen if the profile is not readable.

@@ +39,5 @@
> +      return file;
> +    });
> +    
> +    this._files = files.every(function(f) f.exists()) ? files : null;
> +  }

I'd prefer an early return

@@ +44,4 @@
>  }
>  
> +FilesResource.prototype = {
> +  get exists() /* this._startupMigration && */ this._files != null,

??

@@ +81,5 @@
> +  // both considered "History" (the localization string mentions both).
> +  FilesResource.call(this, aMigrator, aProfile, aStartupMigration,
> +                     MigrationUtils.itemTypes.HISTORY);
> +
> +  let backups = aMigrator._sourceProfileFolder.clone();

ditto, may _sourceProfileFolder be null?

@@ +86,5 @@
> +  backups.QueryInterface(Ci.nsILocalFile);
> +  backups.appendRelativePath(PlacesUtils.backups.profileRelativeFolderPath);
> +  this._files = backups.exists() ? [backups] : null;
> +}
> +BookmarkBackups.prototype = FilesResource.prototype;

Any reason to not just put everything into the Places migrator and make a unique array of files? just for keeping things separated?

@@ +115,5 @@
> +
> +Object.defineProperties(FirefoxProfileMigrator.prototype, {
> +  getSourceExists: {
> +    value: function(aDoingStartup)
> +      /*aDoingStartup && */ this._sourceProfileFolder != null,

??

::: browser/components/migration/src/Makefile.in
@@ +73,5 @@
>  	$(NULL)
>  
> +EXTRA_JS_MODULES = \
> +  MigrationUtils.jsm \
> +	$(NULL)

some indentation issue here

::: browser/components/migration/src/MigrationUtils.jsm
@@ +9,5 @@
> +let EXPORTED_SYMBOLS = ["MigrationUtils"];
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

Probably not worth laziness for Services, it's so widely used

@@ +27,5 @@
> +   * single-profile case.  They must be overridden for migrators which
> +   * support multiple profiles.
> +   *
> +   * getSourceExists and sourceHomePageURL are not implemented, and must be
> +   * overridden.

maybe you could make this documentation more idl-like, or just 
interface {
 method: what to do
 method: what to do
}
I would mostly like the method names to be implemented to be more prominent, so it's clearer what's the non-interface the resource has to implement.
Something that one may copy/paste to begin with with be the perfection, but whatever.

@@ +32,5 @@
> +   *
> +   * @param aResourcesConstructors
> +   *        Array of constructor functions for "migration resources" objects.
> +   *        Each of those should be dedicated to one migration type.  However,
> +   *        it is possible to provide multiple migration resoucres for a single

typo: resoucres

@@ +44,5 @@
> +   *        - aProfile is the source-profile for with which migration is
> +   *          associated.  Valid values are any value included in the
> +   *          "sourceProfiles" array property of the migrator.
> +   *          Migrators which don't support multiple profiles should ignore this
> +   *          parameter.

what does "ignore" mean, should I pass null, undefined, -1, "please-ignore-me"?
Would be worth it to move this at the end and make it optional? At least "Migrators who don't support multiple profiles may omit this parameter" would be clearer.

@@ +47,5 @@
> +   *          Migrators which don't support multiple profiles should ignore this
> +   *          parameter.
> +   *        - aStartupMigration - boolean indicating whether the current
> +   *          migration procedure run automatically during startup.
> +   *        If a construstor throws, the resource is treated as non-existent.

typo: construstor
Btw, I'd move this note above, after "if
* *any* of the migration resources exists.

@@ +60,5 @@
> +   *          when migration is done.  The callback should be called with one
> +   *          boolean parameter, |aSuccess|, indicating whether the migrartion
> +   *          succeeded.  In the case of an exception synchronously thrown from
> +   *          |migrate|, it'll be taken as if |aCallback(false)| was called,
> +   *          even if it wasn't.

as above would like the non-interface to be implemented more prominent.

@@ +86,5 @@
> +      });
> +  },
> +
> +  createImportedBookmarksFolder:
> +  function MU_createImportedBookmarksFolder(aSourceNameStr, aParentId) {

moar documentation please (params, what this does and when should be used). I suppose places where this may be used don't care if it throws.

@@ +88,5 @@
> +
> +  createImportedBookmarksFolder:
> +  function MU_createImportedBookmarksFolder(aSourceNameStr, aParentId) {
> +    let bundle = Services.strings.createBundle(
> +     "chrome://browser/locale/migration/migration.properties");

please move this to a MIGRATION_BUNDLE const at the top and oneline this, if possible.

@@ +98,5 @@
> +  },
> +
> +  getDirectoryIfExists: function MU_getDirectoryIfExists(aKey, aPathArray) {
> +    let dir = FileUtils.getDir(aKey, aPathArray, null);
> +    return dir.exists() ? dir : null;

may be worth to upstream this to FileUtils as getDirIfExists?

@@ +101,5 @@
> +    let dir = FileUtils.getDir(aKey, aPathArray, null);
> +    return dir.exists() ? dir : null;
> +  },
> +
> +  itemTypes: {

the word "item" is a bit abused across the codebase... maybe resourceTypes

@@ +113,5 @@
> +  },
> +
> +  // TODO: remove currentProfileFolder in bug 718586.
> +  _currentProfileFolder: null,
> +  get currentProfileFolder() {

more documentation on what "current" means.

@@ +121,5 @@
> +    return this._currentProfileFolder = FileUtils.getDir("ProfD", []);
> +  },
> +
> +  // "ProfD" isn't available in the case of startup migration, thus
> +  // ProfileMigrator calls this setter with nsIProfileStratup::directory.

typo: stratup

@@ +123,5 @@
> +
> +  // "ProfD" isn't available in the case of startup migration, thus
> +  // ProfileMigrator calls this setter with nsIProfileStratup::directory.
> +  set currentProfileFolder(val)
> +    this._currentProfileFolder = val

I think in our codebase setters usually return the set value

Btw, if this is invoked currentProfileFolder will never reach the ProfD code path. So, it this set (And should be called) only in case of startup migration? may you point out this requirement better in the comment?

@@ +151,5 @@
> +    }
> +    else {
> +      aItems = [t for each (t in MigrationUtils.itemTypes)
> +                if ((aItems & t) != 0)];
> +    }

I count 7 lines!

@@ +155,5 @@
> +    }
> +
> +    // This cannot actually happen before the bit field is removed.
> +    if (aItems.length == 0)
> +      throw "migrate must be called for at least one item";

new Error, pls

@@ +167,5 @@
> +    let itemsDone = 0;
> +    aItems.forEach(function(type) {
> +      let onItemDone = function(aSuccess) {
> +        Services.obs.notifyObservers(null,
> +          aSuccess ? "Migration:ItemAfterMigrate" : "Migration:ItemError",

move this to a temp notificationMsg (or as you prefer) var and oneline the notify

@@ +175,5 @@
> +      };
> +
> +      let itemResources = [r for each (r in resources) if (r.type == type)];
> +      if (itemResources.length != 0) {
> +        Services.obs.notifyObservers(null, "Migration:ItemBeforeMigrate",

nit: I'd like these notification message strings as consts at the top

@@ +220,5 @@
> +  },
> +
> +  /**
> +   * Gets all existent migration resources for the given profile+migration-mode
> +   * combination.

I initially read that as profile + migration - mode, maybe "for the given (profile + migration-mode)" would be better.

@note that this ignores throwing constructors (iirc you already said above, but better being redundant near the actual method)

@@ +250,5 @@
> +    resourcesTypes.forEach(function(t) {
> +      if (uniqueItemTypes.indexOf(t) == -1)
> +        uniqueItemTypes.push(t);
> +    });
> +    return uniqueItemTypes;

you could you use filter() here, I think. Something like return resourcesTypes.filter(function(e, i) resourcesTypes.indexOf(e) == i);

::: browser/components/migration/src/ProfileMigrator.js
@@ +19,2 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/FileUtils.jsm");

I suppose you either lazyget or get them, not both. Not sure if lazygetting Services.jsm is really useful, it's simple and likely used by a bunch of components already (similarly to XPCOMUtils)

@@ +30,2 @@
>      let [key, migrator] = this._getDefaultMigrator();
> +        Cu.reportError("key: " + key);

debug to be removed

::: browser/components/migration/src/nsIEProfileMigrator.cpp
@@ +428,1 @@
>    NOTIFY_OBSERVERS(MIGRATION_STARTED, nsnull);

The doStartup change here doesn't seem to be part of the scope of this bug, should likely be moved to a separate patch in bug 718586, but not be done here.

	Marco Bonardo [:mak]
	Comment 8 • 13 years ago

Ehr looks like splinter may have coalesced my previous review with the new one :( sorry, I guess you'll have to go through some comments again. sigh.

	Matthew N. [:MattN]
	Comment 9 • 13 years ago

Mano, do you have an ETA on getting this up for review again?  I'm making some changes to Migration/Firefox migrator code in bug 717070 which I'd like to get in Firefox 12 so I don't want to rebase on top of this patch if it make it in time.

	Asaf Romano (gone) Assignee
	Comment 10 • 13 years ago

About two days or so, with quite radical changes. If this feature is indeed happening for 12, you should ignore this patch.

However, as I noted on your bug, I've some reservation on the implementation. With regard to this bug (and bug 718586), you should not rely on doStartup.

	Marco Bonardo [:mak]
	Comment 11 • 13 years ago

Mano, ping? This blocks a bunch of stuff :(

	Asaf Romano (gone) Assignee
	Comment 12 • 13 years ago

Patch ready for your review expected /late/ this Friday.

	Asaf Romano (gone) Assignee
	Comment 13 • 13 years ago

(Slight delay here. Will update tomorrow)

	Matthew N. [:MattN]
	Comment 14 • 13 years ago

Hey Mano,

How is the revised patch coming along?  There are some follow-ups for migrators I would like to work on but I'm waiting on this cleanup to start.  Do you have a revised ETA?

	Asaf Romano (gone) Assignee
	Comment 15 • 13 years ago

Hey Matthew. Sorry for the extra delays here. Short answer, this week. I'm also splitting the patch so the critical parts can land asap.  What part of the original patch here are of interest for you (In particular, do you depend on the changes to the Fx-migrator)?

	Matthew N. [:MattN]
	Comment 16 • 13 years ago

In comment 10 you mentioned radical changes so I'm not sure what the extent of the subsequent changes are.  Some of the changes I'd like to make are for the Firefox migrator while some are for the overall migration infrastructure. Some of the bugs are bug 717070, bug 705927, & bug 715315. I also want to look at possibly exposing migration errors in the migration UI rather than silently ignoring them.

	Asaf Romano (gone) Assignee
	Comment 17 • 13 years ago

Marco: I still need to check the comments for typos, and reread them, so please don't review those yet. I'll upload another version addressing the comments later, after finishing the Safari part.

The api itself should be stable in this patch, and so are the changes to the Chrome migrator. I'll comment later explaining the (temporary) trade-offs I made here.

The Firefox migrator changes will come next.

	Marco Bonardo [:mak]
	Comment 18 • 13 years ago

Comment on attachment 601015 [details] [diff] [review]
"stable"

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

As a global thought, considered number of changes, this should probably land after we merge for 14, on 13/03.

The approach looks fine, as you said a lot of comments need cleanup, so I just did a panoramic check.

::: browser/components/migration/public/nsIBrowserProfileMigrator.idl
@@ +84,5 @@
>    /** 
>     * An enumeration of available profiles. If the import source does 
>     * not support profiles, this attribute is null.
>     */
> +  readonly attribute jsval            sourceProfiles;

you should bump the uuid (And then get SR)

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +269,5 @@
> +        },
> +
> +        handleCompletion : function(aReason) {
> +          dbConn.asyncClose();
> +          aCallback(this._success)

you should rather check aReason, instead of caching _success, see mozIStorageStatementCallback

@@ +327,5 @@
>          },
>  
>          handleCompletion : function(aReason) {
> +          dbConn.asyncClose();
> +          aCallback(this._success);

ditto on using aReason

::: browser/components/migration/src/MigrationUtils.jsm
@@ +36,5 @@
> +   *    it should override the sourceHomePageURL getter.
> +   * 7. If you don't want the profile to be initialized before migration (in the
> +   *    case of startup migration), override "shouldInitProfile"
> +   */
> +  ProfileMigratorBase: Object.seal({

why don't you expose this as a separate module symbol?

@@ +59,5 @@
> +    get sourceExists() {
> +      // A migrator 
> +      let profiles = this.sourceProfiles;
> +      return ( (!profiles && this.getResources("")) ||
> +                (profiles && profiles.length > 0) );

nit: the external wrapping parenthesis look useless

@@ +109,5 @@
> +     */
> +    shouldInitProfile: true,
> +
> +    // See nsIBrowserProfileMigrator
> +    // DO NOT OVERRIDE - After deCOMing migration, the UI will just call

you may want to groups methods/properties that should/may be overridden and things that should not, and don't mix them
you could then add some comment separator to make clear what the implementer should look and what not.

@@ +189,5 @@
> +
> +    // Override if your migrator supports importing the homepage.
> +    sourceHomePageURL: "",
> +
> +    _resourcesByProfile: { },

this should no be defined here, wouldn't Object.create use the same object for all the migrators then?
better to lazy init it from _getMaybeCachedResources

@@ +245,5 @@
> +      }
> +      catch(ex) {
> +        Cu.reportError(ex);
> +      }
> +      aCallback(success);

nit: aCallback(true) in the try and aCallback(false) in the catch would be shorter
I'm not sure I understand why this is called async, apart taking a callback that is invoked synchronously. Couldn't the delay be integrated into it? was that the original scope?

@@ +289,5 @@
> +   *
> +   * @return profile migrator implementing nsIBrowserProfileMigrator, if
> +   * it can import any data; null otherwise.
> +   */
> +  _migrators: new Map(),

may this keep the migrator alive longer than it's needed? wouldn't a WeakMap simplify final GC since the migrators are already kept alive by the service manager?

::: browser/components/places/content/places.js
@@ +39,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +Components.utils.import("resource://gre/modules/MigrationUtils.jsm");

defineLazyModuleGetter this one please

	Asaf Romano (gone) Assignee
	Comment 20 • 13 years ago

(In reply to Marco Bonardo [:mak] from comment #18)

> @@ +245,5 @@
> > +      }
> > +      catch(ex) {
> > +        Cu.reportError(ex);
> > +      }
> > +      aCallback(success);
> 
> nit: aCallback(true) in the try and aCallback(false) in the catch would be
> shorter

We would end up calling aCallback twice if aCallback threw.

> [a.r.: about asyncMigrateFunction]
> I'm not sure I understand why this is called async, apart taking a callback
> that is invoked synchronously. Couldn't the delay be integrated into it? was
> that the original scope?

The naming is bad. I renamed to wrapMigrateFunction, which isn't great either. I'm open to suggestions.

The delay could not be integrated, of course, because it could be any kind of delay (asyncFetch, read-plist, etc).

> @@ +289,5 @@
> > +   *
> > +   * @return profile migrator implementing nsIBrowserProfileMigrator, if
> > +   * it can import any data; null otherwise.
> > +   */
> > +  _migrators: new Map(),
> 
> may this keep the migrator alive longer than it's needed? wouldn't a WeakMap
> simplify final GC since the migrators are already kept alive by the service
> manager?

I don't think this could leak.  If it could, Services.jsm would leak much ;)
Anyway, a weakmap here doesn't make sense in general, we don't want the migrator to go away (once we decom...) when the last reference for it goes away.

> ::: browser/components/places/content/places.js
> @@ +39,5 @@
> >   * the terms of any one of the MPL, the GPL or the LGPL.
> >   *
> >   * ***** END LICENSE BLOCK ***** */
> >  
> > +Components.utils.import("resource://gre/modules/MigrationUtils.jsm");
> 
> defineLazyModuleGetter this one please

Missed that. Will do in the next iteration.

	Marco Bonardo [:mak]
	Comment 21 • 13 years ago

Comment on attachment 607033 [details] [diff] [review]
stable (quotations removed)

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

(In reply to Mano from comment #20)
> Anyway, a weakmap here doesn't make sense in general, we don't want the
> migrator to go away (once we decom...) when the last reference for it goes
> away.

If not that you define migrators as singleton and getService them, so they are kept alive regardless. I was thinking we should throw away migrators once we are finished importing, if so we can't make them singleton, we seem to usually createInstance them in current code. Is there a reason to make them services?

Apart this, would like to do a basic testing on win7 of the next version, before r+, just as a sanity check.

::: browser/components/migration/content/migration.js
@@ +8,5 @@
> +
> +const kIMig = Ci.nsIBrowserProfileMigrator;
> +const kIPStartup = Ci.nsIProfileStartup;
> +
> +Cu.import("resource://gre/modules/MigrationUtils.jsm");

just a note to remember defineLazyModuleGetter

@@ +155,5 @@
>        else
>          this._wiz.currentPage.next = "importItems";
>  
>        var sourceProfiles = this._migrator.sourceProfiles;
> +      if (sourceProfiles && sourceProfiles.length == 1)

could cache this._migrator.sourceProfiles much earlier, before the first if

::: browser/components/migration/src/MigrationUtils.jsm
@@ +12,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/PlacesUtils.jsm");
> +Cu.import("resource://gre/modules/Dict.jsm");

lazyload PlacesUtils, may not be needed

@@ +19,5 @@
> + * Shared prototype for migrators, implementing nsIBrowserProfileMigrator.
> + *
> + * To implement a migrator:
> + * 1. Import this module.
> + * 2. Create the prototype for the migrator, extending ProfileMigratorBase.

no more ProfileMigratorBase

@@ +22,5 @@
> + * 1. Import this module.
> + * 2. Create the prototype for the migrator, extending ProfileMigratorBase.
> + *    Namely: MosaicMigrator.prototype = Object.create(MigratorPrototype);
> + * 3. Set classDescription, contractID and classID for your migrator.  Also set
> + *    _xpcom_factory by XPCOMUtils.generateSingletonFactory.

see above for singleton and lifecycle

@@ +25,5 @@
> + * 3. Set classDescription, contractID and classID for your migrator.  Also set
> + *    _xpcom_factory by XPCOMUtils.generateSingletonFactory.
> + * 4. If the migrator supports multiple profiles, override the sourceProfiles
> + *    Here we default for single-profile migrator.
> + * 5. Implement getResources(aProfile), as described inits header here.

unclear phrase

@@ +29,5 @@
> + * 5. Implement getResources(aProfile), as described inits header here.
> + * 6. If the migrator supports reading the home page of the source browser,
> + *    override |sourceHomePageURL| getter.
> + * 7. For startup-only migrators (that's just the Firefox migrator, for now),
> + *    override |startupOnlyMigrator|.

then shouldn't you do this in the firefox migrator?

@@ +35,5 @@
> +let MigratorPrototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIBrowserProfileMigrator]),
> +
> +  /**
> +   * OVERRIDE IFF the source supports multiple profiles.

typo: IFF

@@ +39,5 @@
> +   * OVERRIDE IFF the source supports multiple profiles.
> +   *
> +   * Returns array of profiles (by names) from which data may be imported.
> +   *
> +   * Only profiles from which data can be imported should be listed.  Otherwsie

typo: otherwsie

@@ +70,5 @@
> +   *
> +   * For each migration type listed in nsIBrowserProfileMigrator, multiple
> +   * migration resources may be provided.  This practice is useful when the
> +   * data for a certain migration type is independently stored in few
> +   * locations.  For example, the mac version of Safai stores its "reading list"

typo: safai

@@ +82,5 @@
> +   *
> +   * @note  The returned array should only include resources from which data
> +   * can be imported.  So, for example, before adding a resource for the
> +   * BOOKMARKS migration type, you should check if you should check that the
> +   * bookmarks file exists.

better indent here, as other javadoc properties

@@ +106,5 @@
> +   *   but it's only accessible through MigrationUtils.profileStartup.
> +   *   The migrator can call MigrationUtils.profileStartup.doStartup
> +   *   at any point in order to initialize the profile.
> +   */
> +  startupOnlyMigrator: false,

this would be better as a readonly getter

@@ +194,5 @@
> +    });
> +  },
> +
> +  /**
> +   * OVERRIDE IFF your migrator supports importing the homepage.

typo IFF

@@ +197,5 @@
> +  /**
> +   * OVERRIDE IFF your migrator supports importing the homepage.
> +   * @see nsIBrowserProfileMigrator
> +   */
> +  get sourceHomePageURL() "",

move this above getMigrateData, so all stuff that may be overridden is near, and all the stuff that should not be is too.

@@ +253,5 @@
> +   * You may write:
> +   * setTimeout(0, MigrationUtils.asyncMigrateFunction(function() {
> +   *   if (importingFromMosaic)
> +   *     throw Cr.NS_ERROR_UNEXPECTED;
> +   * }, aCallback), 0);

the setTimeout call seems bogus (in both examples), what is the first 0 argument?

@@ +289,5 @@
> +   * migration source.  The folder is created at the end of the given folder.
> +   *
> +   * @param aSourceNameStr
> +   *        the source name (first letter capitalized).  This is used
> +   *        to for reading the localized source name from the migration

typo: to for

@@ +323,5 @@
> +   * or mosaic everywhere).  This method should be used rather than direct
> +   * getService for future compatibility (see bug 718280).
> +   *
> +   * @return profile migrator implementing nsIBrowserProfileMigrator, if
> +   * it can import any data; null otherwise.

better indent, s/;/,/

@@ +346,5 @@
> +    return migrator && migrator.sourceExists ? migrator : null;
> +  },
> +
> +  // Whether or not we're in the process of startup migration
> +  get startupMigration() this._profileStartup != null,

either rename to isStartupMigration, or get rid of it and just check this.profileStartup

@@ +378,5 @@
> +   * @param [optional] aSkipImportSourcePage
> +   *        Whether or not to skip the migration-source selection page in the
> +   *        wizard (ignored if aProfileStartup is not set).  Default: false.
> +   *
> +   * @thrwos if aKey is set to an invalid or non-existent migration source.

typo thrwos

@@ +384,5 @@
> +   *       on this platform.
> +   */
> +  startMigration:
> +  function MU_startMigration(aOpener, aProfileStartup, aMigratorKey,
> +                             aSkipImportSourcePage) {

basically, this always ends up opening the migration UI, I feel like the name is not good enough.
Indicating either MigrationUI or MigrationWizard in the name would be better imo.  And with either show or open prefix. Thoughts?

::: browser/components/migration/src/ProfileMigrator.js
@@ +18,5 @@
>  ProfileMigrator.prototype = {
>    migrate: function PM_migrate(aStartup, aKey) {
> +    let key = aKey;
> +    let skipSourcePage = false;
> +    if (key != "") {

may key.length > 0 be faster?

@@ +19,5 @@
>    migrate: function PM_migrate(aStartup, aKey) {
> +    let key = aKey;
> +    let skipSourcePage = false;
> +    if (key != "") {
> +      if (!qMigrationUtils.getMigrator(aKey)) {

hm, what is qMigrationUtils? typo?

::: browser/components/places/content/places.js
@@ +39,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +Components.utils.import("resource://gre/modules/MigrationUtils.jsm");

and please remember to lazyload this as well

	Asaf Romano (gone) Assignee
	Comment 22 • 13 years ago

Please ignore the "extra" changes to the makefile. I'm working on the Safari migrator within the same tree.

	Asaf Romano (gone) Assignee
	Comment 23 • 13 years ago

Comment on attachment 607511 [details] [diff] [review]
comments addressed

Oooops.

	Asaf Romano (gone) Assignee
	Comment 25 • 13 years ago

(In reply to Marco Bonardo [:mak] from comment #21)

> (In reply to Mano from comment #20)
> > Anyway, a weakmap here doesn't make sense in general, we don't want the
> > migrator to go away (once we decom...) when the last reference for it goes
> > away.
> 
> If not that you define migrators as singleton and getService them, so they
> are kept alive regardless. I was thinking we should throw away migrators
> once we are finished importing, if so we can't make them singleton, we seem
> to usually createInstance them in current code. Is there a reason to make
> them services?
> 

This is solved as discussed over irc (switched back to createInstance, strong-referenced in the module, cleaned up when migration is done).
 
> ::: browser/components/migration/content/migration.js
> @@ +8,5 @@
> > +
> > +const kIMig = Ci.nsIBrowserProfileMigrator;
> > +const kIPStartup = Ci.nsIProfileStartup;
> > +
> > +Cu.import("resource://gre/modules/MigrationUtils.jsm");
> 
> just a note to remember defineLazyModuleGetter

I didn't fix that after all, because there's just one module here, so it's not worth importing xpcomutils. ditto for places.js
 
> @@ +155,5 @@
> >        else
> >          this._wiz.currentPage.next = "importItems";
> >  
> >        var sourceProfiles = this._migrator.sourceProfiles;
> > +      if (sourceProfiles && sourceProfiles.length == 1)
> 
> could cache this._migrator.sourceProfiles much earlier, before the first if
> 

Done.

> ::: browser/components/migration/src/MigrationUtils.jsm
> @@ +12,5 @@
> > +
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/PlacesUtils.jsm");
> > +Cu.import("resource://gre/modules/Dict.jsm");
> 
> lazyload PlacesUtils, may not be needed

Done, also for Dict.jsm.


> 
> @@ +29,5 @@
> > + * 5. Implement getResources(aProfile), as described inits header here.
> > + * 6. If the migrator supports reading the home page of the source browser,
> > + *    override |sourceHomePageURL| getter.
> > + * 7. For startup-only migrators (that's just the Firefox migrator, for now),
> > + *    override |startupOnlyMigrator|.
> 
> then shouldn't you do this in the firefox migrator?

I've removed this comment, and referenced bug 737381 in the header for startupOnlyMigrator.


> IFF

I've all instances of that to IF AND ONLY IF.


> > +   *   but it's only accessible through MigrationUtils.profileStartup.
> > +   *   The migrator can call MigrationUtils.profileStartup.doStartup
> > +   *   at any point in order to initialize the profile.
> > +   */
> > +  startupOnlyMigrator: false,
> 
> this would be better as a readonly getter

Done, but note it makes overriding a little bit more painful.
 

> @@ +253,5 @@
> > +   * You may write:
> > +   * setTimeout(0, MigrationUtils.asyncMigrateFunction(function() {
> > +   *   if (importingFromMosaic)
> > +   *     throw Cr.NS_ERROR_UNEXPECTED;
> > +   * }, aCallback), 0);
> 
> the setTimeout call seems bogus (in both examples), what is the first 0
> argument?

Yup.

> either rename to isStartupMigration, or get rid of it and just check
> this.profileStartup

renamed.

> basically, this always ends up opening the migration UI, I feel like the
> name is not good enough.
> Indicating either MigrationUI or MigrationWizard in the name would be better
> imo.  And with either show or open prefix. Thoughts?

showMigrationWizard

> >    migrate: function PM_migrate(aStartup, aKey) {
> > +    let key = aKey;
> > +    let skipSourcePage = false;
> > +    if (key != "") {
> 
> may key.length > 0 be faster?

Done, but I really hope spidermonkey optimizes this anyway...

	Marco Bonardo [:mak]
	Comment 26 • 13 years ago

Comment on attachment 607513 [details] [diff] [review]
comments addressed

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

may you give me a final version to test, with the Makefile.in stuff fixed?

::: browser/components/migration/src/Makefile.in
@@ +58,5 @@
>  EXTRA_PP_COMPONENTS = \
>    ProfileMigrator.js \
>    ChromeProfileMigrator.js \
>    FirefoxProfileMigrator.js \
> +  SafariMigrator.js \

just remember to undo this stuff :)

::: browser/components/migration/src/MigrationUtils.jsm
@@ +40,5 @@
> +let MigratorPrototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIBrowserProfileMigrator]),
> +
> +  /**
> +   * OVERRIDE OVERRIDE IF AND ONLY IF the source supports multiple profiles.

typo override override

::: browser/components/migration/src/ProfileMigrator.js
@@ +19,5 @@
>    migrate: function PM_migrate(aStartup, aKey) {
> +    let key = aKey;
> +    let skipSourcePage = false;
> +    if (key.length > 0) {
> +      if (!MigrationUtils.getMigrator(aKey)) {

nit: use key, rather then aKey, for coherence

	Marco Bonardo [:mak]
	Comment 27 • 13 years ago

we likely will need these changes before bug 482911, since that bug has an hard time finding a shared place to import default bookmarks skipping the firefox migrator, this code adds some better hook point.

	Marco Bonardo [:mak]
	Comment 29 • 13 years ago

We may have a problem, Windows won't compile browsercomps.dll cause you are trying to use jsapi but this library doesn't include libjs and the linker can't see JSVAL_NULL.  On other platforms may work fine for build order, though I had similar issues recently and looks like old msvc 2005 (that we use on tinderboxes) is even worse.  So probably we have to revert the jsval change to the old nsIArray, until we finish deCOM.

	Marco Bonardo [:mak]
	Comment 30 • 13 years ago

Benjamin, any idea how we may fix the linking process to have msvc see jsapi from browsercomps.dll?
Right now we get
nsIEProfileMigrator.obj : error LNK2001: unresolved external symbol __imp__JSVAL_NULL
Though Mac seems to work fine.

	Benjamin Smedberg
	Comment 31 • 13 years ago

On non-Windows mozjs is folded into libxul. On Windows it is a separate library. You want something like this:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/mac/Makefile.in#73

I'm assuming that we're still dllexporting the JS symbols. If we're not, then you just can't do this.

	Marco Bonardo [:mak]
	Comment 32 • 13 years ago

Yes, I verified adding $(MOZ_JS_LIBS) to EXTRA_DSO_LDOPTS solves the linkage. Thank you!

	Marco Bonardo [:mak]
	Comment 33 • 13 years ago

this seems to be enough for our needs, does it look correct?

	Marco Bonardo [:mak]
	Comment 34 • 13 years ago

Comment on attachment 607663 [details] [diff] [review]
final?

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

Just in case a tryrun may come at hand to check for unexpected failures.
You still need the build patch to make it properly build on Win and a SR on the idl changes.
I briefly discusses with Juan to setup a special QA session for migrators, I just have to collect all the bugs with their ETA and send them a mail to plan it, so we may get some better coverage.

::: browser/components/migration/src/MigrationUtils.jsm
@@ +102,5 @@
> +  },
> +
> +  /**
> +   * Whether or not the migrator is a startup-only migrator (For now, that is
> +   * just the Firefox migrator, see bug 737381).  Default: false.

nit: may want an "OVERRIDE IF AND ONLY IF" like the others

@@ +235,5 @@
> +    return this._resourcesByProfile[aProfile] = this.getResources(aProfile);
> +  }
> +};
> +
> +let MigrationUtils = Object.seal({

Remember to freeze

@@ +255,5 @@
> +   *   }
> +   * }, 0);
> +   *
> +   * You may write:
> +   * setTimeout(MigrationUtils.asyncMigrateFunction(function() {

needs name update

	Mozilla RelEng Bot
	Comment 37 • 13 years ago

Autoland Patchset:
	Patches: 607880
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=2636e7565bad
Try run started, revision 2636e7565bad. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=2636e7565bad

	Mozilla RelEng Bot
	Comment 38 • 13 years ago

Try run for 2636e7565bad is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2636e7565bad
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-2636e7565bad

	Asaf Romano (gone) Assignee
	Comment 39 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/9023803c5d64