Still unable to import bookmarks from IE on initial import wizard

VERIFIED FIXED in Firefox 12

Status

()

Firefox
Migration
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({regression})

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

Firefox Tracking Flags

(firefox12 verified, firefox13 verified)

Details

(Whiteboard: [qa!])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 605593 [details] [diff] [review]
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.
Attachment #605593 - Flags: review?(felipc)
Looks like this is getting rid of the only caller of ImportHTMLFromFileToFolder. Can we remove that too?

Comment 4

6 years ago
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
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
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)
(Assignee)

Updated

6 years ago
Blocks: 735625
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
(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

6 years ago
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?
(Assignee)

Comment 12

6 years ago
See comment 4

Comment 13

6 years ago
Created attachment 605749 [details]
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.
(Assignee)

Comment 14

6 years ago
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.
(Assignee)

Comment 15

6 years ago
I added a note in bug 710895 about alphabetical order.
(Assignee)

Comment 16

6 years ago
(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

6 years ago
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.
(Assignee)

Comment 18

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a10f5ab758b4
Target Milestone: --- → Firefox 14
(Assignee)

Updated

6 years ago
Attachment #605593 - Flags: feedback?(vlad.ghetiu)
(Assignee)

Updated

6 years ago
Blocks: 556644
Flags: in-testsuite+
Keywords: regression
(Assignee)

Comment 19

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

Comment 20

6 years ago
Created attachment 605959 [details] [diff] [review]
patch v1.2

coalesced patch for approval, once we get positive testing in Nightly.
Attachment #605593 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
https://hg.mozilla.org/mozilla-central/rev/a10f5ab758b4
https://hg.mozilla.org/mozilla-central/rev/88112eb6a1f5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 22

6 years ago
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+
(Assignee)

Comment 24

6 years ago
Thanks.
https://hg.mozilla.org/releases/mozilla-aurora/rev/245b87d73a55
https://hg.mozilla.org/releases/mozilla-beta/rev/32515f50755a
status-firefox12: --- → fixed
status-firefox13: --- → fixed

Updated

6 years ago
Depends on: 736504
Created attachment 609119 [details] [diff] [review]
(AAv1) Remove MIGRATION_BUNDLE leftover
[Checked in: Comment 27]
Attachment #609119 - Flags: review?(mak77)
(Assignee)

Comment 26

6 years ago
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.
status-firefox12: fixed → verified
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
(Assignee)

Comment 30

5 years ago
(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
status-firefox13: fixed → verified
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.