854761 - Remove previous bookmark calls from PlacesUtils

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Andres Hernandez [:andreshm]]

After all BookmarkJSONUtils.jsm calls are updated, remove all previous code left in Places.

Comment 1

[Andres Hernandez [:andreshm]]

Bug 854288 handles all import calls, this bug handles export calls.

Comment 2

[Raymond Lee [:raymondlee]]

(In reply to Andres Hernandez [:andreshm] from comment #1)
> Bug 854288 handles all import calls, this bug handles export calls.

PlacesUtils.backups.saveBookmarksToJSONFile is replaced by BookmarkJSONUtils.exportToFile in bug 852041. Then, PlacesUtils.backups would be handled by bug 857429, therefore, no work is required in this bug.

Comment 3

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #2)
> (In reply to Andres Hernandez [:andreshm] from comment #1)
> > Bug 854288 handles all import calls, this bug handles export calls.
> 
> PlacesUtils.backups.saveBookmarksToJSONFile is replaced by
> BookmarkJSONUtils.exportToFile in bug 852041. Then, PlacesUtils.backups
> would be handled by bug 857429, therefore, no work is required in this bug.


After checking the code, PlacesUtils.serializeNodeAsJSONToOutputStream does the same thing as BookmarkNode.serializeAsJSONToOutputStream so I guess we should depreciate PlacesUtils.serializeNodeAsJSONToOutputStream and wrap it with the BookmarkNode.serializeAsJSONToOutputStream.

Andres: can you confirm that please?

Comment 4

[Andres Hernandez [:andreshm]]

(In reply to Raymond Lee [:raymondlee] from comment #3)
> (In reply to Raymond Lee [:raymondlee] from comment #2)
> > (In reply to Andres Hernandez [:andreshm] from comment #1)
> > > Bug 854288 handles all import calls, this bug handles export calls.
> > 
> > PlacesUtils.backups.saveBookmarksToJSONFile is replaced by
> > BookmarkJSONUtils.exportToFile in bug 852041. Then, PlacesUtils.backups
> > would be handled by bug 857429, therefore, no work is required in this bug.
> 
> 
> After checking the code, PlacesUtils.serializeNodeAsJSONToOutputStream does
> the same thing as BookmarkNode.serializeAsJSONToOutputStream so I guess we
> should depreciate PlacesUtils.serializeNodeAsJSONToOutputStream and wrap it
> with the BookmarkNode.serializeAsJSONToOutputStream.
> 
> Andres: can you confirm that please?

That's right, but this bug depends on changes apply on bug 855842 in order to avoid conflicts.

I'll work on those changes.

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 738900 [details] [diff] [review]
Patch v1

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

there is a call in /services/sync/tps/extensions/tps/modules/bookmarks.jsm that is unhandled here

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +118,5 @@
> +    let deferred = Promise.defer();
> +    Services.tm.mainThread.dispatch(function() {
> +      BookmarkNode.serializeAsJSONToOutputStream(
> +        aNode, aStream, aIsUICommand, aResolveShortcuts, aExcludeItems);
> +      deferred.resolve();

please try/catch and handle exceptions with a reject

::: toolkit/components/places/PlacesUtils.jsm
@@ +562,5 @@
> +          if (shouldClose)
> +            node.containerOpen = false;
> +
> +          return writer.value;
> +        });

unfortunately we cannot do this... and I'm not sure we have a solution off-hand since wrapNode is totally synchronous... this should probably keep using the synchronous version for now, you should move the body of serializeNodeAsJSONToOutputStream to a "private" method ( _ prefix) and invoke the private method from the deprecated serializeNodeAsJSONToOutputStream and from here (so that wrapNode won't cause the deprecation).

We'll re-evaluate this once we are done with moving APIs.

Comment 7

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Comment 8

[Raymond Lee [:raymondlee]]

(In reply to Andres Hernandez [:andreshm] from comment #7)
> Created attachment 740673 [details] [diff] [review]
> Patch v2

The patch in bug 860625 touches BookmarkNode.serializeAsJSONToOutputStream so it's best to wait for it to land first.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 740673 [details] [diff] [review]
Patch v2

please, re-flag once bug 860625 lands and this has been unbitrotted against that.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: Part 1: toolkit/ component v3

bug 860625 is in mozilla-inbound so I just updated this patch.  Splitting the previous patch into two as one part is for services/sync/

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: Part 2: services/sync

Patch for services/sync.  I have run the tps tests and the following test failed before and after applying the patch so I guess it's ok.

TEST-UNEXPECTED-FAIL | test_tabs.js | [phase3] Exception caught: ASSERTION FAILED! error locating tab No traceback available

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: Part 1: v4

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: Part 2: v4

Comment 14

[Raymond Lee [:raymondlee]]

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=10f44110f540

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3203f0d0d225
https://hg.mozilla.org/integration/mozilla-inbound/rev/2412bacd1fe2

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3203f0d0d225
https://hg.mozilla.org/mozilla-central/rev/2412bacd1fe2