1192032 - Import bookmarks / favorites from Microsoft Edge

Status: VERIFIED FIXED

(Firefox :: Migration, defect, P2)

Description

[Justin Dolske [:Dolske]]

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

Comment 1

[Justin Dolske [:Dolske]]

Edge also has a Reading List. We have Pocket, but importing to Pocket would be rather complicated. I think as part of the Favorites import, we should also import a "Reading List" folder with the user's Edge Reading List items.

Comment 2

[Marco Bonardo [:mak]]

We do the same for Safari, we create a "Reading List (From Safari)" folder in the bookmarks menu, see importedSafariReadingList

Comment 3

[:Gijs (he/him)]

I've started looking at this.

Good news: once you find the right folder, favourites seem to be stored in exactly the same format + layout as they were under IE. I've not tried finding the reading list stuff yet, and I think in principle that could be a followup bug.

Bad news: bug 1163644 comment #9 and bug 1163644 comment #12 seem to not be particularly useful, as best I can tell, in that getting the SID for the app requires the "family name" which includes the useless goop (on win10 public release that seems to be _8wekyb3d8bbwe) at the end. The goop seems to be the same everywhere, judging by the number of hits on Google. It seems to have been different when Edge was still "Spartan". I don't know if it will change in the future, but I am worried about hardcoding it.

