967192 - Bookmarks restore and import should use OS.File

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

we should deprecate any nsIFile usage in import/export code

Comment 1

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

while here I'd like to kill BookmarkHTMLUTils.importJSONNode that is only needed by 1 test.
I'll also file a bug to remove the deprecated stuff from PlacesUtils.

Comment 2

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

to remove importJSONNode, I need to figure the whole reasoning behind resolveShortcuts (in wrapNode). I honestly don't see any case where we may want to convert a simple place: query to a deep copy of its contents. My understanding is that originally we made it to protect roots from being moved to other locations, though now:
- roots are protected in the backend, nobody can remove or move them (just in case)
- we support proper "copy" actions when the parent is readonly
- we can also double check just before moving the item if it can really be moved otherwise fallback to a copy.

Basically, I think resolveShortcuts is a bunch of useless code today, though to remove it I basically have to fix bug 435851. Not that it's a bad idea, it will just some more hours of work.
I want to simply make shortcuts being dragged as shortcuts, remove resolveShortcuts, remove test_423515_forceCopyShortcuts.js and finally I should be able to remove the importJSONNode method, and maybe something more.

Comment 3

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

the patch in bug 435851 solves comment 2, so I can now proceed with the other APIs

Comment 4

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

Attachment: patch v1

I will attach a separate patch for tests

Comment 5

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

Attachment: patch v1.1

forgot one rethrow

Comment 6

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

Attachment: tests v1

this is just "automatic" conversion of tests, nothing interesting to see

Comment 7

[Asaf Romano (gone)]

Comment on attachment 8377274 [details] [diff] [review]
patch v1.1

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

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +133,5 @@
>  this.BookmarkHTMLUtils = Object.freeze({
>    /**
>     * Loads the current bookmarks hierarchy from a "bookmarks.html" file.
>     *
> +   * @param aUrl

I prefer aUrlString over aUrl for a spec, but, of course, aURLSpec or aSpec is better.

@@ +163,5 @@
>    /**
>     * Loads the current bookmarks hierarchy from a "bookmarks.html" file.
>     *
> +   * @param aFilePath
> +   *        OS.File path of the "bookmarks.html" file to be loaded.

As I said for the other patch,, I wouldn't mention "OS.File path" as if it is something different from a plain string.

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +58,5 @@
>     * @note any item annotated with "places/excludeFromBackup" won't be removed
>     *       before executing the restore.
>     *
>     * @param aFilePath
> +   *        OS.File path of bookmarks in JSON format to be restored.

Ditto.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0cf7cdc0301
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a294c3292c3

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0cf7cdc0301
https://hg.mozilla.org/mozilla-central/rev/6a294c3292c3