Last Comment Bug 735312 - Still unable to import bookmarks from IE on initial import wizard
: Still unable to import bookmarks from IE on initial import wizard
Status: VERIFIED FIXED
[qa!]
: regression
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Marco Bonardo [::mak]
:
Mentors:
Depends on: 591884 736504
Blocks: 556644 718608 735625
  Show dependency treegraph
 
Reported: 2012-03-13 10:40 PDT by Marco Bonardo [::mak]
Modified: 2012-05-16 07:02 PDT (History)
9 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified


Attachments
patch v1.1 (8.38 KB, patch)
2012-03-13 16:38 PDT, Marco Bonardo [::mak]
felipc: review+
Details | Diff | Review
screenshot (178.34 KB, image/jpeg)
2012-03-14 07:46 PDT, Vlad [QA]
no flags Details
patch v1.2 (9.01 KB, patch)
2012-03-14 14:42 PDT, Marco Bonardo [::mak]
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
(AAv1) Remove MIGRATION_BUNDLE leftover [Checked in: Comment 27] (1.00 KB, patch)
2012-03-25 07:42 PDT, Serge Gautherie (:sgautherie)
mak77: review+
Details | Diff | Review

Description Marco Bonardo [::mak] 2012-03-13 10:40:40 PDT
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] 2012-03-13 12:36:56 PDT
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] 2012-03-13 16:38:55 PDT
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.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-13 19:22:03 PDT
Looks like this is getting rid of the only caller of ImportHTMLFromFileToFolder. Can we remove that too?
Comment 4 Mozilla RelEng Bot 2012-03-13 22:33:40 PDT
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
Comment 5 Marco Bonardo [::mak] 2012-03-14 03:42:28 PDT
(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 6 Marco Bonardo [::mak] 2012-03-14 03:50:00 PDT
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 me!) [offline until Jun 24] 2012-03-14 03:55:36 PDT
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] 2012-03-14 04:01:04 PDT
(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 me!) [offline until Jun 24] 2012-03-14 04:03:18 PDT
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] 2012-03-14 04:13:11 PDT
(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] 2012-03-14 07:00:27 PDT
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] 2012-03-14 07:12:10 PDT
See comment 4
Comment 13 Vlad [QA] 2012-03-14 07:46:40 PDT
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.
Comment 14 Marco Bonardo [::mak] 2012-03-14 08:06:23 PDT
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] 2012-03-14 08:26:29 PDT
I added a note in bug 710895 about alphabetical order.
Comment 16 Marco Bonardo [::mak] 2012-03-14 08:29:25 PDT
(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] 2012-03-14 09:33:35 PDT
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 19 Marco Bonardo [::mak] 2012-03-14 14:38:45 PDT
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] 2012-03-14 14:42:45 PDT
Created attachment 605959 [details] [diff] [review]
patch v1.2

coalesced patch for approval, once we get positive testing in Nightly.
Comment 22 Marco Bonardo [::mak] 2012-03-15 08:13:51 PDT
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] 2012-03-15 13:51:56 PDT
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 25 Serge Gautherie (:sgautherie) 2012-03-25 07:42:25 PDT
Created attachment 609119 [details] [diff] [review]
(AAv1) Remove MIGRATION_BUNDLE leftover
[Checked in: Comment 27]
Comment 26 Marco Bonardo [::mak] 2012-03-27 16:27:32 PDT
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) 2012-03-28 01:57:37 PDT
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] 2012-04-02 05:05:37 PDT
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) 2012-04-27 06:05:03 PDT
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] 2012-04-27 07:13:04 PDT
(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] 2012-05-16 07:02:24 PDT
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

Note You need to log in before you can comment on or make changes to this bug.