Closed
Bug 735312
Opened 13 years ago
Closed 13 years ago
Still unable to import bookmarks from IE on initial import wizard
Categories
(Firefox :: Migration, defect)
Firefox
Migration
Tracking
()
VERIFIED
FIXED
Firefox 14
People
(Reporter: mak, Assigned: mak)
References
Details
(Keywords: regression, Whiteboard: [qa!])
Attachments
(3 files, 1 obsolete file)
178.34 KB,
image/jpeg
|
Details | |
9.01 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.00 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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•13 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•13 years ago
|
||
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)
Comment 3•13 years ago
|
||
Looks like this is getting rid of the only caller of ImportHTMLFromFileToFolder. Can we remove that too?
Comment 4•13 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•13 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•13 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)
Comment 7•13 years ago
|
||
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•13 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).
Comment 9•13 years ago
|
||
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•13 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•13 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•13 years ago
|
||
See comment 4
Comment 13•13 years ago
|
||
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•13 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•13 years ago
|
||
I added a note in bug 710895 about alphabetical order.
Assignee | ||
Comment 16•13 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•13 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•13 years ago
|
||
Target Milestone: --- → Firefox 14
Assignee | ||
Updated•13 years ago
|
Attachment #605593 -
Flags: feedback?(vlad.ghetiu)
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 19•13 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•13 years ago
|
||
coalesced patch for approval, once we get positive testing in Nightly.
Attachment #605593 -
Attachment is obsolete: true
Assignee | ||
Comment 21•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a10f5ab758b4
https://hg.mozilla.org/mozilla-central/rev/88112eb6a1f5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 22•13 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 23•13 years ago
|
||
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•13 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
Comment 25•13 years ago
|
||
Attachment #609119 -
Flags: review?(mak77)
Assignee | ||
Comment 26•13 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 27•13 years ago
|
||
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]
Comment 28•13 years ago
|
||
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•13 years ago
|
||
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•13 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.
Comment 31•13 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•