801450 - Can't import bookmarks from Safari-created HTML files

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Virgil Dupras]

Attachment: 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.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Thanks for the example file, will be useful for debugging.

Comment 2

[Virgil Dupras]

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?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Sure, the related code is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkHTMLUtils.jsm

Comment 4

[Virgil Dupras]

Attachment: Automated test (xpcshell-based) for the bug

Comment 5

[Virgil Dupras]

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.

Comment 6

[Virgil Dupras]

Attachment: Testcase + fix for the bug

Added a fix to the testcase.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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)

Comment 8

[Virgil Dupras]

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

Comment 9

[Virgil Dupras]

Attachment: Patch w/ mak's adjustment propositions

I've applied mak's suggestions and created a new patch.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 11

[Virgil Dupras]

Attachment: Patch w/ mak's adjustment propositions (again)

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

ops, looks like we lost this along the way. This needs a tryrun and to be pushed after it returns.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: 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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2757708863

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb2757708863