Closed Bug 855842 Opened 7 years ago Closed 7 years ago

Update calls to PlacesUtils.importJSONNode with BookmarkJSONUtils.importJSONNode

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: andreshm, Assigned: andreshm)

References

Details

Attachments

(1 file, 4 obsolete files)

Update tests using PlacesUtils.importJSONNode to use BookmarkJSONUtils.importJSONNode and remove the PlacesUtils version
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #730903 - Flags: review?(mak77)
Status: NEW → ASSIGNED
Comment on attachment 730903 [details] [diff] [review]
Patch v1

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

::: toolkit/components/places/PlacesUtils.jsm
@@ -1174,5 @@
> -   * @returns an array containing of maps of old folder ids to new folder ids,
> -   *          and an array of saved search ids that need to be fixed up.
> -   *          eg: [[[oldFolder1, newFolder1]], [search1]]
> -   */
> -  importJSONNode: function PU_importJSONNode(aData, aContainer, aIndex, aGrandParentId) {

unfortunately fast-dial add-on is using this API, and it has many users, so we can't just remove it.

please forward the call adding a Deprecated.warning message
Attachment #730903 - Flags: review?(mak77)
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to Marco Bonardo [:mak] from comment #2)
> 
> unfortunately fast-dial add-on is using this API, and it has many users, so
> we can't just remove it.
> 
> please forward the call adding a Deprecated.warning message

Done.
Attachment #730903 - Attachment is obsolete: true
Attachment #735004 - Flags: review?(mak77)
Comment on attachment 735004 [details] [diff] [review]
Patch v2

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

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +87,1 @@
>    }

Sigh, I didn't notice this was synchronously returning an array.
We can't add synchronous APIs to BookmarksJSONUtils, this method should return a promise, resolved to the array at a maximum.

So, let's make it fake-async, fix the test to use the async API, while the PlacesUtils version for now will stay as it is, but warn the deprecation to consumers.

sorry for the roundtrip.
Attachment #735004 - Flags: review?(mak77)
Attached patch Patch v3 (obsolete) — Splinter Review
Update the method to be fake async as requested.
Attachment #735004 - Attachment is obsolete: true
Attachment #737839 - Flags: review?(mak77)
Comment on attachment 737839 [details] [diff] [review]
Patch v3

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

The changes look scary, indeed there are various mistakes. I suggest you rather leave BookmarkImporter.importJSONNode as-is for now, instead just change BookmarkJSONUtils.importJSONNode as

let deferred = Promise.defer();
Services.tm.mainThread.dispatch(function() {
 let importer = new BookmarkImporter();
 deferred.resolve(importer.importJSONNode(aData, aContainer, aIndex, aGrandParentId));
}, Ci.nsIThread.DISPATCH_NORMAL);
return deferred.promise;

We will fix the internals later when making them actually async.

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +77,5 @@
> +   * @param aIndex
> +   *        The index within the container the item was dropped or pasted at
> +   * @return an array containing of maps of old folder ids to new folder ids,
> +   *         and an array of saved search ids that need to be fixed up.
> +   *         eg: [[[oldFolder1, newFolder1]], [search1]]

the @return documentation should be updated

@@ +207,5 @@
>          let folderIdMap = [];
>  
> +        Task.spawn(function() {
> +          for (let i = 0; i < batch.nodes.length; i++) {
> +            let node = batch.nodes[i];

for (let node of batch.nodes)

@@ +230,5 @@
> +              }
> +
> +              // Insert the data into the db
> +              for (let j = 0; j < node.children.length; j++) {
> +                let child = node.children[j];

for (let child of node.children)

@@ +260,4 @@
>  
> +          Services.tm.mainThread.dispatch(function() {
> +            deferred.resolve();
> +          }, Ci.nsIThread.DISPATCH_NORMAL);

you should also handle the onfailure callback, by rejecting deferred

@@ +294,5 @@
>          if (aContainer == PlacesUtils.tagsFolderId) {
>            // Node is a tag
>            if (aData.children) {
> +            for (let i = 0; i < aData.children.length; i++) {
> +              let child = aData.chidren[i];

for (let child of aData.children)

@@ +304,5 @@
>                }
> +            }
> +            Services.tm.mainThread.dispatch(function() {
> +              deferred.resolve([folderIdMap, searchIds]);
> +            }, Ci.nsIThread.DISPATCH_NORMAL);

as it is this is resolving the promise, and continuing, and later we resolve again?

@@ +327,2 @@
>              }
> +          }

could you please explain why the need to change all of the aData.annos stuff and remove .filter from here?

@@ +364,1 @@
>                }

again, nothing is waiting for this...

::: toolkit/components/places/tests/bookmarks/test_423515_forceCopyShortcuts.js
@@ +115,3 @@
>  
> +    root.containerOpen = false;
> +  });

this is wrong, the test should at least use do_test_pending and wait for this to invoke do_test_finished, unless you want to convert it to use add_task
Attachment #737839 - Flags: review?(mak77)
Attached patch Patch v4 (obsolete) — Splinter Review
Updated patch as requested.
Attachment #737839 - Attachment is obsolete: true
Attachment #738886 - Flags: review?(mak77)
Blocks: 854761
Comment on attachment 738886 [details] [diff] [review]
Patch v4

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

r=me, provided you properly handle exceptions from importer.importJSONNode

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +86,5 @@
> +                                              aGrandParentId) {
> +    let deferred = Promise.defer();
> +    Services.tm.mainThread.dispatch(function() {
> +      let importer = new BookmarkImporter();
> +      deferred.resolve(importer.importJSONNode(aData, aContainer, aIndex, aGrandParentId));

please try/catch and reject properly
Attachment #738886 - Flags: review?(mak77) → review+
Attached patch Patch v5Splinter Review
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8288984750ba
Attachment #738886 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0127d272357d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.