Can't import bookmarks from Safari-created HTML files

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Virgil Dupras, Assigned: Virgil Dupras)

Tracking

16 Branch
Firefox 20
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 671260 [details]
Importing this file does nothing

When I try to import bookmarks from a Safari-created HTML file (through Import and Backup --> Import bookmarks from HTML...), nothing happens.

I've messed up quite a bit with my HTML file and I could reproduce a minimal bookmark file that reproduces the issue. I attach this file with the ticket.

If I remove all lines with H3 tags on them, the file can correctly be imported, so that must be related somehow.
Thanks for the example file, will be useful for debugging.
(Assignee)

Comment 2

5 years ago
I've been toying with the idea of getting my feet wet with the Mozilla codebase and investigated to bug further. It turns out that wrapping all the code below the initial "<H1>" header into an extra "<DL>" tags (to make the structure look a bit more like what Firefox produces on export) fixes the problem.

I'd like to try fixing that bug. Could I be assigned to it?
Sure, the related code is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkHTMLUtils.jsm
Assignee: nobody → hsoft

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

5 years ago
Flags: in-testsuite?
(Assignee)

Comment 4

5 years ago
Created attachment 674429 [details] [diff] [review]
Automated test (xpcshell-based) for the bug
(Assignee)

Comment 5

5 years ago
I've started with the test. After a bit of reading on how testing works in the project, I though that the xpcshell suite was the best place to include that kind of test.
(Assignee)

Comment 6

5 years ago
Created attachment 674495 [details] [diff] [review]
Testcase + fix for the bug

Added a fix to the testcase.
Attachment #674429 - Attachment is obsolete: true
Attachment #674495 - Flags: review?(mak77)
Comment on attachment 674495 [details] [diff] [review]
Testcase + fix for the bug

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

So, I've verified that in the original cpp code we were invoking PopFrame() in this case, and it was returning a NS_ERROR_FAILURE. Unfortunately (or luckily?) the call was ignoring the error and proceeding, making it a no-op, that is basically what this patch does. I'm fine with restoring the original behavior.

r=me with these comments addressed
Thank you for taking care of this.

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +256,5 @@
>      // items will be treated as further siblings of FOO and BAR
> +    // This special frame popping business, of course, only happens when our
> +    // frame array has more than one element so we can avoid situations where
> +    // we don't have a frame to parse into anymore.
> +    if ((frame.containerNesting == 0) && (this._frames.length > 1)) {

there's no need for the additional parenthesis, the operators precedence is clear enough already.

::: toolkit/components/places/tests/unit/test_801450.js
@@ +1,5 @@
> +/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

please name the test in a meaningful way, we are trying to move from old test_#.js style since it's unreadable.
I'd suggest test_bookmarks_html_singleframe.js (and test_bookmarks_html_singleframe.html)

@@ +8,5 @@
> +Cu.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
> +
> +function run_test() {
> +  do_test_pending();
> +  var bookmarksFile = do_get_file("bug801450.html");

please use "let" everywhere, we took that as a coding style convention in this module

@@ +11,5 @@
> +  do_test_pending();
> +  var bookmarksFile = do_get_file("bug801450.html");
> +  try {
> +    BookmarkHTMLUtils.importFromFile(bookmarksFile, true, after_import);
> +  } catch(ex) { do_throw("couldn't import bookmarks file: " + ex); }

there's no need for this try/catch + do_throw, if it throws the test will fail already

@@ +16,5 @@
> +}
> +
> +function after_import(success) {
> +  do_check_true(success);
> +  root = PlacesUtils.getFolderContents(PlacesUtils.bookmarksMenuFolderId).root;

missing "let"

@@ +21,5 @@
> +  root.containerOpen = true;
> +  do_check_eq(root.childCount, 1);
> +  var folder = root.getChild(0);
> +  folder = folder.QueryInterface(Ci.nsINavHistoryContainerResultNode);
> +  folder.containerOpen = true;

PlacesUtils.asContainer(folder).containerOpen = true;

@@ +27,5 @@
> +  do_check_eq(folder.childCount, 1);
> +  var bookmark = folder.getChild(0);
> +  do_check_eq(bookmark.uri, "http://www.mozilla.org/")
> +  do_check_eq(bookmark.title, "Mozilla")
> +  do_test_finished();

just before finished, please
folder.containerOpen = false;
root.containerOpen = false;

(it's not "needed" by the interface, but doing so is a good habit since allows to free memory earlier)
Attachment #674495 - Flags: review?(mak77) → review+
(Assignee)

Comment 8

5 years ago
Thanks for the review, that's a lot of nice tips I've learned there. I'll produce an updated patch shortly.
(Assignee)

Comment 9

5 years ago
Created attachment 675694 [details] [diff] [review]
Patch w/ mak's adjustment propositions

I've applied mak's suggestions and created a new patch.
Attachment #674495 - Attachment is obsolete: true
Attachment #675694 - Flags: review?(mak77)
Comment on attachment 675694 [details] [diff] [review]
Patch w/ mak's adjustment propositions

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

just a couple minor things, you don't need a further review. thanks.

::: toolkit/components/places/tests/unit/test_bookmarks_html_singleframe.js
@@ +17,5 @@
> +
> +function after_import(success) {
> +  do_check_true(success);
> +  let root = PlacesUtils.getFolderContents(PlacesUtils.bookmarksMenuFolderId).root;
> +  root.containerOpen = true;

don't need to open the root, getFoldercontents gives you an already opened container

@@ +26,5 @@
> +  do_check_eq(folder.childCount, 1);
> +  let bookmark = folder.getChild(0);
> +  do_check_eq(bookmark.uri, "http://www.mozilla.org/");
> +  do_check_eq(bookmark.title, "Mozilla");
> +  PlacesUtils.asContainer(folder).containerOpen = false;

you don't need to call asContainer again, you already QI this object above.
Attachment #675694 - Flags: review?(mak77) → review+

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 11

5 years ago
Created attachment 675851 [details] [diff] [review]
Patch w/ mak's adjustment propositions (again)
Attachment #675694 - Attachment is obsolete: true

Updated

5 years ago
Duplicate of this bug: 817327
ops, looks like we lost this along the way. This needs a tryrun and to be pushed after it returns.
Flags: needinfo?(mak77)
Created attachment 688005 [details] [diff] [review]
with checkin comment

I took the freedom to add author and checkin-comment.
This is the try run
https://tbpl.mozilla.org/?tree=Try&rev=29839c529c1e
Attachment #675851 - Attachment is obsolete: true
Flags: needinfo?(mak77)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2757708863
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb2757708863
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.