1047819 - make an async getAnnotationsForItem and deprecate the synchronous one

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[Marco Bonardo [:mak]]

we need an async version of getAnnotationsForItem and remove the synchronous version.

Comment 1

[Marco Bonardo [:mak]]

This seems to be a common cause of long janks on arewesmoothyet.com
I'm not sure we can completely remove the sync APIs due to D&D code, but we can probably provide an async alternative at least, for all the code points that can use it.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1047819 - Add a basic test for PlacesUtils.getAnnotationsForItem.

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

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1047819 - Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem.

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

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1047819 - Add an async version of PlacesUtils.getAnnotationsForItem.

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

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1047819 - Remove unused nsIAnnotationService::copyPageAnnotations/copyItemAnnotations.

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

Comment 6

[Mark Banner (:standard8)]

I think we could go further on the sync version of getAnnotationsForItem to improve performance better by merging the two calls within it. Since I've just found bug 1047820, I think we can take it further there.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8963522 [details]
Bug 1047819 - Add a basic test for PlacesUtils.getAnnotationsForItem.

https://reviewboard.mozilla.org/r/232460/#review237896

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8963523 [details]
Bug 1047819 - Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem.

https://reviewboard.mozilla.org/r/232462/#review237900

I suspect the original shape was due to the presence of binary annotations, for which you likely didn't want to fetch the value, plus the variant complication.
But this is a fine interim change while we move on with removing useless annos.

::: toolkit/components/places/nsAnnotationService.cpp:995
(Diff revision 1)
>  
>  
>  NS_IMETHODIMP
>  nsAnnotationService::GetItemAnnotationInfo(int64_t aItemId,
>                                             const nsACString& aName,
> +                                           nsIVariant** aValue,

nit: usually per xpcom convention output pointers use _lowerCamelCase instead of aCamelCase

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8963524 [details]
Bug 1047819 - Add an async version of PlacesUtils.getAnnotationsForItem.

https://reviewboard.mozilla.org/r/232464/#review237908

::: toolkit/components/places/PlacesUtils.jsm:106
(Diff revision 1)
> + * @param aItemId
> + *        The identifier of the itme for which annotations are to be
> + *        retrieved.
> + * @return Array of objects, each containing the following properties:
> + *         name, flags, expires, mimeType, type, value
> + */

Let's add a @note that one should instead use the async version unless there's absolutely no way to make the caller async.

::: toolkit/components/places/PlacesUtils.jsm:107
(Diff revision 1)
> + *        The identifier of the itme for which annotations are to be
> + *        retrieved.
> + * @return Array of objects, each containing the following properties:
> + *         name, flags, expires, mimeType, type, value
> + */
> +function getAnnotationsForItem(aItemId) {

Oh, I love that this is no more exposed!

::: toolkit/components/places/PlacesUtils.jsm:110
(Diff revision 1)
> + *         name, flags, expires, mimeType, type, value
> + */
> +function getAnnotationsForItem(aItemId) {
> +  var annos = [];
> +  var annoNames = PlacesUtils.annotations.getItemAnnotationNames(aItemId);
> +  for (var i = 0; i < annoNames.length; i++) {

can use for...of

::: toolkit/components/places/PlacesUtils.jsm:1214
(Diff revision 1)
>     * @return Array of objects, each containing the following properties:
>     *         name, flags, expires, mimeType, type, value
>     */
> -  getAnnotationsForItem: function PU_getAnnotationsForItem(aItemId) {
> -    var annosvc = this.annotations;
> -    var annos = [];
> +  async promiseAnnotationsForItem(aItemId) {
> +    let db =  await PlacesUtils.promiseDBConnection();
> +    let rows = await db.execute(

executeCached

::: toolkit/components/places/PlacesUtils.jsm:1217
(Diff revision 1)
> -  getAnnotationsForItem: function PU_getAnnotationsForItem(aItemId) {
> -    var annosvc = this.annotations;
> -    var annos = [];
> -    var annoNames = annosvc.getItemAnnotationNames(aItemId);
> -    for (var i = 0; i < annoNames.length; i++) {
> -      let value = {}, flags = {}, exp = {}, storageType = {};
> +  async promiseAnnotationsForItem(aItemId) {
> +    let db =  await PlacesUtils.promiseDBConnection();
> +    let rows = await db.execute(
> +      `SELECT n.name, a.content, a.expiration, a.flags
> +       FROM moz_anno_attributes n
> +       JOIN moz_items_annos a ON a.anno_attribute_id = n.id

nit: It doesn't matter in practice, but I'd probably invert the from and join tables, since logically what we are trying to select are single annos, not attributes

::: toolkit/components/places/PlacesUtils.jsm:1219
(Diff revision 1)
> -    var annos = [];
> -    var annoNames = annosvc.getItemAnnotationNames(aItemId);
> -    for (var i = 0; i < annoNames.length; i++) {
> -      let value = {}, flags = {}, exp = {}, storageType = {};
> -      annosvc.getItemAnnotationInfo(aItemId, annoNames[i], value, flags, exp, storageType);
> -      annos.push({name: annoNames[i],
> +    let rows = await db.execute(
> +      `SELECT n.name, a.content, a.expiration, a.flags
> +       FROM moz_anno_attributes n
> +       JOIN moz_items_annos a ON a.anno_attribute_id = n.id
> +       WHERE a.item_id = :item_id
> +      `, { item_id: aItemId });

just name both the argument and the param "itemId" and make this a shorthand

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8963525 [details]
Bug 1047819 - Remove unused nsIAnnotationService::copyPageAnnotations/copyItemAnnotations.

https://reviewboard.mozilla.org/r/232466/#review237918

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8963523 [details]
Bug 1047819 - Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem.

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

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8963524 [details]
Bug 1047819 - Add an async version of PlacesUtils.getAnnotationsForItem.

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

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8963525 [details]
Bug 1047819 - Remove unused nsIAnnotationService::copyPageAnnotations/copyItemAnnotations.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2b99ec4dca6
Add a basic test for PlacesUtils.getAnnotationsForItem. r=mak
https://hg.mozilla.org/integration/autoland/rev/cb8cbe3c000f
Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem. r=mak
https://hg.mozilla.org/integration/autoland/rev/6bc3bc48fc87
Add an async version of PlacesUtils.getAnnotationsForItem. r=mak
https://hg.mozilla.org/integration/autoland/rev/729748e603e5
Remove unused nsIAnnotationService::copyPageAnnotations/copyItemAnnotations. r=mak

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b2b99ec4dca6
https://hg.mozilla.org/mozilla-central/rev/cb8cbe3c000f
https://hg.mozilla.org/mozilla-central/rev/6bc3bc48fc87
https://hg.mozilla.org/mozilla-central/rev/729748e603e5