Last Comment Bug 710895 - Port IE profile migrator to JS
: Port IE profile migrator to JS
Status: RESOLVED FIXED
[snappy:P1]
:
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Marco Bonardo [::mak]
:
Mentors:
: 796855 (view as bug list)
Depends on: 591884 749548
Blocks: 718280 719321 745853 256052 353802 361858 562307 698433 722723 733641 739213 741993
  Show dependency treegraph
 
Reported: 2011-12-14 14:40 PST by :Felipe Gomes (needinfo me!)
Modified: 2015-07-27 22:29 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Migrator structure + bookmarks (15.63 KB, patch)
2011-12-14 14:45 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Part 2 - History reader helper (12.50 KB, patch)
2011-12-14 14:46 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Part 3 - Main IE prefs (8.22 KB, patch)
2011-12-14 14:47 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
IE Migrator structure + bookmarks (7.89 KB, patch)
2012-01-20 01:29 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Import preferences (10.96 KB, patch)
2012-01-20 01:32 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
History reader helper (12.41 KB, patch)
2012-01-24 18:55 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
History migrator (3.17 KB, patch)
2012-01-24 18:56 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Remove previous C++ migrator (106.75 KB, patch)
2012-01-24 18:57 PST, :Felipe Gomes (needinfo me!)
mak77: review+
Details | Diff | Splinter Review
Import preferences (7.16 KB, patch)
2012-01-30 03:12 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
History reader helper, v2 (13.01 KB, patch)
2012-02-13 16:35 PST, :Felipe Gomes (needinfo me!)
mak77: review+
Details | Diff | Splinter Review
Part 1: IE History Reader v3.0 (13.04 KB, patch)
2012-04-03 16:45 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 2: Remove old migrator v3.0 (93.04 KB, patch)
2012-04-03 16:48 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 3: Boilerplate and bookmarks v3.0 (7.69 KB, patch)
2012-04-03 16:58 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 4: History Migrator v3.0 (3.97 KB, patch)
2012-04-03 17:02 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
IE History Enumerator v3.1 (10.90 KB, patch)
2012-04-04 05:19 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 1: IE History Enumerator v3.1 (10.96 KB, patch)
2012-04-04 06:07 PDT, Marco Bonardo [::mak]
asaf: review+
mak77: checkin+
Details | Diff | Splinter Review
Part 3: boilerplate and bookmarks (8.29 KB, patch)
2012-04-05 04:58 PDT, Marco Bonardo [::mak]
asaf: feedback+
Details | Diff | Splinter Review
Part 4: history (4.00 KB, patch)
2012-04-05 04:59 PDT, Marco Bonardo [::mak]
asaf: feedback+
Details | Diff | Splinter Review
Part 3: boilerplate and bookmarks (8.25 KB, patch)
2012-04-06 17:22 PDT, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Splinter Review
Part 4: history (4.02 KB, patch)
2012-04-06 17:38 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 4: history (4.02 KB, patch)
2012-04-06 17:39 PDT, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Splinter Review
Part 5: Cookies migrator (9.89 KB, patch)
2012-04-11 05:45 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 6: Homepage migrator. (5.37 KB, patch)
2012-04-11 05:48 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 7: Keywords migrator (4.36 KB, patch)
2012-04-11 05:49 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 3: boilerplate and bookmarks (8.20 KB, patch)
2012-04-13 03:04 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 4: history (4.18 KB, patch)
2012-04-13 03:04 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 5: Cookies migrator (9.81 KB, patch)
2012-04-13 03:05 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 6: Homepage migrator (6.20 KB, patch)
2012-04-13 03:07 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 7: Keywords migrator (4.02 KB, patch)
2012-04-13 03:08 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part8: Settings migrator (4.68 KB, patch)
2012-04-13 03:09 PDT, Marco Bonardo [::mak]
asaf: review-
Details | Diff | Splinter Review
Part 2: remove old migrator and unused stuff (104.05 KB, patch)
2012-04-16 13:16 PDT, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Splinter Review
Part 3: the new JS migrator (27.97 KB, patch)
2012-04-16 14:00 PDT, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Splinter Review
Part 3: the new JS migrator (27.20 KB, patch)
2012-04-17 06:15 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
Part 4: fix the test (6.32 KB, patch)
2012-04-17 17:04 PDT, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Splinter Review

Description :Felipe Gomes (needinfo me!) 2011-12-14 14:40:13 PST
Port the old IE migrator to JS and use new places async APIs
Comment 1 :Felipe Gomes (needinfo me!) 2011-12-14 14:45:19 PST
Created attachment 581797 [details] [diff] [review]
Part 1 - Migrator structure + bookmarks
Comment 2 :Felipe Gomes (needinfo me!) 2011-12-14 14:46:02 PST
Created attachment 581798 [details] [diff] [review]
Part 2 - History reader helper
Comment 3 :Felipe Gomes (needinfo me!) 2011-12-14 14:47:04 PST
Created attachment 581799 [details] [diff] [review]
Part 3 - Main IE prefs
Comment 4 Dietrich Ayala (:dietrich) 2012-01-19 11:25:47 PST
Felipe, do you have any update on this?
Comment 5 :Felipe Gomes (needinfo me!) 2012-01-20 01:29:16 PST
Created attachment 590141 [details] [diff] [review]
IE Migrator structure + bookmarks

Sorry, I got a bit behind on this because I was updating all the patches to already use Mano's changes to the MigrationUtils helpers. Thus this patch requires the changes from the Safari's patch that have been spun off to bug 718608.

This part sets up the new migrator JS module and handles all the bookmarks importing features.

I'm breaking everything in separate patches for easier review. The removal of the old .cpp files are done on a separate patch
Comment 6 :Felipe Gomes (needinfo me!) 2012-01-20 01:32:39 PST
Created attachment 590142 [details] [diff] [review]
Import preferences

This patch ports the Preferences importer. I moved most of the prefs conversion code to a separate file that is #included in the main file; this should make it easier to visualize the list of imported preferences and update/add new prefs in the future.

I'll post the other patches tomorrow
Comment 7 Marco Bonardo [::mak] 2012-01-20 03:11:47 PST
Thanks, I'll try to complete reviewing Mano's changes today and then these, so we can move on and hopefully have working migrators in 12.
Comment 8 :Felipe Gomes (needinfo me!) 2012-01-24 18:55:52 PST
Created attachment 591347 [details] [diff] [review]
History reader helper

This is the history reader service. I changed it a bit from the last version to output an object with the data instead of using outparams. I was gonna use mozIVisitInfo or mozIPlaceInfo but none of those fit the requirements, so I created a new one (nsIIEHistoryEntry).

I've split the JS code that uses it and talks to Places on a separate patch so you can direct this review to some windev if you prefer
Comment 9 :Felipe Gomes (needinfo me!) 2012-01-24 18:56:55 PST
Created attachment 591348 [details] [diff] [review]
History migrator

