we should deprecate any nsIFile usage in import/export code

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.

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.

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

I will attach a separate patch for tests

forgot one rethrow

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

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.

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

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