735312 - Still unable to import bookmarks from IE on initial import wizard

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[Marco Bonardo [:mak]]

Looks like even after bug 591884, that fixed toolbar importing, we are failing when trying to import default bookmarks.html file.
investigating.

Comment 1

[Marco Bonardo [:mak]]

we try to import bookmarks.html from the profile folder, but looks like now that file is inside the omni.jar (my guess is this happens from Bug 556644).
In debug builds we commonly use, we don't use the omni.jar, so when ProfDef gets copied, bookmarks.html is, and ends up in the profile folder, then everything works fine.
nsBrowserGlue was updated to import from resource:///defaults/profile/bookmarks.html but the migrators code was not, it still tries to directly read from the profile folder.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Importing a separate bookmarks.html file in the test ended up covering this issue, so I removed it.
The resource address automatically resolves to the right destination, depending on whether omni.ja exists.
Will likely make a try to help testing.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Looks like this is getting rid of the only caller of ImportHTMLFromFileToFolder. Can we remove that too?

Comment 4

[Mozilla RelEng Bot]

Try run for c4cd8ee2197e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c4cd8ee2197e
Results (out of 25 total builds):
    success: 21
    warnings: 1
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c4cd8ee2197e

Comment 5

[Marco Bonardo [:mak]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #3)
> Looks like this is getting rid of the only caller of
> ImportHTMLFromFileToFolder. Can we remove that too?

yes, looks like add-ons (we know about) are not using it. Though I'd prefer doing it as a follow-up, cause I'd still like to backport the patch here to Aurora/Beta while we are at the beginning of the cycle. Will file that bug and attach patch.

(In reply to Mozilla RelEng Bot from comment #4)
>     failure: 3

looks like I forgot to import Util.h, will fix it.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 605593 [details] [diff] [review]
patch v1.1

Vlad, could you preventive check the Windows try builds and tell me if now they correctly import for you?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 605593 [details] [diff] [review]
patch v1.1

Review of attachment 605593 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp
@@ +164,4 @@
>  }
>  
>  nsresult
> +ImportDefaultBookmarks()

CC'ed Mano as I'm not sure if he was using ImportBookmarksHTML in his refactoring

::: browser/components/migration/src/nsIEProfileMigrator.cpp
@@ +1406,5 @@
>    else {
> +    // If importing defaults fails for whatever reason, let the import process
> +    // continue.
> +    DebugOnly<nsresult> rv = ImportDefaultBookmarks();
> +    MOZ_ASSERT(NS_SUCCEEDED(rv), "Should be able to import default bookmarks");

With this check, the NS_ENSURE_SUCCESS(rv, rv) in ImportDefaultBookmarks seems redundant. You could just `return rv` there and keep this check here

::: browser/components/migration/src/nsSafariProfileMigrator.cpp
@@ +980,5 @@
> +    // If importing defaults fails for whatever reason, let the import process
> +    // continue.
> +    DebugOnly<nsresult> rv = ImportDefaultBookmarks();
> +    MOZ_ASSERT(NS_SUCCEEDED(rv), "Should be able to import default bookmarks");
> +

tbpl showed that we'll need to include mozilla/Util.h for DebugOnly here

Comment 8

[Marco Bonardo [:mak]]

(In reply to Felipe Gomes (:felipe) from comment #7)
> Comment on attachment 605593 [details] [diff] [review]
> patch v1.1

> CC'ed Mano as I'm not sure if he was using ImportBookmarksHTML in his
> refactoring

his refactoring is a rewrite, will have to apply this fix to his patch as well (added dependency).

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Unrelated to this fix, but worth following up later is to take a look at tests that are referring to bookmarks.html or these functions: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#413
Some of them might be relying in not using omni.ja{r} for tests which means other things might be broken but passing tests.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Felipe Gomes (:felipe) from comment #9)
> Unrelated to this fix, but worth following up later is to take a look at
> tests that are referring to bookmarks.html or these functions:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> tests/head_common.js#413
> Some of them might be relying in not using omni.ja{r} for tests which means
> other things might be broken but passing tests.

Sure, though most of those are to import from an external file or a pre-places file, not for default bookmarks that is the only case affected by omni.ja. Btw, will look and file eventual follow-ups as needed.

Comment 11

[Vlad [QA]]

Hi Marco
Can you please provide the link to the try builds
Thanks

(In reply to Marco Bonardo [:mak] from comment #6)
> Comment on attachment 605593 [details] [diff] [review]
> patch v1.1
> 
> Vlad, could you preventive check the Windows try builds and tell me if now
> they correctly import for you?

Comment 12

[Marco Bonardo [:mak]]

See comment 4

Comment 13

[Vlad [QA]]

Attachment: screenshot

I've tested using the try build and the favorites from IE9 are imported in Bookmarks Menu but items are not present in a new directory and also are not in the same order as in IE9.
See the screenshot.

Comment 14

[Marco Bonardo [:mak]]

I see, they are imported in alphabetical order.  Btw that's another bug, we should not deal with that here, likely would be better to fix that in the new JS rewrite. What we care here is that all bookmarks are imported to the menu and toolbar.

Comment 15

[Marco Bonardo [:mak]]

I added a note in bug 710895 about alphabetical order.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Vlad [QA] from comment #13)
> items are not present in a new directory


what does this mean?
Note that, when importing from the initial wizard items are places in the menu/toolbar directly.
Instead when importing manually we create an "Imported from BROWSERNAME" folder, and import inside it. this is by (old) design and expected.

Comment 17

[Vlad [QA]]

Forget about it. You just gave me the explanation.
 
(In reply to Marco Bonardo [:mak] from comment #16)
> (In reply to Vlad [QA] from comment #13)
> > items are not present in a new directory
> 
> 
> what does this mean?
> Note that, when importing from the initial wizard items are places in the
> menu/toolbar directly.
> Instead when importing manually we create an "Imported from BROWSERNAME"
> folder, and import inside it. this is by (old) design and expected.

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a10f5ab758b4

Comment 19

[Marco Bonardo [:mak]]

and a follow-up, since I forgot a namespace + minor cleanup.
https://hg.mozilla.org/integration/mozilla-inbound/rev/88112eb6a1f5

Comment 20

[Marco Bonardo [:mak]]

Attachment: patch v1.2

coalesced patch for approval, once we get positive testing in Nightly.

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a10f5ab758b4
https://hg.mozilla.org/mozilla-central/rev/88112eb6a1f5

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 605959 [details] [diff] [review]
patch v1.2

[Approval Request Comment]
Regression caused by (bug #): bug 556644
User impact if declined: As expressed in the original bug 591884, our rate conversion from IE is declining, and this may be one of the failure points. In bug 591884 I thought this was just a minor bug related to the toolbar, but instead we really don't import ANY bookmark from IE in the original migrator.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): The change is limited and explicitly removes an error check (converted to an assertion) to let the migrator proceed in any case. That said, this couldn't be more broken than it is now.
String changes made by this patch: none

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 605959 [details] [diff] [review]
patch v1.2

(In reply to Marco Bonardo [:mak] from comment #22)
> Risk to taking this patch (and alternatives if risky): The change is limited
> and explicitly removes an error check (converted to an assertion) to let the
> migrator proceed in any case. That said, this couldn't be more broken than
> it is now.

Given that, approving for Aurora 13 and Beta 12.

Comment 24

[Marco Bonardo [:mak]]

Thanks.
https://hg.mozilla.org/releases/mozilla-aurora/rev/245b87d73a55
https://hg.mozilla.org/releases/mozilla-beta/rev/32515f50755a

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (AAv1) Remove MIGRATION_BUNDLE leftover [Checked in: Comment 27]

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 609119 [details] [diff] [review]
(AAv1) Remove MIGRATION_BUNDLE leftover
[Checked in: Comment 27]

Review of attachment 609119 [details] [diff] [review]:
-----------------------------------------------------------------

the patch is technically correct, though note that we are about to kill these files really shortly (As soon as the new Safari and IE js migrators land), so I'd not spend time in cleaning them up perfectly.

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 609119 [details] [diff] [review]
(AAv1) Remove MIGRATION_BUNDLE leftover
[Checked in: Comment 27]

https://hg.mozilla.org/mozilla-central/rev/a996d3df12a3

Comment 28

[Paul Silaghi, QA [:pauly]]

Changing the flag to verified on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0 beta 3

All the favorites are imported in alphabetical order from IE9.

Comment 29

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120426 Firefox/14.0a2 
Built from http://hg.mozilla.org/releases/mozilla-aurora/rev/7525533b3e42

Verified that the Favorites are imported from IE9, but the folder "Websites for United States" (which is a default Favorites folder) is imported as "Links for United States" in Firefox

Comment 30

[Marco Bonardo [:mak]]

(In reply to Mihaela Velimiroviciu [QA] from comment #29)
> Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20120426 Firefox/14.0a2 
> Built from http://hg.mozilla.org/releases/mozilla-aurora/rev/7525533b3e42
> 
> Verified that the Favorites are imported from IE9, but the folder "Websites
> for United States" (which is a default Favorites folder) is imported as
> "Links for United States" in Firefox

may depend on the Windows localization? We just read folder names, maybe that is a special folder that gets a different translation in the IE UI only.

Comment 31

[Paul Silaghi, QA [:pauly]]

I don't see any favorite folder called "Websites for United States" in my IE 9.0.8112.16421.
All bookmarks are successfully imported in FF 13b3: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Verified fixed