No onItemAnnotationRemoved updates with async PlacesTransactions

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
In bug 1393771 I'm trying to re-order our tests. I've discovered that onItemAnnotationRemoved is not being triggered in the case where we're attempting to bookmark a Livemark, and then cancelling the dialog.

In turn, this leads to the livemark cache being out of sync, which can mess up the UI in other ways (e.g. not allow a bookmark folder to be moved via drag/drop).

If I turn off async places transactions, then it works fine.

Updated

3 months ago
Priority: -- → P1
Whiteboard: [fxsearch]
Comment hidden (mozreview-request)

Comment 2

3 months ago
mozreview-review
Comment on attachment 8902306 [details]
Bug 1394801 - Ensure livemarks are removed from the PlacesUIUtils livemark cache with async PlacesTransactions turned on.

https://reviewboard.mozilla.org/r/173838/#review179486

::: browser/components/places/PlacesUIUtils.jsm:65
(Diff revision 1)
> +      ]),
>  
>        onItemAnnotationSet(itemId, annoName) {
>          if (annoName == LIVEMARK_ANNO)
>            self.ids.add(itemId);
>        },

I think we could completely remove the dependency on nsIAnnotationObserver, and use onItemChanged checking the isAnno and name arguments. We should never remove the livemark annotation from a folder, only add it.

Another advantage is that it would give us the guid, that anno notifications are missing.

::: browser/components/places/PlacesUIUtils.jsm:77
(Diff revision 1)
>  
> +      // Ci.nsINavBookmarkObserver items.
> +
> +      onItemRemoved(itemId) {
> +        // Since the bookmark is removed, we know we can remove any references
> +        // to it from the catch.

typo: "catch"
Attachment #8902306 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 4

3 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c178d9e31307
Ensure livemarks are removed from the PlacesUIUtils livemark cache with async PlacesTransactions turned on. r=mak

Updated

3 months ago
Status: NEW → ASSIGNED

Comment 5

3 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/5ce04b2110b3
Ensure livemarks are removed from the PlacesUIUtils livemark cache with async PlacesTransactions turned on. r=mak

Comment 6

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ce04b2110b3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.