This is the actual JS history code that uses the helper and adds the data to Places using the async API.
Comment 10 :Felipe Gomes (needinfo me!) 2012-01-24 18:57:42 PST
Created attachment 591349 [details] [diff] [review]
Remove previous C++ migrator

Code and module removal for the previous migrator
Comment 11 :Felipe Gomes (needinfo me!) 2012-01-30 03:12:01 PST
Created attachment 592652 [details] [diff] [review]
Import preferences

After talking with bsmith and going over all the network prefs that are currently being imported, we reached the conclusion that those preferences shouldn't be imported at all, and the network setting should be left as "Use system settings", which will keep the current behavior.
The current (old) migrator does the following:

1 - if there are custom network settings on the system, we clone them to our own settings and mark as "use custom setting"
2 - if there are no custom settings, we mark as "use no proxy"

Both of these behaviors are wrong. (Dolske also mentioned prob #2 on bug 719321 comment 1)

So this is an updated patch that removes all the network prefs migration code that I had ported to JS
Comment 12 Marco Bonardo [::mak] 2012-02-03 06:35:32 PST
Comment on attachment 591347 [details] [diff] [review]
History reader helper

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

::: browser/components/migration/public/nsIIEHistoryReader.idl
@@ +22,5 @@
> +
> +  /**
> +   * The time and date of the last visit.
> +   */
> +  readonly attribute PRTime lastVisit;

maybe lastVisitTime would be clearer

@@ +43,5 @@
> +
> +  /*
> +   * Retrieves the next entry from Windows' history service.
> +   *
> +   * @returns nsIIEHistoryEntry Returns an object containing the URI of the

the right form is @return without the s.
Please also add @throws to specify uncommon cases when it throws.
Also specify that returns null on the last entry

@@ +45,5 @@
> +   * Retrieves the next entry from Windows' history service.
> +   *
> +   * @returns nsIIEHistoryEntry Returns an object containing the URI of the
> +                                entry, the time it was last visited, and a
> +   *                            title (which is blank for non-toplevel visits).

as discussed we don't have evidence these blank elements are non-toplevel. Btw, if you can find a correlation among those, this comment should rather be on nsIIEHistoryEntry::title than here.

@@ +47,5 @@
> +   * @returns nsIIEHistoryEntry Returns an object containing the URI of the
> +                                entry, the time it was last visited, and a
> +   *                            title (which is blank for non-toplevel visits).
> +   */
> +  nsIIEHistoryEntry getNextEntry();

As we discussed, I think having a begin() and a end() is pretty much error-prone and looks like redundant work that the instance may do by itself. Imo this should just be a Reader instance that lazily initializes the resource on the first getNextEntry call, and destroy it either on the last entry or on destruction, if the last entry was not required. if the user needs to read it in multiple places he should just instance a new Reader.

::: browser/components/migration/src/nsIEHistoryReader.cpp
@@ +25,5 @@
> +nsIEHistoryEntry::~nsIEHistoryEntry()
> +{
> +}
> +
> +NS_IMETHODIMP nsIEHistoryEntry::GetUri(nsIURI * *aURI)

global: decorators towards types please, nsIURI** is fine

@@ +94,5 @@
> +{
> +  if (!mURLEnumerator)
> +    return NS_ERROR_NOT_INITIALIZED;
> +
> +  *retval = nsnull;

just in case, set this before the initial check. it's true that nobody should use return value when we throw, but we can still be nice

@@ +102,5 @@
> +  SYSTEMTIME st;
> +
> +  HRESULT hr = mURLEnumerator->Next(1, &statURL, &fetched);
> +
> +  if (FAILED(hr) || fetched != 1UL)

nit: no newline in between error fetching and checking

@@ +119,5 @@
> +  prt.tm_wday = 0;
> +  prt.tm_yday = 0;
> +  prt.tm_params.tp_gmt_offset = 0;
> +  prt.tm_params.tp_dst_offset = 0;
> +  PRTime lastVisited = PR_ImplodeTime(&prt);

could ou make this convertion into an anonymous namespaced helper function?

::: browser/components/migration/src/nsIEHistoryReader.h
@@ +21,5 @@
> +  NS_DECL_NSIIEHISTORYENTRY
> +
> +nsIEHistoryEntry(const nsString& aTitle,
> +                 already_AddRefed<nsIURI> aURI,
> +                 PRTime aLastVisit);

wrong indentation
Comment 13 Marco Bonardo [::mak] 2012-02-03 07:42:32 PST
Comment on attachment 590141 [details] [diff] [review]
IE Migrator structure + bookmarks

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

global-nit: please prefix params with "a" per code style

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

I'd suggest to make PlacesUtils a lazyModuleGetter, we may not need it in some case (like migrating just cookies, to say one)

@@ +32,5 @@
> +
> +  _toolbarFolder: null,
> +
> +  migrate: function(aCallback, aReplace) {
> +   AsyncMigrateFunction(function() {

actually, I think that the module already checks for migrate throwing an invokes the callback by itself, there should be no need to do all of this wrapping. and if it doesn't it should. There should really be no need to wrap like this in the end.

@@ +36,5 @@
> +   AsyncMigrateFunction(function() {
> +    let favs = Services.dirsvc.get("Favs", Ci.nsIFile);
> +
> +    if (!favs.exists() || !favs.isReadable())
> +      throw "Could not read bookmarks folder";

always throw New Error( please

@@ +43,5 @@
> +    // in the toolbar. This was previously stored in the registry and changed
> +    // in IE7 to always be called "Links".
> +    let toolbarName = null;
> +    try {
> +      let registry = Cc["@mozilla.org/windows-registry-key;1"].createInstance(Ci.nsIWindowsRegKey);

limit to about 80 chars please

@@ +54,5 @@
> +      }
> +      registry.close();
> +    } catch (ex) { }
> +    this._toolbarFolder = favs.clone();
> +    this._toolbarFolder.append(toolbarName || "Links");

you may make a nice helper to get the toolbarName so we move all this code out of here.

@@ +57,5 @@
> +    this._toolbarFolder = favs.clone();
> +    this._toolbarFolder.append(toolbarName || "Links");
> +
> +
> +    function migrateBatched() {

too many newlines

@@ +61,5 @@
> +    function migrateBatched() {
> +      let placesFolderId;
> +      if (!aReplace) {
> +        placesFolderId = MigrationUtils.createImportedBookmarksFolder("IE",
> +        PlacesUtils.bookmarksMenuFolderId);

better indentation please

@@ +63,5 @@
> +      if (!aReplace) {
> +        placesFolderId = MigrationUtils.createImportedBookmarksFolder("IE",
> +        PlacesUtils.bookmarksMenuFolderId);
> +      } else {
> +        placesFolderId = PlacesUtils.bookmarksMenuFolderId;

just init to the menu folder and overwrite if !aReplace

@@ +74,5 @@
> +    }
> +
> +    PlacesUtils.bookmarks.runInBatchMode({
> +      runBatched: migrateBatched.bind(this)
> +    }, null);

we should have done this from a long time, btw, if you wish you may add the "function" specification to the batch, so that you don't need to make an object.

@@ +81,5 @@
> +
> +   }.bind(this), aCallback)();
> +  },
> +
> +  _migrateFolder: function IE_migrateFolder(folder, placesFolderId) {

s/placesFolderId/targetFolderId

@@ +99,5 @@
> +        let subfolderId = entry.equals(this._toolbarFolder) ?
> +                          PlacesUtils.toolbarFolderId :
> +                          PlacesUtils.bookmarks.createFolder(placesFolderId,
> +                                                             entry.leafName,
> +                                                             PlacesUtils.bookmarks.DEFAULT_INDEX);

A common if/else would be more readable in this case

@@ +103,5 @@
> +                                                             PlacesUtils.bookmarks.DEFAULT_INDEX);
> +
> +        this._migrateFolder(entry, subfolderId);
> +      } else {
> +        let fileURL = Services.io.newFileURI(entry).QueryInterface(Ci.nsIFileURL);

I suspect NetUtil.newURI may do all of this for you?

@@ +105,5 @@
> +        this._migrateFolder(entry, subfolderId);
> +      } else {
> +        let fileURL = Services.io.newFileURI(entry).QueryInterface(Ci.nsIFileURL);
> +
> +        if (/url/i.test(fileURL.fileExtension)) {

doesn't look like you need a regex here, a == fileURL.fileExtension.toLowercase may be cheaper

@@ +106,5 @@
> +      } else {
> +        let fileURL = Services.io.newFileURI(entry).QueryInterface(Ci.nsIFileURL);
> +
> +        if (/url/i.test(fileURL.fileExtension)) {
> +          let fileHandler = Cc["@mozilla.org/network/protocol;1?name=file"].getService(Ci.nsIFileProtocolHandler);

80 chars limit please

@@ +108,5 @@
> +
> +        if (/url/i.test(fileURL.fileExtension)) {
> +          let fileHandler = Cc["@mozilla.org/network/protocol;1?name=file"].getService(Ci.nsIFileProtocolHandler);
> +          let uri = fileHandler.readURLFile(entry);
> +          let name = entry.leafName.substr(0, entry.leafName.length - 4); // -4 == ".url".length

we usually use title, rather than name

::: browser/components/migration/src/Makefile.in
@@ +61,5 @@
>    ProfileMigrator.js \
>    ChromeProfileMigrator.js \
>    FirefoxProfileMigrator.js \
>    SafariProfileMigrator.js \
> +  IEProfileMigrator.js \

doesn't look like you have anything to preprocess, so for now should go to the EXTRA_COMPONENTS section
Comment 14 Marco Bonardo [::mak] 2012-02-03 08:12:49 PST
Comment on attachment 591348 [details] [diff] [review]
History migrator

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

::: browser/components/migration/src/IEProfileMigrator.js
@@ +238,5 @@
> +  migrate: function(aCallback) {
> +    // First get the list of typed URLs since it's stored in the registry rather
> +    // than the history service. Currently, IE stores 25 entries and this value
> +    // is not configurable. We will hard-limit it to 100 entries since we don't
> +    // want to be surprised here from an unbounded changed in a future IE version.

typo: unbounded changed

@@ +242,5 @@
> +    // want to be surprised here from an unbounded changed in a future IE version.
> +    let IETypedURLs = {};
> +
> +    try {
> +      let registry = Cc["@mozilla.org/windows-registry-key;1"].createInstance(Ci.nsIWindowsRegKey);

limit to 80 chars please

@@ +252,5 @@
> +        if (registry.hasValue("url" + entry)) {
> +          let url = registry.readStringValue("url" + entry);
> +          IETypedURLs[url] = true;
> +        } else {
> +          break;

so you could change the for condition to
entry <= 100 && registry.hasValue("url" + entry)

@@ +256,5 @@
> +          break;
> +        }
> +      }
> +      registry.close();
> +    } catch (ex) {}

we should probably reportError this, otherwise I don't see a way users may notice something going wrong.

@@ +259,5 @@
> +      registry.close();
> +    } catch (ex) {}
> +
> +    let places = [];
> +    let histreader = Cc["@mozilla.org/profile/migrator/iehistoryreader;1"].getService(Ci.nsIIEHistoryReader);

createInstance please, according to the idl changes we discussed

@@ +264,5 @@
> +    let entry, transitionType;
> +
> +    histreader.begin();
> +    while (entry = histreader.getNextEntry()) {
> +      if (!entry.uri ||

should we export the entry if it doesn't have an url? My understanding is that the reader should already filter these out.

@@ +271,5 @@
> +        continue;
> +      }
> +
> +      if (!entry.title) {
> +        transitionType = Ci.nsINavHistoryService.TRANSITION_EMBED;

as discussed, no point in storing embed visits, they will be lost with the session since are only used for temporary link coloring
Comment 15 Marco Bonardo [::mak] 2012-02-03 08:14:43 PST
Comment on attachment 591349 [details] [diff] [review]
Remove previous C++ migrator

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

nothing interesting to review here :)
Comment 16 Marco Bonardo [::mak] 2012-02-03 09:19:57 PST
Comment on attachment 592652 [details] [diff] [review]
Import preferences

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

::: browser/components/migration/src/IERegistryPrefs.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +let IERegistryPrefs = [

may you please document the format of each entry above?

@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +let IERegistryPrefs = [
> +  [ "Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\AutoComplete", "AutoSuggest",
> +     "browser.urlbar.autocomplete.enabled", IE_ReadBoolPreference ],

hm, I think here we are shooting ourselves in the foot here, the user may have disavled ie autocomplete cause it sucks, the awesomebar is quite a different concept and a central point of our experience. don't think we should disable it since it's not really the same thing.

@@ +6,5 @@
> +  [ "Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\AutoComplete", "AutoSuggest",
> +     "browser.urlbar.autocomplete.enabled", IE_ReadBoolPreference ],
> +  [ "Software\\Microsoft\\Internet Explorer\\International", "AcceptLanguage",
> +    "intl.accept_languages", IE_ReadLangListPreference ],
> +  // XXX : For now, only x-western font is translated.

no XXX please, ensure if a bug is filed, if not filed the bug and make this a // TODO (bug 123456): some comment

@@ +27,5 @@
> +    "accessibility.browsewithcaret", IE_ReadBoolPreference ],
> +  [ null, "NotifyDownloadComplete",
> +    "browser.download.manager.showAlertOnComplete", IE_ReadBoolPreference ],
> +  [ null, "SmoothScroll",
> +    "general.smoothScroll", IE_ReadBoolPreference ],

our smoothscrolll is likely different, not sure if this makes sense, if ie's one is bad not a sign of ours being. Maybe ask Jared?

@@ +39,5 @@
> +    "browser.anchor_color", IE_ReadDRGBPreference ],
> +  [ null, "Anchor Color Visited",
> +    "browser.visited_color", IE_ReadDRGBPreference ],
> +  [ null, "Always Use My Font Face",
> +    "browser.display.use_document_fonts", IE_ReadInvertedBoolPreference ]

I think to each entry you should add a validator function that ensures that what we are about to store makes any sense... I fear registry corruption here, and we may end up storing bad values. if the validator returns false we should not import the pref.

@@ +42,5 @@
> +  [ null, "Always Use My Font Face",
> +    "browser.display.use_document_fonts", IE_ReadInvertedBoolPreference ]
> +];
> +
> +function IE_ReadLangListPreference(registry, keyName, prefName) {

these functions don't just read the value, they actually convert it prefs to ours.
s/Read/Import/
I'd be fine with cropping Preference to Pref (we do the same for our preferences service)
Comment 17 Dietrich Ayala (:dietrich) 2012-02-09 11:21:49 PST
Hey Felipe, what's the status of this?
Comment 18 :Felipe Gomes (needinfo me!) 2012-02-09 11:34:44 PST
Hi Dietrich, I've updated the patches with most of mak's comments, but I'm waiting on bug 718608 to see what other changes will be needed
Comment 19 :Felipe Gomes (needinfo me!) 2012-02-13 16:35:28 PST
Created attachment 596850 [details] [diff] [review]
History reader helper, v2

Posting the new patch for the history reader already is it won't depend on other changes

(In reply to Marco Bonardo [:mak] from comment #12)
> Review of attachment 591347 [details] [diff] [review]:
> ::: browser/components/migration/public/nsIIEHistoryReader.idl
> > +  readonly attribute PRTime lastVisit;
> 
> maybe lastVisitTime would be clearer
done

> the right form is @return without the s.
> Please also add @throws to specify uncommon cases when it throws.
> Also specify that returns null on the last entry

fixed the @return and explained the null return after the last entry. With the removal of begin/end it doesn't throw anymore (it would throw if you attempted to use it before initializing)

> as discussed we don't have evidence these blank elements are non-toplevel.
> Btw, if you can find a correlation among those, this comment should rather
> be on nsIIEHistoryEntry::title than here.

Moved the comment to nsIIEHistoryEntry::title. The win API docs say that it's possible to tell if an entry is for a top-level visit or not, but that data is always served as false, so apparently that's not implemented. Going by the blank title seems to be our only alternative

> 
> @@ +47,5 @@
> > +   * @returns nsIIEHistoryEntry Returns an object containing the URI of the
> > +                                entry, the time it was last visited, and a
> > +   *                            title (which is blank for non-toplevel visits).
> > +   */
> > +  nsIIEHistoryEntry getNextEntry();
> 
> As we discussed, I think having a begin() and a end() is pretty much
> error-prone and looks like redundant work that the instance may do by
> itself. Imo this should just be a Reader instance that lazily initializes
> the resource on the first getNextEntry call, and destroy it either on the
> last entry or on destruction, if the last entry was not required. if the
> user needs to read it in multiple places he should just instance a new
> Reader.

Done!

> 
> @@ +119,5 @@
> > +  prt.tm_params.tp_gmt_offset = 0;
> > +  prt.tm_params.tp_dst_offset = 0;
> > +  PRTime lastVisited = PR_ImplodeTime(&prt);
> 
> could ou make this convertion into an anonymous namespaced helper function?

yeah, done

All other small nits fixed too
Comment 20 Marco Bonardo [::mak] 2012-02-14 06:30:04 PST
I'll try to do this review tomorrow, or at a maximum on Thursday.
Comment 21 Marco Bonardo [::mak] 2012-02-16 07:59:58 PST
Comment on attachment 596850 [details] [diff] [review]
History reader helper, v2

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

thanks. You will need an SR on this since introduces a new interface, I assume gavin may do it.

::: browser/components/migration/src/nsIEHistoryReader.cpp
@@ +9,5 @@
> +
> +#include "nsNetUtil.h"
> +
> +namespace {
> +  PRTime FileTimeToPlacesTime(FILETIME* filetime) {

FileTimeToPRTime please, this is more generic than just Places.
please indent as
rtype
function(args)
{

could this just take a const FILETIME&?

@@ +28,5 @@
> +    prt.tm_params.tp_gmt_offset = 0;
> +    prt.tm_params.tp_dst_offset = 0;
> +    return PR_ImplodeTime(&prt);
> +  }
> +}

please mark end of namespaces, when they grow it's useful (likely this won't grow though it's good habit)
} // Anonymous namespace.

@@ +100,5 @@
> +  }
> +
> +  hr = mIEHistory->EnumUrls(&mURLEnumerator);
> +
> +  if (FAILED(hr)) {

nit: useless newline

@@ +104,5 @@
> +  if (FAILED(hr)) {
> +    mIEHistory->Release();
> +    ::CoUninitialize();
> +    mIEHistory = nsnull;
> +    mURLEnumerator = nsnull;

may you just call Uninit() here?

@@ +124,5 @@
> +  *retval = nsnull;
> +
> +  Init();
> +  if (!mURLEnumerator)
> +    return NS_OK;

or you could have Init return a rv and throw here, this is an initialization failed error, we may be interested in not hiding it.

@@ +129,5 @@
> +
> +  STATURL statURL;
> +  ULONG fetched;
> +
> +  HRESULT hr = mURLEnumerator->Next(1, &statURL, &fetched);

please add a comment regarding the first argument (the doc says it's just not implemented). As it is it looks like a magic number.
Comment 22 Marco Bonardo [::mak] 2012-03-14 08:25:51 PDT
If possible this should fix bookmarks import order, looks like we are importing them in alphabetical order. or file a follow-up for that.
Comment 23 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-21 02:54:11 PDT
It would be great if the the history helper used simple enumerator and property bags, rather than introducing new interfaces.
Comment 24 Marco Bonardo [::mak] 2012-03-21 03:08:52 PDT
We can't use SimpleEnumerator cause we can't implement hasMoreElements easily, we should fetch the element and cache it for getNext, or fetch it in getNext, that adds some complication.
Comment 25 Marco Bonardo [::mak] 2012-03-21 03:10:14 PDT
well, "can't use" is not correct, using it adds complication.
Comment 26 Marco Bonardo [::mak] 2012-04-03 09:49:02 PDT
Hey Felipe, do you mind if I steal you this bug to bring it to completion?
I have the history reader rewritten to not use custom interfaces, and could free you some time to dedicate to webapps.
Comment 27 Marco Bonardo [::mak] 2012-04-03 16:45:37 PDT
Created attachment 612017 [details] [diff] [review]
Part 1: IE History Reader v3.0

Reimplemented using nsISimpleEnumerator and nsIPropertyBag, to avoid new interfaces.
Comment 28 Marco Bonardo [::mak] 2012-04-03 16:48:16 PDT
Created attachment 612018 [details] [diff] [review]
Part 2: Remove old migrator v3.0

unbitrot, and it's a bit smaller than the previous one since I still don't know the order Safari and IE migrators will land (I'm working on a clean tree atm), btw we may even remove utils.cpp at a later time, doesn't matter much.
Comment 29 Marco Bonardo [::mak] 2012-04-03 16:58:15 PDT
Created attachment 612021 [details] [diff] [review]
Part 3: Boilerplate and bookmarks v3.0

Unbitrot against the new utils and fix my comments. Let's see how far we are.
Comment 30 Marco Bonardo [::mak] 2012-04-03 17:02:53 PDT
Created attachment 612025 [details] [diff] [review]
Part 4: History Migrator v3.0

Unbitrot and using the new reader.
So far I've tested bookmarks and history migration, and they work.
Comment 31 Marco Bonardo [::mak] 2012-04-03 17:24:44 PDT
So, what's left to convert from the old migrator: prefs, cookies, homepage, keywords.
Comment 32 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-04 01:08:42 PDT
Comment on attachment 612017 [details] [diff] [review]
Part 1: IE History Reader v3.0

1. Is there any reason not to use the default writable property-bag implementation?
2. Please switch to CComPtr where appropriate.  I would also move the calls to CoInitialize and CoUninitialize to the constructor and the destructor.
Comment 33 Makoto Kato [:m_kato] 2012-04-04 01:15:48 PDT
(In reply to Mano from comment #32)
> 2. Please switch to CComPtr where appropriate.  I would also move the calls
> to CoInitialize and CoUninitialize to the constructor and the destructor.

We don't use ATL on Firefox code.  CComPtr is ATL's.  So we should use nsRefPtr instead of CComPtr for auto AddRef/Release.
Comment 34 Marco Bonardo [::mak] 2012-04-04 01:18:58 PDT
(In reply to Mano from comment #32)
> Comment on attachment 612017 [details] [diff] [review]
> Part 1: IE History Reader v3.0
> 
> 1. Is there any reason not to use the default writable property-bag
> implementation?

you mean nsHashPropertyBag? It's unaccessible from browsercomps.dll.
Comment 35 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-04 02:49:59 PDT
Comment on attachment 612021 [details] [diff] [review]
Part 3: Boilerplate and bookmarks v3.0

1. As discussed over IRC, the Links folder should be imported as a "From Internet Explorer" folder (under the toolbar) in the case of non-startup migration.
2. Please separate out the _favoritesFolder from the exists getter.
3. s/IEProfileMigrator/IEMigrator
Comment 36 Marco Bonardo [::mak] 2012-04-04 05:19:55 PDT
Created attachment 612154 [details] [diff] [review]
IE History Enumerator v3.1

Updated per IRC and previous comments, hope I didn't forget any comments.
Comment 37 Marco Bonardo [::mak] 2012-04-04 06:07:13 PDT
Created attachment 612162 [details] [diff] [review]
Part 1: IE History Enumerator v3.1
Comment 38 Marco Bonardo [::mak] 2012-04-04 06:36:42 PDT
Landed the IE History Enumerator
https://hg.mozilla.org/integration/mozilla-inbound/rev/13b84bf759eb
Comment 39 Marco Bonardo [::mak] 2012-04-05 04:58:00 PDT
Created attachment 612504 [details] [diff] [review]
Part 3: boilerplate and bookmarks

This should address previous comments
Comment 40 Marco Bonardo [::mak] 2012-04-05 04:59:23 PDT
Created attachment 612505 [details] [diff] [review]
Part 4: history

Updated to the new history enumerator
Comment 41 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-05 08:14:07 PDT
Comment on attachment 612505 [details] [diff] [review]
Part 4: history


>+let History = {
>+  type: Ci.nsIBrowserProfileMigrator.HISTORY,
>+
>+  get exists() true,

I think it's worth creating the enumerator here (as a lazy getter) rather than claiming that.

>+
>+  migrate: function H_migrate(aCallback) {
>+    // First get the list of typed URLs since it's stored in the registry rather
>+    // than the history service. Currently, IE stores 25 entries and this value
>+    // is not configurable. We will hard-limit it to 100 entries since we don't
>+    // want to be surprised from an unbounded changed in a future IE version.
>+    let IETypedURLs = {};
>+    try {
>+      let registry = Cc["@mozilla.org/windows-registry-key;1"].
>+                     createInstance(Ci.nsIWindowsRegKey);
>+      registry.open(Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER,
>+                    "Software\\Microsoft\\Internet Explorer\\TypedURLs",
>+                    Ci.nsIWindowsRegKey.ACCESS_READ);
>+      for (let entry = 1; entry <= 100; entry++) {
>+        if (!registry.hasValue("url" + entry))
>+          break;
>+
>+        let url = registry.readStringValue("url" + entry);
>+        IETypedURLs[url] = true;
>+      }
>+      registry.close();

close should be called in a finally block.

>+    } catch (ex) {}
>+
>+    let places = [];
>+    let historyEnumerator = Cc["@mozilla.org/profile/migrator/iehistoryenumerator;1"].
>+                            getService(Ci.nsISimpleEnumerator);
>+    while (historyEnumerator.hasMoreElements()) {
>+      let entry = historyEnumerator.getNext().QueryInterface(Ci.nsIPropertyBag2);
>+      let uri = entry.get("uri").QueryInterface(Ci.nsIURI);
>+      // MSIE stores some types of URLs in its history that we don't handle,
>+      // like HTMLHelp and others.  Since we don't properly map handling for
>+      // all of them we just avoid importing them.
>+      if (!uri.schemeIs("http") && !uri.schemeIs("https") &&
>+          !uri.schemeIs("ftp") && !uri.schemeIs("file")) {
>+        continue;
>+      }
>+
>+      let title = entry.get("title");
>+      // Embed visits have no title and don't need to be imported.
>+      if (title.length == 0) {

Please make sure this does not filter out ftp entries.
Comment 42 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-05 08:16:49 PDT
The comment about registry.close also applies to the bookmarks resource.
Comment 44 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-05 14:56:03 PDT
Comment on attachment 612504 [details] [diff] [review]
Part 3: boilerplate and bookmarks

(reviewed over irc)
Comment 45 Marco Bonardo [::mak] 2012-04-06 17:22:49 PDT
Created attachment 613032 [details] [diff] [review]
Part 3: boilerplate and bookmarks

Should address all the comments so far.
Comment 46 Marco Bonardo [::mak] 2012-04-06 17:26:38 PDT
(In reply to Mano from comment #41)
> Comment on attachment 612505 [details] [diff] [review]
> Part 4: history
> 
> 
> >+let History = {
> >+  type: Ci.nsIBrowserProfileMigrator.HISTORY,
> >+
> >+  get exists() true,
> 
> I think it's worth creating the enumerator here (as a lazy getter) rather
> than claiming that.

not sure if that makes sense, the only thing that may go wrong in creating the enumerator is the CoInitialize call, but if that fails we have far worse issues in widgets and shell than just an empty history import.
Comment 47 Marco Bonardo [::mak] 2012-04-06 17:38:27 PDT
Created attachment 613036 [details] [diff] [review]
Part 4: history

As well as this one should fix comments.
I verified that both ftp:// and file:// have titles, luckily file: entries added by the text editor (for example) don't have a title, so we'll skip them!

But please check comment 46.
Comment 48 Marco Bonardo [::mak] 2012-04-06 17:39:10 PDT
Created attachment 613037 [details] [diff] [review]
Part 4: history

sorry, didn't qref.
Comment 49 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-07 06:07:29 PDT
Comment on attachment 613032 [details] [diff] [review]
Part 3: boilerplate and bookmarks

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

Looks good overall.

::: browser/components/migration/src/IEProfileMigrator.js
@@ +3,5 @@
> + * This Source Code is subject to the terms of the Mozilla Public License
> + * version 2.0 (the "License"). You can obtain a copy of the License at
> + * http://mozilla.org/MPL/2.0/. */
> +
> +"use strict"

nit: missing semicolon.

@@ +37,5 @@
> +  get _toolbarFolderName() {
> +    if (!this.__toolbarFolderName) {
> +      // Retrieve the name of IE's favorites subfolder that holds the bookmarks
> +      // in the toolbar. This was previously stored in the registry and changed
> +      // in IE7 to always be called "Links".

Should we ignore the registry entry then? What happens if the user has IE 7 installed after s/he changed the folder name in IE 6?

@@ +69,5 @@
> +
> +        this._migrateFolder(this._favoritesFolder, destFolderId);
> +      }).bind(this)
> +    }, null);
> +    aCallback(true);

aCallback should be called in runBatched.

@@ +73,5 @@
> +    aCallback(true);
> +  },
> +
> +  _migrateFolder: function B__migrateFolder(aSourceFolder, aDestFolderId) {
> +    // TODO (bug 741993): the favorites order is stored in the Registry, at

nit: s/the/The

@@ +82,5 @@
> +      let entry = entries.getNext().QueryInterface(Ci.nsIFile);
> +
> +      // Make sure that entry.path == entry.target to not follow .lnk folder
> +      // shortcuts which could lead to infinite cycles.
> +      // IE does not support .lnk folders in their UI.

"in their UI" is somewhat confusing.  Does it mean that one can create these manually and then IE would honor that?

If they're not supported at all, just remove this phrase.

@@ +83,5 @@
> +
> +      // Make sure that entry.path == entry.target to not follow .lnk folder
> +      // shortcuts which could lead to infinite cycles.
> +      // IE does not support .lnk folders in their UI.
> +      if (entry.isDirectory() && entry.path == entry.target) {

You could use !isSymlink.
Comment 50 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-07 06:22:58 PDT
Comment on attachment 613037 [details] [diff] [review]
Part 4: history

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

r=mano.

I disagree with comment 46, but given that other (async) importers have to do the same (report empty history resource as importable), I'm fine with leaving it as is for now.

::: browser/components/migration/src/IEProfileMigrator.js
@@ +199,5 @@
> +      handleError: function() {},
> +      handleCompletion: function() {
> +        aCallback(this._success);
> +      }
> +    });

I'm not sure what's the behavior of updatePlaces for an empty places array (it /should/ throw, IMO).  I would, however, special case it regardless, and call aCallback right away.
Comment 51 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-07 06:27:07 PDT
To be clear, my suggested implementation for exists() would have called hasMoreElements.
Comment 52 Marco Bonardo [::mak] 2012-04-10 12:53:48 PDT
(In reply to Mano from comment #50)
> I'm not sure what's the behavior of updatePlaces for an empty places array
> (it /should/ throw, IMO).  I would, however, special case it regardless, and
> call aCallback right away.

It throws.
Comment 53 Marco Bonardo [::mak] 2012-04-10 15:05:32 PDT
(In reply to Mano from comment #49)
> > +      let entry = entries.getNext().QueryInterface(Ci.nsIFile);
> > +
> > +      // Make sure that entry.path == entry.target to not follow .lnk folder
> > +      // shortcuts which could lead to infinite cycles.
> > +      // IE does not support .lnk folders in their UI.
> 
> "in their UI" is somewhat confusing.  Does it mean that one can create these
> manually and then IE would honor that?
> 
> If they're not supported at all, just remove this phrase.

I tried to make one, it shows a fake folder but not properly (you can't see its contents, it then tries to open it in Explorer with a security dialog), so yeah, not worth talking about it.

> You could use !isSymlink.

Nope, doesn't work with .lnk files.
Comment 54 Marco Bonardo [::mak] 2012-04-11 05:45:26 PDT
Created attachment 613954 [details] [diff] [review]
Part 5: Cookies migrator
Comment 55 Marco Bonardo [::mak] 2012-04-11 05:48:18 PDT
Created attachment 613957 [details] [diff] [review]
Part 6: Homepage migrator.
Comment 56 Marco Bonardo [::mak] 2012-04-11 05:49:42 PDT
Created attachment 613958 [details] [diff] [review]
Part 7: Keywords migrator
Comment 57 Marco Bonardo [::mak] 2012-04-13 03:04:36 PDT
Created attachment 614709 [details] [diff] [review]
Part 3: boilerplate and bookmarks
Comment 58 Marco Bonardo [::mak] 2012-04-13 03:04:58 PDT
Created attachment 614710 [details] [diff] [review]
Part 4: history
Comment 59 Marco Bonardo [::mak] 2012-04-13 03:05:25 PDT
Created attachment 614711 [details] [diff] [review]
Part 5: Cookies migrator
Comment 60 Marco Bonardo [::mak] 2012-04-13 03:07:57 PDT
Created attachment 614712 [details] [diff] [review]
Part 6: Homepage migrator

the type passed with the keyname may look a bit cumbersome, but I reuse this method in prefs migration and there additional optional arguments may hit readability, so I went for this.
Comment 61 Marco Bonardo [::mak] 2012-04-13 03:08:28 PDT
Created attachment 614713 [details] [diff] [review]
Part 7: Keywords migrator
Comment 62 Marco Bonardo [::mak] 2012-04-13 03:09:56 PDT
Created attachment 614714 [details] [diff] [review]
Part8: Settings migrator

I removed some prefs from Felipe's patch, and added another couple ones.
Obviously there are for sure other settings we may import, but I think this is a good start.
Comment 63 Marco Bonardo [::mak] 2012-04-13 03:11:47 PDT
Obviously this could leverage the module changes that are in the revised Firefox migrator, but that didn't land yet.  I'll update to those once available.
Comment 64 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-14 06:21:48 PDT
Comment on attachment 614714 [details] [diff] [review]
Part8: Settings migrator

>+    [ kMainKey,
>+      "Enable AutoImageResize",
>+      "browser.enable_automatic_image_resizing",
>+      function (v) v == "yes" ],
...
>+    [ kMainKey,
>+      "REG_DWORD,Expand Alt Text",
>+      "browser.display.force_inline_alttext",
>+      function (v) v == 1 ],
...
>+    [ "Software\\Microsoft\\Internet Explorer\\TabbedBrowsing\\",
>+      "REG_DWORD,OpenAdjacent",
>+      "browser.tabs.insertRelatedAfterCurrent",
>+      function (v) v == 1 ],

Don't import preferences which has no UI.

>+    [ kMainKey,
>+      "Display Inline Images",
>+      "permissions.default.image",
>+      function (v) Number(v == "yes") ],

0 is not a valid value for permissions.default.image (should be 2, I think)

>+    [ "Software\\Microsoft\\Internet Explorer\\International",
>+      "AcceptLanguage",
>+      "intl.accept_languages",
>+      function (v) {
>+        // Copy source format like "en-us,ar-kw;q=0.7,ar-om;q=0.3" into
>+        // destination format like "en-us, ar-kw, ar-om".
>+        return v.replace(/;[^,]+/g, "").replace(/,/g, ", ");

The old migrator is broken too, yes, but it doesn't make sense to ignore the order for this preference.  Either fix it, or leave this preference to a follow up.

>+Settings.prototype = {
>+  type: Ci.nsIBrowserProfileMigrator.SETTINGS,
>+
>+  get exists() true,
>+
>+  get _prefs() [

I don't see the point for this getter. Make it a constant within the migrate method.


>+    // TODO: For now, only x-western font is translated.
>+    [ "Software\\Microsoft\\Internet Explorer\\International\\Scripts\\3",
>+      "IEFixedFontName",
>+      "font.name.monospace.x-western" ],

As you said, 1998-todo, but please file it.

>+    [ kMainKey,
>+      "Use FormSuggest",
>+      "browser.formfill.enable",
>+      function (v) v == "yes" ],

You may want to define this transform once (same goes for the invert method). Not-so-important though.



>+    [ "Software\\Microsoft\\Internet Explorer\\TabbedBrowsing\\",
>+      "REG_DWORD,WarnOnClose",
>+      "browser.tabs.warnOnClose",
>+      function (v) v == 1 ],

use Boolean
Comment 65 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-14 06:48:57 PDT
Comment on attachment 614712 [details] [diff] [review]
Part 6: Homepage migrator

>+ * Safely reads a value from the registry.
...
>+ * @param aKey
>+ *        The key name.  Can be prefixed by "keyType,".
>+ *        Supported types are: REG_SZ, REG_DWORD, REG_MULTI_SZ, if omitted
>+ *        REG_SZ is assumed.

I Don't Like This. Yet again, leaving the final decision for you :)

>+ * @return The key value or undefined if it doesn't exist.  If the key is
>+ *         a REG_MULTI_SZ, entries are separated by "|".
>+ *
>+ */
>+function readRegKey(aRoot, aPath, aKey) {
>+  let type = "REG_SZ";
>+  let key = aKey;
>+  if (key.indexOf(",") != -1) {
>+    [type, key] = key.split(",");
>+    key = key.trim();
>+  }
>+
>+  let registry = Cc["@mozilla.org/windows-registry-key;1"].
>+                 createInstance(Ci.nsIWindowsRegKey);
>+  try {
>+    registry.open(aRoot, aPath, Ci.nsIWindowsRegKey.ACCESS_READ);
>+    if (registry.hasValue(key)) {
>+      switch (type) {
>+        case "REG_MULTI_SZ":
>+          // nsIWindowsRegKey doesn't support REG_MULTI_SZ type out of the box.
>+          let buffer = registry.readBinaryValue(aKey).replace("\0\0", "\0|");
>+          return [String.fromCharCode(buffer.charCodeAt(i) |
>+                                     (buffer.charCodeAt(i + 1) << 8))
>+                  for (i in buffer) if (i % 2 == 0)
>+                 ].join("");

It seems this dark magic might not be necessary. I'm not sure how \0 in nsAutoString is handled when converted to a js-string.

If it turns out this is necessary as-is: (1) document it a little bit; (2) file a bug on fixing win-reg-key.
Comment 66 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-14 07:09:39 PDT
Comment on attachment 614713 [details] [diff] [review]
Part 7: Keywords migrator

As there's no UI for these keywords in IE, I don't think we should continue importing them. I realize some advanced users (or Maxthon users) have found this, but I'm sure they can manage.
Comment 67 Marco Bonardo [::mak] 2012-04-16 13:16:50 PDT
Created attachment 615441 [details] [diff] [review]
Part 2: remove old migrator and unused stuff

This should contain all of the removals now.
Comment 68 Marco Bonardo [::mak] 2012-04-16 14:00:12 PDT
Created attachment 615457 [details] [diff] [review]
Part 3: the new JS migrator
Comment 69 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-17 03:37:14 PDT
Comment on attachment 615457 [details] [diff] [review]
Part 3: the new JS migrator

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

r=mano.

::: browser/components/migration/src/IEProfileMigrator.js
@@ +120,5 @@
> +function hostIsIPAddress(aHost) {
> +  try {
> +    Services.eTLD.getBaseDomainFromHost(aHost);
> +  } catch (e if e.result == Cr.NS_ERROR_HOST_IS_IP_ADDRESS) {
> +    return true;

This is pretty ugly. Please file a bug on exposing PR_StringToNetAddr.

@@ +135,5 @@
> + *        The registry path to the key.
> + * @param aKey
> + *        The key name.
> + * @return The key value or undefined if it doesn't exist.  If the key is
> + *         a REG_MULTI_SZ, entries are separated by "|".

"If the key is
* a REG_MULTI_SZ, entries are separated by "|"." - I think it's cleaner to do the final join in the caller.  Here you should just return the array.

@@ +283,5 @@
> +    if (!__typedURLs) {
> +      // The list of typed URLs is a sort of annotation stored in the registry.
> +      // Currently, IE stores 25 entries and this value is not configurable.  We
> +      // rather limit it to 100 entries, to support eventual increases in future
> +      // IE versions.

Given that the list in question is ordered, a less arbitrary criteria would be to stop at the first url that was not returned by the enumerator.

I realize this does require some changes, and it's probably not so important, so I'm ok with landing this as-is.

With all that said, I'm not sure what are you afraid of here - reading registry keys isn't that slow, is it?

@@ +284,5 @@
> +      // The list of typed URLs is a sort of annotation stored in the registry.
> +      // Currently, IE stores 25 entries and this value is not configurable.  We
> +      // rather limit it to 100 entries, to support eventual increases in future
> +      // IE versions.
> +      this.__typedURLs = {};

Looking at the relevant bugs, it seems that on trunk it's safe to use Set.  However, if you decide not to so, go with a simple array.

@@ +309,5 @@
> +
> +  migrate: function H_migrate(aCallback) {
> +    let places = [];
> +    let historyEnumerator = Cc["@mozilla.org/profile/migrator/iehistoryenumerator;1"].
> +                            getService(Ci.nsISimpleEnumerator);

Not a service.  Using this as a service would make it work only once-per-session.

@@ +327,5 @@
> +      if (title.length == 0) {
> +        continue;
> +      }
> +
> +      let transitionType = this._typedURLs[uri.spec] ?

It may worth stating that those  "typed" urls are fixed up urls, and therefore that comparing them to the spec is guaranteed to work.

@@ +381,5 @@
> +    // - Windows Vista/7 for apps running with low privileges (UAC):
> +    //     Users\<username>\AppData\Roaming\Microsoft\Windows\Cookies\Low\
> +    //     This is the most common destination for IE, unless the user
> +    //     explicitly executes the browser with administrative privileges or
> +    //     disables UAC.  The latter case is what we care about.

Please remove the two other cases (CookD is here for that) and focus on the UAC case.

@@ +387,5 @@
> +      let cookiesFolder = Services.dirsvc.get("CookD", Ci.nsIFile);
> +      if (cookiesFolder.exists() && cookiesFolder.isReadable()) {
> +        // Check if UAC is enabled.
> +        if (Services.appinfo.QueryInterface(Ci.nsIWinAppHelper).userCanElevate) {
> +          cookiesFolder.append("Low");

Shouldn't it be done by dirsvc though? File a bug please.

@@ +426,5 @@
> +    }).apply(this);
> +    cookiesGenerator.next();
> +  },
> +
> +  _parseCookieFile: function C__parseCookieFile(aFile, aCallback) {

parse -> read

@@ +512,5 @@
> +function Settings() {
> +}
> +
> +Settings.prototype = {
> +  type: Ci.nsIBrowserProfileMigrator.SETTINGS,

I'm unhappy about the number of preferences not imported, but it shouldn't block landing. Please file a bug on extending this resource.

@@ +539,5 @@
> +    //  * http://msdn.microsoft.com/en-us/library/cc980059%28v=prot.13%29.aspx
> +
> +    // Note that only settings exposed in our UI should be migrated.
> +
> +    let anySet = false;

Remember to remove this, as discussed over #places. _set could still return whether or not the value was set, if you wish to keep it in sync with its version in the Safari migrator. I don't mind either way.

@@ +660,5 @@
> +    let startPage = readRegKey(Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER,
> +                               kMainKey, "Start Page");
> +    // If the user didn't customize the Start Page, he is still on the default
> +    // page, that may be considered the equivalent of our about:home.  There's
> +    // no reason to retain it, since may be heavily targeted to IE.

...since it is heavily

@@ +663,5 @@
> +    // page, that may be considered the equivalent of our about:home.  There's
> +    // no reason to retain it, since may be heavily targeted to IE.
> +    let homepage = startPage != defaultStartPage ? startPage : "";
> +
> +    // IE7 supports secondary home pages located in a REG_MULTI_SZ key.  These

IE7+?

@@ +664,5 @@
> +    // no reason to retain it, since may be heavily targeted to IE.
> +    let homepage = startPage != defaultStartPage ? startPage : "";
> +
> +    // IE7 supports secondary home pages located in a REG_MULTI_SZ key.  These
> +    // are in addition to the Start Page and no empty entries are possible,

nit: , after "to the Start Page".
Comment 70 Marco Bonardo [::mak] 2012-04-17 05:11:40 PDT
(In reply to Mano from comment #69)
> ::: browser/components/migration/src/IEProfileMigrator.js
> @@ +120,5 @@
> > +function hostIsIPAddress(aHost) {

> This is pretty ugly. Please file a bug on exposing PR_StringToNetAddr.

filed Bug 746104.

> > +      this.__typedURLs = {};
> 
> Looking at the relevant bugs, it seems that on trunk it's safe to use Set. 
> However, if you decide not to so, go with a simple array.

we need a better confirmation of when we can use Set definitely, and array is too slow here, so not touching this.

> I'm unhappy about the number of preferences not imported, but it shouldn't
> block landing. Please file a bug on extending this resource.

Didn't want to go over the scope of this bug.  Bug 719321 will be fine for that.
Comment 71 Marco Bonardo [::mak] 2012-04-17 06:15:29 PDT
Created attachment 615685 [details] [diff] [review]
Part 3: the new JS migrator

Addressed comments, some additional IRC comments, new .resourceTypes constants from MigrationUtils, and a typo breaking history migration.
Comment 73 Marco Bonardo [::mak] 2012-04-17 17:04:49 PDT
Created attachment 615958 [details] [diff] [review]
Part 4: fix the test

Looks like I broke the only test we have, but actually it's the test that is broken.
So I have disabled it with https://hg.mozilla.org/integration/mozilla-inbound/rev/e4aaf4456c6f
and this is the patch to re-enable it. I'm not yet 100% happy with the bookmarks check, suggestions are welcome, but it's better than before.
Comment 74 Marco Bonardo [::mak] 2012-04-17 17:07:46 PDT
ah, the test is failing cause we use Services.appinfo, and there isn't a nsIXULAppInfo in xpcshell (thus the mock I added).
While there I updated the test to MigrationUtils and removed the dependency on default bookmarks migration.
Comment 75 Ed Morley [:emorley] 2012-04-17 18:26:25 PDT
Parts 2-3 + followup to disable test:
https://hg.mozilla.org/mozilla-central/rev/2e3090639b46
https://hg.mozilla.org/mozilla-central/rev/193999908be8
https://hg.mozilla.org/mozilla-central/rev/e4aaf4456c6f

(Leaving open for part 4 patch)
Comment 78 Matthew N. [:MattN] 2015-07-27 22:29:00 PDT
*** Bug 796855 has been marked as a duplicate of this bug. ***

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