1205053 - Use registry typedURLs list to import rudimentary history from Microsoft Edge

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P2)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1192034 +++

In bug 1192034 we discovered that fully importing the history from Edge is Very Hard. Just reading the URLs typed in the URL bar is easier, so let's start with that for now.

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1205053 - use registry typedURLs to import rudimentary history from MS Edge, r?dolske

Bug 1205053 - use registry typedURLs to import rudimentary history from MS Edge, r?dolske

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 8661711 [details]
MozReview Request: Bug 1205053 - use registry typedURLs to import rudimentary history from MS Edge, r?dolske

https://reviewboard.mozilla.org/r/19373/#review17881

LGTM, although would be good to clean up the last "value or true" issue.

::: browser/components/migration/IEProfileMigrator.js:74
(Diff revision 1)
> +      let lastVisitTime = entry.get("time") || (Date.now() * 1000);

Oof, nice catch.

Also, it looks like I've got an actual entry on my box with a timestamp of 0. (It's a longer URL that I'm pretty sure I pasted.), so the fallback to Date.now is good to have even if we have an entry.

::: browser/components/migration/MSMigrationUtils.jsm:574
(Diff revision 1)
> +                           Ci.nsIWindowsRegKey.ACCESS_READ);

Hmm, looks like IE has this too, but we didn't previously use it.

Edge also has a TypedURLsVisitCount, should we pull that out too?

::: browser/components/migration/MSMigrationUtils.jsm:579
(Diff revision 1)
> +    for (let entry = 1; typedURLKey.hasValue((entryName = "url" + entry)); entry++) {

Because I was curious... Removing a history entry in Edge does seem to renumber other entries, so it's always  contigious list.

::: browser/components/migration/MSMigrationUtils.jsm:583
(Diff revision 1)
> +        let urlTime = typedURLTimeKey.readBinaryValue(entryName);

Does readInt64Value() work here? Although it's a REG_BINARY value, so that would technically be a type mismatch...

::: browser/components/migration/MSMigrationUtils.jsm:601
(Diff revision 1)
> +      typedURLs.set(url, timeTyped || true);

This is a weird pattern. Value or true?

Seems like we should either always ensure a time value here (actual of Date.now() fudged), or return 0 if we want to specifically indicate we didn't find a value (and let the caller fudge things).

Comment 3

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #2)
> ::: browser/components/migration/MSMigrationUtils.jsm:574
> (Diff revision 1)
> > +                           Ci.nsIWindowsRegKey.ACCESS_READ);
> 
> Hmm, looks like IE has this too, but we didn't previously use it.

Right, we use the actual history data in that case.

> Edge also has a TypedURLsVisitCount, should we pull that out too?

I'm not really sure how to use that. AFAICT we store visits with timestamps, and so weighting the visit count appropriately given the timestamps would be interesting. I'll file a followup to discuss this further.

> ::: browser/components/migration/MSMigrationUtils.jsm:579
> (Diff revision 1)
> > +    for (let entry = 1; typedURLKey.hasValue((entryName = "url" + entry)); entry++) {
> 
> Because I was curious... Removing a history entry in Edge does seem to
> renumber other entries, so it's always  contigious list.
> 
> ::: browser/components/migration/MSMigrationUtils.jsm:583
> (Diff revision 1)
> > +        let urlTime = typedURLTimeKey.readBinaryValue(entryName);
> 
> Does readInt64Value() work here? Although it's a REG_BINARY value, so that
> would technically be a type mismatch...

Yes and no. It doesn't throw an exception, but we can't then use bitwise operators still because the JS bitwise operators assume 32-bit numbers. Something something nice things something.

I was going to still do this because it cleans the code up a little, and then I found out that this actually drops bits because JS' representation is not as accurate:

> key.readInt64Value("url1")
< 130852543042568560
> key.readInt64Value("url1").toString(16)
< "1d0e1b1d2024170"
> (key.readInt64Value("url1") + 1).toString(16)
< "1d0e1b1d2024170"

and the result of using toString(16) on the int64 value is 2 off from the result of using the binary blob thing, which makes sense because looking in the registry, the final byte is 0x72, not 0x70.

So I ended up leaving this as-is. Let me know / file a followup if you think this needs addressing still.


> ::: browser/components/migration/MSMigrationUtils.jsm:601
> (Diff revision 1)
> > +      typedURLs.set(url, timeTyped || true);
> 
> This is a weird pattern. Value or true?
> 
> Seems like we should either always ensure a time value here (actual of
> Date.now() fudged), or return 0 if we want to specifically indicate we
> didn't find a value (and let the caller fudge things).

Fixed.

 https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=417e00a78107

Comment 4

[Pulsebot]

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

Comment 5

[Pulsebot]

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

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/358949058ff8
https://hg.mozilla.org/mozilla-central/rev/76697bfb343c

Comment 7

[:Gijs (he/him)]

Comment on attachment 8661711 [details]
MozReview Request: Bug 1205053 - use registry typedURLs to import rudimentary history from MS Edge, r?dolske

Approval Request Comment
[Feature/regressing bug #]: win10 edge user data migration
[User impact if declined]: users can't get any kind of history out of MS Edge when they switch to Firefox. It also contains some improvements in fallbacks for other imports for IE that users would otherwise miss out on.
[Describe test coverage new/current, TreeHerder]: very limited sanity test on win10 which doesn't run on treeherder (bug 1192842 and friends). The patch also moves some code which was used for IE and which might have independent test coverage that does run on treeherder
[Risks and why]: low - the patch reuses existing code that we used for IE, and serious issues would have been found already (it's baked a while now) and would be limited in impact anyway.
[String/UUID change made/needed]: nope

Comment 8

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch from comment #7)

> Approval Request Comment
> [Feature/regressing bug #]: win10 edge user data migration
> [User impact if declined]: users can't get any kind of history out of MS
> Edge when they switch to Firefox. It also contains some improvements in
> fallbacks for other imports for IE that users would otherwise miss out on.

I'd also add that this is the last major piece of Edge migrator work we've been working on, and so are very keen to have 42 as the release with this feature basically done/complete.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8661711 [details]
MozReview Request: Bug 1205053 - use registry typedURLs to import rudimentary history from MS Edge, r?dolske

Taking it to complete the edge import support, taking it. Should be 42 beta 5.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/52a759d29584
https://hg.mozilla.org/releases/mozilla-aurora/rev/8b59e77a2c71

Comment 11

[Carsten Book [:Tomcat]]

Hi Gijs, this cause problems when uplifting to beta like :

grafting 302968:358949058ff8 "Bug 1205053 - use registry typedURLs to import rudimentary history from MS Edge, r=dolske"
merging browser/components/migration/EdgeProfileMigrator.js
merging browser/components/migration/IEProfileMigrator.js
merging browser/components/migration/MSMigrationUtils.jsm
warning: conflicts during merge.
merging browser/components/migration/MSMigrationUtils.jsm incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue

Comment 12

[:Gijs (he/him)]

Collapsed into a single rev and pushed:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/42d71a9d572a