Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Chrome migrator shouldn't duplicate type constants

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Migration
--
minor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 578153 [details] [diff] [review]
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;
Attachment #578153 - Flags: review?(dolske)
Attachment #578153 - Flags: review?(dolske) → review?(mak77)
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.
Attachment #578153 - Flags: review?(mak77) → review-
(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?
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.
Created attachment 582179 [details] [diff] [review]
Use MIGRATOR rather than BPM

s/BPM/MIGRATOR/g
Attachment #578153 - Attachment is obsolete: true
Attachment #582179 - Flags: review?(mak77)

Updated

6 years ago
Attachment #582179 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d90462cc6d
Target Milestone: --- → Firefox 12

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/b6d90462cc6d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.