Closed Bug 1226556 Opened 6 years ago Closed 5 years ago

Fix Microsoft Edge bookmark import now that they are stored in EDB database (like Reading List)

Categories

(Firefox :: Migration, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox45 --- verified
firefox46 --- verified
firefox47 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from bug 1225466 comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Can you elaborate on [the failed bookmark import]? When did you create the bookmarks and when did
> > you delete them? Your comment didn't specify.
> 
> Before performing the migration process I deleted a few favorites and added
> new ones in Edge, but in Firefox are imported another bookmarks, such as
> very old ones.
> See screenshot: http://i.imgur.com/HMcT6cy.jpg

We should look at what's going on here.
Flags: needinfo?(gijskruitbosch+bugs)
It looks like favorites are now no longer saved in the location where they used to be saved. I expect Edge is now using the same database as it is for History (see bug 1192034).
(In reply to :Gijs Kruitbosch from comment #1)
> It looks like favorites are now no longer saved in the location where they
> used to be saved. I expect Edge is now using the same database as it is for
> History (see bug 1192034).

Hm, looks like it's actually the same db as the reading list rather than webcache. Which is good... but still is going to take work to get the import right here...
Assignee: nobody → gijskruitbosch+bugs
Flags: needinfo?(gijskruitbosch+bugs)
Summary: Investigate issues with bookmark import from Microsoft Edge → Microsoft Edge bookmarks are now stored in EDB database (like Reading List)
Summary: Microsoft Edge bookmarks are now stored in EDB database (like Reading List) → Fix Microsoft Edge bookmark import now that they are stored in EDB database (like Reading List)
Depends on: 1229076
Comment on attachment 8714100 [details]
MozReview Request: Bug 1226556 - part 1: allow reading GUID columns in ESEDBReader.jsm, r=MattN

https://reviewboard.mozilla.org/r/32931/#review29923
Attachment #8714100 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8714101 [details]
MozReview Request: Bug 1226556 - part 2: use ESE database to import Edge bookmarks, r=MattN

https://reviewboard.mozilla.org/r/32933/#review29925

The more we touch ESE code the more I wish we implemented tests… please make sure this gets manual verification and continues to be tested by QE during the Beta cycle. I guess we also should uplift this somewhat after verification.

::: browser/components/migration/EdgeProfileMigrator.js:47
(Diff revision 1)
> + * @param tableName the name of the table to read.
> + * @param columns   a list of column specifiers (see ESEDBReader.jsm) or a function that
> + *                  generates them based on the database reference once opened.
> + * @param filterFn  a function that is called for each row. Only rows for which it returns

Can you add types to the JSDoc e.g.:
@param {String[]|function} columns…

::: browser/components/migration/EdgeProfileMigrator.js:367
(Diff revision 1)
> -    MSMigrationUtils.getBookmarksMigrator(MSMigrationUtils.MIGRATION_TYPE_EDGE),
> +    new EdgeBookmarksMigrator(),

Do we need to support users on older versions of Windows/Edge? If so, we could try the new code path and fallback to old. If not, we should at least comment on MSMigrationUtils.jsm Bookmark constructor to note that MIGRATION_TYPE_EDGE is no longer used.
Attachment #8714101 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] from comment #6)
> Do we need to support users on older versions of Windows/Edge?

I thought the answer was "no", but it seems the update from an older Windows version to Windows 10 will initially not get you the November update where I believe all this changed. See the note at the top of:

http://windows.microsoft.com/en-gb/windows-10/upgrade-to-windows-10-faq

So I will likely have to make some way of determining what's going on and picking the right bookmarks implementation. :-(

As for tests, I just checked, and it seems that while these databases are huge, they compress down to about 60k-odd with just plain windows put-in-zip compression, so hopefully I can write some tests that use that to get reasonable-size testcases in the tree, or something.
Attachment #8714100 - Attachment description: MozReview Request: Bug 1226556 - part 1: allow reading GUID columns in ESEDBReader.jsm, r?MattN → MozReview Request: Bug 1226556 - part 1: allow reading GUID columns in ESEDBReader.jsm, r=MattN
Comment on attachment 8714100 [details]
MozReview Request: Bug 1226556 - part 1: allow reading GUID columns in ESEDBReader.jsm, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32931/diff/1-2/
Comment on attachment 8714101 [details]
MozReview Request: Bug 1226556 - part 2: use ESE database to import Edge bookmarks, r=MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32933/diff/1-2/
Attachment #8714101 - Attachment description: MozReview Request: Bug 1226556 - part 2: use ESE database to import Edge bookmarks, r?MattN → MozReview Request: Bug 1226556 - part 2: use ESE database to import Edge bookmarks, r=MattN
Comment on attachment 8715306 [details]
MozReview Request: Bug 1226556 - part 3: add tableExists to ESEDBReader, use it to fall back to normal bookmarks migration, r?MattN

https://reviewboard.mozilla.org/r/33379/#review30551

Sorry for the delay

::: browser/components/migration/ESEDBReader.jsm:328
(Diff revision 1)
> +    if (!this._opened) {
> +      throw "The database was closed!";
> +    }

I thought we're always supposed to throw an `Error` instead of string so we get a stack. That probably deserves as eslint rule if it's still the case.
Attachment #8715306 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] from comment #11)
> Comment on attachment 8715306 [details]
> MozReview Request: Bug 1226556 - part 3: add tableExists to ESEDBReader, use
> it to fall back to normal bookmarks migration, r?MattN
> 
> https://reviewboard.mozilla.org/r/33379/#review30551
> 
> Sorry for the delay

No worries!

> ::: browser/components/migration/ESEDBReader.jsm:328
> (Diff revision 1)
> > +    if (!this._opened) {
> > +      throw "The database was closed!";
> > +    }
> 
> I thought we're always supposed to throw an `Error` instead of string so we
> get a stack. That probably deserves as eslint rule if it's still the case.

Fixed, as well as other occurrences in this file. And yes, it probably could do with eslint support - though note that none of these files are currently linted. :-(
Ryan, this basically fixes bookmark import from Edge after the November Windows 10 update. I would really like to uplift this (and bug 1229076 and bug 1237679) to 45 (beta, and going to be ESR!), but I would feel better about doing that if it had had some QA attention to verify there aren't gaping holes in what I implemented. Do you think that is feasible?
Flags: needinfo?(ryanvm)
Rares, can you please assign this to someone on your team. Alternatively, we may want Vasilica to look at this since she already has experience testing this feature. I'll leave that to you and Florin to decide :)
Flags: needinfo?(ryanvm) → needinfo?(rares.bologa)
Flags: needinfo?(rares.bologa) → needinfo?(simona.marcu)
I could not reproduce the issue exactly as stated in the Description - unfortunately I didn't had any old bookmarks in Edge to work with.

Here are the results I got:

=>  When migration was done on the latest Nightly (Build ID: 20160209025057), none of the bookmarks saved under Favorites or under Favorites Bar were imported. The Reading list (from Edge) items were the only ones imported under the Bookmarks Menu. 

=> When migration was done on an fx-team build that contained the commits from Comment 13 (downloaded from http://archive.mozilla.org/pub/firefox/tinderbox-builds/fx-team-win32/1455015057/ ), all the bookmarks were imported as following: 
- The Bookmarks saved under Favorites in Edge were imported in Firefox under the Bookmarks Menu.
- The Bookmarks saved under Favorites Bar in Edge were imported in Firefox under the Bookmarks Toolbar section.
- The Reading list items from Edge were also imported under the Bookmarks Menu section.
(In reply to Simona B from comment #17)
> I could not reproduce the issue exactly as stated in the Description -
> unfortunately I didn't had any old bookmarks in Edge to work with.
> 
> Here are the results I got:
> 
> =>  When migration was done on the latest Nightly (Build ID:
> 20160209025057), none of the bookmarks saved under Favorites or under
> Favorites Bar were imported. The Reading list (from Edge) items were the
> only ones imported under the Bookmarks Menu. 
> 
> => When migration was done on an fx-team build that contained the commits
> from Comment 13 (downloaded from
> http://archive.mozilla.org/pub/firefox/tinderbox-builds/fx-team-win32/
> 1455015057/ ), all the bookmarks were imported as following: 
> - The Bookmarks saved under Favorites in Edge were imported in Firefox under
> the Bookmarks Menu.
> - The Bookmarks saved under Favorites Bar in Edge were imported in Firefox
> under the Bookmarks Toolbar section.
> - The Reading list items from Edge were also imported under the Bookmarks
> Menu section.

I mean, works for me! Let's get this sorted for 45, then. Even if we missed something here, working some of the time > working none of the time.
Status: RESOLVED → VERIFIED
Comment on attachment 8715306 [details]
MozReview Request: Bug 1226556 - part 3: add tableExists to ESEDBReader, use it to fall back to normal bookmarks migration, r?MattN

Approval Request Comment
[Feature/regressing bug #]: Windows 10 update broke this, feature being importing bookmarks from Edge
[User impact if declined]: No (or very much outdated) bookmarks imported from Edge
[Describe test coverage new/current, TreeHerder]: nope. :-(
[Risks and why]: low risk. Not because the code is perfect, but because what we're improving upon is "it doesn't work". Improving "it doesn't work" is not hard. The code itself is limited to the Edge importer, so it's hard to imagine it would have an impact outside that.
[String/UUID change made/needed]: no

Note that I'm requesting uplift for all 3 patches on this bug, and will also be requesting uplift for 2 bugs that are required to make this work right: bug 1237679 and bug 1229076.
Attachment #8715306 - Flags: approval-mozilla-beta?
Attachment #8715306 - Flags: approval-mozilla-aurora?
I was able to reproduce this issue on Firefox 47.0a1 (2016-02-01) under Windows 10 64-bit.

Verified fixed on Firefox 47.0a1 (2016-02-09) under Windows 10 64-bit. All bookmarks, both old and new ones, are successfully imported from Edge.
Flags: needinfo?(simona.marcu)
Comment on attachment 8715306 [details]
MozReview Request: Bug 1226556 - part 3: add tableExists to ESEDBReader, use it to fall back to normal bookmarks migration, r?MattN

Improve the import, taking it.
Should be in 45 beta 5
Attachment #8715306 - Flags: approval-mozilla-beta?
Attachment #8715306 - Flags: approval-mozilla-beta+
Attachment #8715306 - Flags: approval-mozilla-aurora?
Attachment #8715306 - Flags: approval-mozilla-aurora+
Attachment #8714101 - Flags: approval-mozilla-beta+
Attachment #8714101 - Flags: approval-mozilla-aurora+
Attachment #8714100 - Flags: approval-mozilla-beta+
Attachment #8714100 - Flags: approval-mozilla-aurora+
This isn't applying correctly to beta. Can we get rebased patches for it?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: qe-verify+
We've also tested this issue using Fx 45 beta 8 (build ID 20160221141421) and 2015-02-23 Aurora 46.0a2 on Windows 10x64.
Every bookmark is now imported correctly from Edge, so we can consider this issue verified.
Flags: qe-verify+
QA Contact: cornel.ionce
Depends on: 1344644
Depends on: 1349632
You need to log in before you can comment on or make changes to this bug.