Remove previous bookmark calls from PlacesUtils

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: andreshm, Assigned: andreshm)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
After all BookmarkJSONUtils.jsm calls are updated, remove all previous code left in Places.
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Depends on: 854288
(Assignee)

Comment 1

6 years ago
Bug 854288 handles all import calls, this bug handles export calls.
(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.
No longer blocks: 852032
Depends on: 857429
(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?
Flags: needinfo?(andres)
(Assignee)

Comment 4

6 years ago
(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.
Depends on: 855842
Flags: needinfo?(andres)
(Assignee)

Comment 5

6 years ago
Created attachment 738900 [details] [diff] [review]
Patch v1
Attachment #738900 - Flags: review?(mak77)
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.
Attachment #738900 - Flags: review?(mak77)
(Assignee)

Comment 7

6 years ago
Created attachment 740673 [details] [diff] [review]
Patch v2
Attachment #738900 - Attachment is obsolete: true
Attachment #740673 - Flags: review?(mak77)
(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.
Depends on: 860625
Blocks: 846644
Comment on attachment 740673 [details] [diff] [review]
Patch v2

please, re-flag once bug 860625 lands and this has been unbitrotted against that.
Attachment #740673 - Flags: review?(mak77)
Created attachment 755787 [details] [diff] [review]
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/
Attachment #740673 - Attachment is obsolete: true
Attachment #755787 - Flags: review?(mak77)
Created attachment 755828 [details] [diff] [review]
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
Attachment #755828 - Flags: review?(rnewman)
Attachment #755828 - Flags: review?(rnewman) → review+
Attachment #755787 - Flags: review?(mak77) → review+
Created attachment 757746 [details] [diff] [review]
Part 1: v4
Attachment #755787 - Attachment is obsolete: true
Created attachment 757747 [details] [diff] [review]
Part 2: v4
Attachment #755828 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3203f0d0d225
https://hg.mozilla.org/mozilla-central/rev/2412bacd1fe2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.