1394801 - No onItemAnnotationRemoved updates with async PlacesTransactions

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1394801 - Ensure livemarks are removed from the PlacesUIUtils livemark cache with async PlacesTransactions turned on.

Review commit: https://reviewboard.mozilla.org/r/173838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173838/

Comment 2

[Marco Bonardo [:mak]]

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"

Comment 3

[Mark Banner (:standard8)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173838/diff/1-2/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c178d9e31307
Ensure livemarks are removed from the PlacesUIUtils livemark cache with async PlacesTransactions turned on. r=mak

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ce04b2110b3