Last Comment Bug 706733 - Chrome migrator shouldn't duplicate type constants
: Chrome migrator shouldn't duplicate type constants
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Firefox 12
Assigned To: Matthew N. [:MattN] (PM me if requests are blocking you)
:
: Matthew N. [:MattN] (PM me if requests are blocking you)
Mentors:
Depends on:
Blocks: chrome-migration
  Show dependency treegraph
 
Reported: 2011-11-30 18:17 PST by Matthew N. [:MattN] (PM me if requests are blocking you)
Modified: 2012-01-11 18:16 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use constants defined on nsIBrowserProfileMigrator rather than redefining them (9.36 KB, patch)
2011-11-30 18:17 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
mak77: review-
Details | Diff | Splinter Review
Use MIGRATOR rather than BPM (9.43 KB, patch)
2011-12-15 21:57 PST, Matthew N. [:MattN] (PM me if requests are blocking you)
mak77: review+
Details | Diff | Splinter Review

Description Matthew N. [:MattN] (PM me if requests are blocking you) 2011-11-30 18:17:28 PST
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;
Comment 1 Marco Bonardo [::mak] 2011-12-01 06:14:26 PST
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] (PM me if requests are blocking you) 2011-12-01 15:35:47 PST
(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] 2011-12-02 03:49:41 PST
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] (PM me if requests are blocking you) 2011-12-15 21:57:00 PST
Created attachment 582179 [details] [diff] [review]
Use MIGRATOR rather than BPM

s/BPM/MIGRATOR/g
Comment 5 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-10 18:34:44 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d90462cc6d
Comment 6 Ed Morley [:emorley] 2012-01-11 18:16:57 PST
https://hg.mozilla.org/mozilla-central/rev/b6d90462cc6d

Note You need to log in before you can comment on or make changes to this bug.