706733 - Chrome migrator shouldn't duplicate type constants

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Matthew N. [:MattN]]

Attachment: Use constants defined on nsIBrowserProfileMigrator rather than redefining them

Constants to indicate the data types supported for migration were redefined in ChromeProfileMigrator.js.  These should be replaced with a reference to the original const's on the nsIBrowserProfileMigrator interface.

::: browser/components/migration/src/ChromeProfileMigrator.js
+const MIGRATE_ALL = 0x0000;
+const MIGRATE_SETTINGS = 0x0001;
+const MIGRATE_COOKIES = 0x0002;
+const MIGRATE_HISTORY = 0x0004;
+const MIGRATE_FORMDATA = 0x0008;
+const MIGRATE_PASSWORDS = 0x0010;
+const MIGRATE_BOOKMARKS = 0x0020;
+const MIGRATE_OTHERDATA = 0x0040;

Comment 1

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

Comment on attachment 578153 [details] [diff] [review]
Use constants defined on nsIBrowserProfileMigrator rather than redefining them

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

I find this less readable than the previous consts, and still you need to shortcut Ci.nsIBrowserProfileMigrator in a const.
What if we instead just forward the consts like:
const MIGRATE_BOOKMARKS = Ci.nsIBrowserProfileMigrator.BOOKMARKS;

this would also help when in future we kill xpcom craziness from here.

Comment 2

[Matthew N. [:MattN]]

(In reply to Marco Bonardo [:mak] from comment #1)
> Review of attachment 578153 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I find this less readable than the previous consts, and still you need to
> shortcut Ci.nsIBrowserProfileMigrator in a const.

One const instead of duplicating 8 still seems like a win to me.  If by readable, you mean that the name is not clear then I can easily change "BPM" to "MIGRATOR" or "MIGRATION_ITEM".

> What if we instead just forward the consts like:
> const MIGRATE_BOOKMARKS = Ci.nsIBrowserProfileMigrator.BOOKMARKS;

IMO that seems error-prone and unnecessary if a new name has equivalent readability.

> this would also help when in future we kill xpcom craziness from here.

We can easily do a search and replace at that point and these consts wouldn't live here then anyways.  I'm not sure what "XPCOM craziness" you are referring to and I didn't even know that was planned.  I thought we were changing the implementations to JS but not necessarily deCOMtaminating.

At this point I think this cleanup is still worthwhile.  Having this pattern copied in all of the migrators written in JS doesn't make sense to me.  There are two being written in JS right now and they are both copying the patterns from the Chrome migrator.

Do you think renaming BPM is fine?

Comment 3

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

I think deCOMtaminating should be feasible, btw that's another story.

yeah my first concern was BPM.BOOKMARKS doesn't mean a thing. MIGRATOR.BOOKMARKS is slightly better, not perfect but I could live with it.

Comment 4

[Matthew N. [:MattN]]

Attachment: Use MIGRATOR rather than BPM

s/BPM/MIGRATOR/g

Comment 5

[Matthew N. [:MattN]]

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

Comment 6

[Ed Morley [:emorley]]

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