Closed Bug 706733 Opened 13 years ago Closed 13 years ago

Chrome migrator shouldn't duplicate type constants

Categories

(Firefox :: Migration, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 12

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
s/BPM/MIGRATOR/g
Attachment #578153 - Attachment is obsolete: true
Attachment #582179 - Flags: review?(mak77)
Attachment #582179 - Flags: review?(mak77) → review+
Target Milestone: --- → Firefox 12
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: