Still unable to import bookmarks from IE on initial import wizard

VERIFIED FIXED in Firefox 12

Status

()

defect
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({regression})

Trunk
Firefox 14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12 verified, firefox13 verified)

Details

(Whiteboard: [qa!])

Attachments

(3 attachments, 1 obsolete attachment)

Looks like even after bug 591884, that fixed toolbar importing, we are failing when trying to import default bookmarks.html file.
investigating.
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.
Posted patch patch v1.1 (obsolete) — Splinter Review
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.
Attachment #605593 - Flags: review?(felipc)
Looks like this is getting rid of the only caller of ImportHTMLFromFileToFolder. Can we remove that too?
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/mak77@bonardo.net-c4cd8ee2197e
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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 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?
Attachment #605593 - Flags: review?(felipc) → feedback?(vlad.ghetiu)
Blocks: 735625
Blocks: 718608
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
Attachment #605593 - Flags: review+
(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).
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.
(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.
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?
Posted image 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.
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.
I added a note in bug 710895 about alphabetical order.
(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.
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.
Attachment #605593 - Flags: feedback?(vlad.ghetiu)
Blocks: 556644
Flags: in-testsuite+
Keywords: regression
and a follow-up, since I forgot a namespace + minor cleanup.
https://hg.mozilla.org/integration/mozilla-inbound/rev/88112eb6a1f5
Posted patch patch v1.2Splinter Review
coalesced patch for approval, once we get positive testing in Nightly.
Attachment #605593 - Attachment is obsolete: true
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
Attachment #605959 - Flags: approval-mozilla-beta?
Attachment #605959 - Flags: approval-mozilla-aurora?
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.
Attachment #605959 - Flags: approval-mozilla-beta?
Attachment #605959 - Flags: approval-mozilla-beta+
Attachment #605959 - Flags: approval-mozilla-aurora?
Attachment #605959 - Flags: approval-mozilla-aurora+
Depends on: 736504
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.
Attachment #609119 - Flags: review?(mak77) → review+
Comment on attachment 609119 [details] [diff] [review]
(AAv1) Remove MIGRATION_BUNDLE leftover
[Checked in: Comment 27]

https://hg.mozilla.org/mozilla-central/rev/a996d3df12a3
Attachment #609119 - Attachment description: (AAv1) Remove MIGRATION_BUNDLE leftover → (AAv1) Remove MIGRATION_BUNDLE leftover [Checked in: Comment 27]
Whiteboard: [qa+]
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.
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
(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.
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
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.