While we could try to use the firewall APIs (see bug 1163644 comment #12) to enumerate app containers and find it that way, I think the simplest thing that would work here is to just grab the local app data folder, smack "Packages" on and enumerate subfolders (if possible, only those starting with Microsoft.MicrosoftEdge), and then find the sub(subsubsub...) folder we want.

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> getting the SID for the app
> requires the "family name" which includes the useless goop (on win10 public
> release that seems to be _8wekyb3d8bbwe) at the end.

The same goes for the package query APIs described in https://msdn.microsoft.com/en-us/library/windows/desktop/hh446597%28v=vs.85%29.aspx . I mean, that or a process ID, but obviously we can't assume Edge is running, and starting it sounds weird and would maybe be a bit of a chicken-egg problem anyway.

Comment 5

[Masatoshi Kimura [:emk]]

Edge definitely changed the (In reply to :Gijs Kruitbosch from comment #3)
> Bad news: bug 1163644 comment #9 and bug 1163644 comment #12 seem to not be
> particularly useful, as best I can tell, in that getting the SID for the app
> requires the "family name" which includes the useless goop (on win10 public
> release that seems to be _8wekyb3d8bbwe) at the end. The goop seems to be
> the same everywhere, judging by the number of hits on Google. It seems to
> have been different when Edge was still "Spartan". I don't know if it will
> change in the future, but I am worried about hardcoding it.

It was the same everywhere and changed when "Spartan" was changed to Edge. See the blog post from Microsoft:
http://blogs.windows.com/bloggingwindows/2015/06/19/upcoming-changes-to-windows-10-insider-preview-builds/
> In the next build we release to Windows Insiders in the Fast ring, the “Project Spartan”
> name will officially change to Microsoft Edge. One result of this naming means that the
> Microsoft Edge app has a new app ID. This will cause any favorites, cookies, history and
> Reading list items that you had saved in the Project Spartan app to be lost after upgrading
> from a previous Windows 10 Insider Preview build. If you want to keep these, you will need
> to back up your favorites before the next flight! To save your favorites, follow these
> steps before upgrading to the next build we release (do it now):
> - Copy your favorites from %localappdata%/Packages/Microsoft.Windows.Spartan_cw5n1h2txyewy/AC/Spartan/User/Default/Favorites.
> - Save them to %userprofile%/Favorites.
> - After upgrading to the next build open Microsoft Edge, choose Settings, and you’ll see an
> option to import favorites from another browser. Choose Internet Explorer to import the
> favorites you saved in your %userprofile% directory into Microsoft Edge.

I think we can expect it will not be changed again because otherwise users will lose the bookmarks (and other settings) again.

Moreover, we already depend on a hard-coded app ID:
https://mxr.mozilla.org/mozilla-central/source/browser/components/shell/nsWindowsShellService.cpp?rev=7c48a3782efe&mark=875-875#862

Comment 6

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1192032 - add edge bookmarks/favorites migrator, r?MattN

Bug 1192032 - add edge bookmarks/favorites migrator, r?MattN

Comment 7

[:Gijs (he/him)]

Comment on attachment 8646421 [details]
MozReview Request: Bug 1192032 - add edge bookmarks/favorites migrator, r?MattN

Bug 1192032 - add edge bookmarks/favorites migrator, r?MattN

Comment 8

[Matthew N. [:MattN]]

Sorry for not getting to this review today… it was probably the day with the most meetings in my life so far…

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8646421 [details]
MozReview Request: Bug 1192032 - add edge bookmarks/favorites migrator, r?MattN

https://reviewboard.mozilla.org/r/15735/#review14207

Looks good!

::: browser/components/migration/EdgeProfileMigrator.js:13
(Diff revision 2)
> +function EdgeProfileMigrator()
> +{

Nit: brace on the same line please. The IE migrator was the only one using this incorrect style.

::: browser/components/migration/EdgeProfileMigrator.js:19
(Diff revision 2)
> +EdgeProfileMigrator.prototype.getResources = function Edge_getResources() {

Nit: My understanding is that you don't need to name the function here anymore.

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

I'm not familiar with this global and I also don't see it being used.

::: browser/components/migration/MSMigrationUtils.jsm:30
(Diff revision 2)
> +  get exists() !!this._favoritesFolder,

Could you at least add a sanity check test that ensures that `.exists` returns true for MIGRATION_TYPE_EDGE? Since we can't use skip-if AFAIK to skip this on <Win10, have the test do a Windows version check at runtime and bail saying it's not applicable. I realize this test will always bail in automation now since we don't have Win10 test machines yet but it will at least catch issues for developers locally and in the future when the test machines are running Win10 or later.

::: browser/components/migration/MSMigrationUtils.jsm:38
(Diff revision 2)
> +        let favoritesFolder = Services.dirsvc.get("Favs", Ci.nsIFile);

It's surprising to me that the "Favorites" directory still exists in the user's home directory on Windows 10 but doesn't contain their Edge Favorites (even when Edge is set as the default browser). This seems like a weird decision on MSFT's part but okay.

::: browser/components/migration/MSMigrationUtils.jsm:167
(Diff revision 2)
> +  MIGRATION_TYPE_IE: 0,

Nit: 0 is falsy which makes me worry someone will accidentally go into an error condition when migrating IE.
e.g.  if (!migrationType)…

I would start numbering at 1 to avoid this

::: browser/components/migration/MSMigrationUtils.jsm:169
(Diff revision 2)
> +  getBookmarksMigrator(migrationType=this.MIGRATION_TYPE_IE) {

Nit: spaces around operators

::: browser/components/migration/MigrationUtils.jsm:499
(Diff revision 2)
> -      "firefox", "ie", "chrome", "chromium", "safari", "360se", "canary"
> +      "firefox", "ie", "edge", "chrome", "chromium", "safari", "360se", "canary"

I think Edge should come before IE since Edge is the default when it's available.

::: browser/components/migration/content/migration.xul:37
(Diff revision 2)
>        <radio id="ie"        label="&importFromIE.label;"        accesskey="&importFromIE.accesskey;"/>
> +      <radio id="edge"      label="&importFromEdge.label;"      accesskey="&importFromEdge.accesskey;"/>

I think these get dynamically re-ordered based on the order in the migratorKeysOrdered(?) array but can you double-check that. Just in case and for consistency, can you please put Edge before IE for the same reason as my comment on migratorKeysOrdered?

Comment 10

[Pulsebot]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7308129e3aca

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

The follow-up patch that landed included changes to browser/themes/windows/downloads/indicator.css that appear unintentional. Can you back out those changes or add a note in this bug why they were necessary?

Comment 13

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> The follow-up patch that landed included changes to
> browser/themes/windows/downloads/indicator.css that appear unintentional.
> Can you back out those changes or add a note in this bug why they were
> necessary?

Ugh. Sorry.

remote:   https://hg.mozilla.org/integration/fx-team/rev/404d17fdf3b2

Comment 14

[Pulsebot]

https://hg.mozilla.org/mozilla-central/rev/4e883591bb5d

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a8ef04862821
https://hg.mozilla.org/mozilla-central/rev/7308129e3aca
https://hg.mozilla.org/mozilla-central/rev/4e883591bb5d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/404d17fdf3b2

Comment 17

[:Gijs (he/him)]

Florin, how do I get this prioritized for verification? I'd like to uplift this, but I'd like to make sure I'm not regressing anything in terms of IE favorites/bookmarks migration.

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to :Gijs Kruitbosch from comment #17)
> Florin, how do I get this prioritized for verification? I'd like to uplift
> this, but I'd like to make sure I'm not regressing anything in terms of IE
> favorites/bookmarks migration.

Just a needinfo on me is enough. Keeping it on to verify this as soon as we get the time.

Comment 19

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

I don't see Microsoft Edge (bug 1163644) in the latest Nightly (build ID: 20150818030209) importer.
I've tried on two machines running Windows 10 64-bit.

Importing from IE seems to work only for the Menu bookmarks, a "From Internet Explorer" folder is created and the bookmarks are properly imported.
The Toolbar Bookmarks are *not* imported, the "From Internet Explorer" folder is created, but it's empty.

Comment 20

[:Gijs (he/him)]

(In reply to Cornel Ionce [QA] from comment #19)
> I don't see Microsoft Edge (bug 1163644) in the latest Nightly (build ID:
> 20150818030209) importer.
> I've tried on two machines running Windows 10 64-bit.

Do they actually have bookmarks/favorites stored in Edge? The migrator will only show up if there's anything to import.

> Importing from IE seems to work only for the Menu bookmarks, a "From
> Internet Explorer" folder is created and the bookmarks are properly imported.
> The Toolbar Bookmarks are *not* imported, the "From Internet Explorer"
> folder is created, but it's empty.

Please can you file a separate bug for that, and let me know what the folder for the toolbar bookmarks is called on those machines? Should just be "Links", but maybe this has changed since that migrator was written?

Comment 21

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #20)
> (In reply to Cornel Ionce [QA] from comment #19)
> > Importing from IE seems to work only for the Menu bookmarks, a "From
> > Internet Explorer" folder is created and the bookmarks are properly imported.
> > The Toolbar Bookmarks are *not* imported, the "From Internet Explorer"
> > folder is created, but it's empty.
> 
> Please can you file a separate bug for that, and let me know what the folder
> for the toolbar bookmarks is called on those machines? Should just be
> "Links", but maybe this has changed since that migrator was written?

Actually, still interested in this, which shouldn't be affected by bug 1196311.

Comment 22

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

Filled bug 1197112 and cc'ed you.

Comment 23

[:Gijs (he/him)]

Cornel, did you have a chance to verify the Edge migration here?

Comment 24

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

I've tested the Edge migration and  the bookmarks/favorites are properly imported, being the only option that can be imported for now: http://i.imgur.com/9Ava3Ee.png

The thumbnails of the imported favorites are not displayed until the user visits them.

The bookmarks from the toolbar are sent into a "From Microsoft Edge" folder while the ones from the bookmarks menu are imported into "From Microsoft Edge" - "Edge" folders: http://i.imgur.com/FortKdX.png
Is this the desired behavior? I think a single "From Microsoft Edge" folder is enough for them too.

Comment 25

[:Gijs (he/him)]

(In reply to Cornel Ionce [QA] from comment #24)
> I've tested the Edge migration and  the bookmarks/favorites are properly
> imported, being the only option that can be imported for now:
> http://i.imgur.com/9Ava3Ee.png

Right.

> The thumbnails of the imported favorites are not displayed until the user
> visits them.

Yes, see also bug 718220.

> The bookmarks from the toolbar are sent into a "From Microsoft Edge" folder
> while the ones from the bookmarks menu are imported into "From Microsoft
> Edge" - "Edge" folders: http://i.imgur.com/FortKdX.png
> Is this the desired behavior? I think a single "From Microsoft Edge" folder
> is enough for them too.

I don't see this on my machine. Can you file a bug and include your windows and edge version, and ensure that your bookmarks aren't themselves in a folder called Edge? :-)

Comment 26

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #25)
> and ensure that your bookmarks aren't themselves in a
> folder called Edge? :-)

... in edge, that is? :-)

The bookmarks migration will import the folder structure as it exists in the Favorites folder that Edge uses.

Comment 27

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

My bad, you were right. The favorites contained an "Edge" folder.
I've retested with several folders containing bookmarks and bookmarks saved without a folder. The entire structure was properly imported.

I've tested with Windows 10 Insider build 10525 and Edge 20.10525.0.0

Comment 28

[:Gijs (he/him)]

(In reply to Cornel Ionce [QA] from comment #27)
> My bad, you were right. The favorites contained an "Edge" folder.
> I've retested with several folders containing bookmarks and bookmarks saved
> without a folder. The entire structure was properly imported.
> 
> I've tested with Windows 10 Insider build 10525 and Edge 20.10525.0.0

Perfect, thanks! Let's call this verified, then.

Comment 29

[:Gijs (he/him)]

Attachment: Patch for aurora uplift

Approval Request Comment
[Feature/regressing bug #]: Start doing migration of user data from Edge
[User impact if declined]: It's not possible to migrate data out of Edge
[Describe test coverage new/current, TreeHerder]: there is some basic test coverage that (for now, with no win10 machines on treeherder) will only check that we don't expose the migrator on older versions of Windows. Once win10 comes online it'll check that that is working. There are also pre-existing tests for the IE favorites migration on which this is based.
[Risks and why]: very low. The impact should be restricted to Edge migration. In principle this patch moved some code used for IE migration. Bug 1197112 was reported when testing this, though it isn't clear if that is a regression from this bug or not (and it doesn't reproduce everywhere).
[String/UUID change made/needed]: Nope

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8654091 [details] [diff] [review]
Patch for aurora uplift

Verified patch, important feature, taking it to aurora. Ritu will make the call for 42.

Comment 31

[:Gijs (he/him)]

Comment on attachment 8654091 [details] [diff] [review]
Patch for aurora uplift

I just realized that beta will need hardcoded strings, and the patch has conflicts there because we didn't support canary/chromium migration. The strings were prelanded for 42 and applies there, so neither is an issue for Aurora.

I'll try to get a beta patch ready and put up but I don't know that I'll manage it still today (and I'm on PTO next week).

Comment 32

[:Gijs (he/him)]

Attachment: Patch for beta

Approval Request Comment: see comment 29

Built and tested the hardcoded strings on beta. :-)

Comment 33

[:Gijs (he/him)]

(In reply to Gijs Kruitbosch (PTO until Sep 6) from comment #32)
> Created attachment 8654205 [details] [diff] [review]
> Patch for beta
> 
> Approval Request Comment: see comment 29
> 
> Built and tested the hardcoded strings on beta. :-)

Actually, considering the strings, let me update that part of the approval request to be thorough:


[String/UUID change made/needed]: 

There are no new strings in the patch.

The patch would normally have needed new strings. It would have needed those for two things:
1) the name of the browser we're importing from
2) the names of the things we're importing (for this patch: "Favorites", for some of the other IE import work, potentially things like "Cookies" or "Passwords").

I added a small bit of code to use the strings from IE for (2) (because the names should be identical, and to hardcode the name of the browser to use "Microsoft Edge". 

The name of the browser doesn't, as far as we know, change between different Windows locales, so I believe this is OK and isn't an issue.

Using the IE names for things like bookmarks and cookies and so on, considering they are all standalone nouns without being in a sentence or something like that, should in principle work everywhere unless the translations end up relating to the browser name because of language differences with English.

Either way, I think given the choice between not having the feature anywhere, and having it but having the potential for imperfect translations for the names of the resources to import, I believe we should pick the latter.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/98983a6fd50d

Comment 35

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

Gijs, should we file a separate bug for new strings that may be needed instead of the string from IE that we used? We can fix that in Nightly and let it ride the trains.

Comment 36

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

Comment on attachment 8654205 [details] [diff] [review]
Patch for beta

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

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0eee84804240

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ritu Kothari (:ritu) from comment #35)
> Gijs, should we file a separate bug for new strings that may be needed
> instead of the string from IE that we used? We can fix that in Nightly and
> let it ride the trains.

We don't need a follow-up for new strings. The strings exist on mozilla-central, they just weren't available on the beta branch.

Comment 39

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

I'm also confirming this fix for the latest Aurora (build ID: 20150831004018) and Firefox 41 beta 6 (build ID: 20150831172306).
Tested on a computer with Windows 1- 32-bit and a MS Pro 2 device running Windows 10 64-bit.