Closed Bug 1192036 Opened 4 years ago Closed 4 years ago

Import cookies from Microsoft Edge

Categories

(Firefox :: Migration, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 43
Tracking Status
firefox41 --- verified
firefox42 --- verified
firefox43 --- verified

People

(Reporter: Dolske, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.)
Priority: -- → P3
Bug 1192036 - import cookies from Edge, r?MattN
Attachment #8649934 - Flags: review?(MattN+bmo)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
Attachment #8649934 - Flags: review?(MattN+bmo) → review+
(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)
https://hg.mozilla.org/mozilla-central/rev/42d684a76ece
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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.
Attachment #8649934 - Flags: approval-mozilla-beta?
Attachment #8649934 - Flags: approval-mozilla-aurora?
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.
Attachment #8649934 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8649934 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Contact: cornel.ionce
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
Depends on: 1200598
Depends on: 1358549
You need to log in before you can comment on or make changes to this bug.