1192036 - Import cookies from Microsoft Edge

Status: VERIFIED FIXED

(Firefox :: Migration, defect, P3)

Description

[Justin Dolske [:Dolske]]

We need a migrator that can import a user's existing Edge cookies.

(As a first step we should see what's involved and how difficult it is, this migrator is likely less important to users than other data types.)

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1192036 - import cookies from Edge, r?MattN

Bug 1192036 - import cookies from Edge, r?MattN

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8649934 [details]
MozReview Request: Bug 1192036 - import cookies from Edge, r?MattN

https://reviewboard.mozilla.org/r/16507/#review15331

::: browser/components/migration/MSMigrationUtils.jsm:23
(Diff revision 1)
> +const EDGE_COOKIE_PATH_OPTIONS = ["", "#!001\\", "#!002\\"];

A basic test that checks that at least one of these exist would be good (via .exists probably) like you did for Bookmarks. I'm trying to at least have trivial coverage for more of the migrators as we touch them. These magic numbers with no reference here don't exactly give me confidence that this will work in the future.

::: browser/components/migration/MSMigrationUtils.jsm:27
(Diff revision 1)
> +Cu.importGlobalProperties(["File"]);

I don't see `File` being used but maybe this does some other magic? I think I recently told someone else to remove this.

::: browser/components/migration/MSMigrationUtils.jsm:124
(Diff revision 1)
> + * @param aHost
> + *        The host to check.
> + * @return whether aHost is an IP address.
> + */
> +function hostIsIPAddress(aHost) {

I wondered if this is better in MigrationUtils but there doesn't seem to be helpers like this there yet so *shrug*

::: browser/components/migration/MSMigrationUtils.jsm:310
(Diff revision 1)
> +    if (this._migrationType != MSMigrationUtils.MIGRATION_TYPE_IE)
> +      throw "Shouldn't be looking for a single cookie folder unless we're migrating IE";

Nit: My understanding is that new code should include braces for single-line statements and that we should always throw an Error object instead of a string.

::: browser/components/migration/MSMigrationUtils.jsm:336
(Diff revision 1)
> +    if (this._migrationType != MSMigrationUtils.MIGRATION_TYPE_EDGE)
> +      throw "Shouldn't be looking for multiple cookie folders unless we're migrating Edge";

Ditto

::: browser/components/migration/MSMigrationUtils.jsm:352
(Diff revision 1)
> +    this.__cookiesFolders = folders.length ? folders : null;
> +    return this.__cookiesFolders;

Feel free to put this on one line if you want.

::: browser/components/migration/MSMigrationUtils.jsm:356
(Diff revision 1)
> +  migrate: function C_migrate(aCallback) {

Feel free to switch to newer, less-verbose method syntax in the future while changing blame anyways.

::: browser/components/migration/MSMigrationUtils.jsm:394
(Diff revision 1)
> +    fileReader.addEventListener("loadend", (function onLoadEnd() {

Reminder to rebase this over the switch to an arrow functino.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/42d684a76ece

Comment 4

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #2)
> Comment on attachment 8649934 [details]
> MozReview Request: Bug 1192036 - import cookies from Edge, r?MattN
> 
> https://reviewboard.mozilla.org/r/16507/#review15331
> 
> ::: browser/components/migration/MSMigrationUtils.jsm:23
> (Diff revision 1)
> > +const EDGE_COOKIE_PATH_OPTIONS = ["", "#!001\\", "#!002\\"];
> 
> A basic test that checks that at least one of these exist would be good (via
> .exists probably) like you did for Bookmarks. I'm trying to at least have
> trivial coverage for more of the migrators as we touch them. These magic
> numbers with no reference here don't exactly give me confidence that this
> will work in the future.

Done.

> ::: browser/components/migration/MSMigrationUtils.jsm:27
> (Diff revision 1)
> > +Cu.importGlobalProperties(["File"]);
> 
> I don't see `File` being used but maybe this does some other magic? I think
> I recently told someone else to remove this.

Me! That time you were correct, this time, it is used here: https://hg.mozilla.org/integration/fx-team/rev/42d684a76ece#l3.347

(which is why it was rm'd from the original file - this was the only use)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42d684a76ece

Comment 6

[:Gijs (he/him)]

Comment on attachment 8649934 [details]
MozReview Request: Bug 1192036 - import cookies from Edge, r?MattN

Approval Request Comment
[Feature/regressing bug #]: Also migrate user cookies from Edge
[User impact if declined]: without the yummy cookies from Edge on win10, Firefox will taste a bit bland...
[Describe test coverage new/current, TreeHerder]: like with the other migration code (see bug 1192032 comment 29), only basic testing at this point.
[Risks and why]: low - the risk is pretty much constrained to the IE and Edge cookie migration. The IE migration was broken for ages without people noticing, recently fixed up, and should now be in a good state.
[String/UUID change made/needed]: nope.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8649934 [details]
MozReview Request: Bug 1192036 - import cookies from Edge, r?MattN

This is definitely an important scenario for Firefox end-users. However, given that the change is pretty big, I would like to uplift to Aurora first, and let it sit over the weekend before uplifting to Beta41.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2d041aa5782

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8649934 [details]
MozReview Request: Bug 1192036 - import cookies from Edge, r?MattN

This patch stabilized on Aurora for the last 2 days, seems safe to uplift to Beta. Also this patch will definitely increase the end-user experience on Edge -> Firefox migration so it definitely meets the Beta41 uplift criteria.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/59d7bb89d55d

Comment 11

[Cornel Ionce [:noni] [Hubs QA]]

The cookies are properly imported now on Windows 10 32-bit using:
- latest Nightly, build ID: 20150831030209
- latest Aurora, build ID: 20150831004018
- Firefox 41 beta 6, build ID: 20150831172306