1081102 - Implement batch fetch options in Bookmarks.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Bug 1068009 implements the single fetch option, batch fetches must still be implemented.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1

Comment 2

[Asaf Romano (gone)]

Comment on attachment 8507194 [details] [diff] [review]
patch v1

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

Please file a bug for post-facto in-depth review of all those tests.

::: toolkit/components/places/Bookmarks.jsm
@@ +357,5 @@
>  
>    /**
>     * Fetches information about a bookmarked item.
>     *
> +   * REMARK: The API will always resolve to a single bookmarked item (or null),

"Any successful call to this method resolves to [...]" reads better, I think.

@@ +360,5 @@
>     *
> +   * REMARK: The API will always resolve to a single bookmarked item (or null),
> +   *         even in those cases, like url and keyword, where multiple bookmarks
> +   *         exist.  If your use-case involves working with all of the bookmarks
> +   *         for a given match, you should use the callback instead.

Should/If you wish..., use...

@@ +398,5 @@
>      let info = guidOrInfo;
>      if (!info)
>        throw new Error("Input should be a valid object");
> +    if (onResult && typeof onResult != "function")
> +      throw new Error("Callback must be a valid function");

onResult callback

@@ +427,5 @@
>      return Task.spawn(function* () {
>        let results;
>        if (info.guid)
>          results = yield fetchBookmark(info);
> +      else if (info.parentGuid && "index" in info)

When you're done, please review all "in" info cases and use hasOwnProperty, as long as use getOwnPropertyNames.

@@ +443,5 @@
> +      results = results.map(r => copyPropsForOutput({}, r));
> +
> +      // Ideally this should handle an incremental behavior and thus be invoked
> +      // while we fetch.  Though, the likelihood of 2 or more bookmarks for the
> +      // same match is very low, so it's not worth the added code complication.

Well, this will remain true only as long as the result API is the only search-bookmarks API that's in place. I imagine the result API replacement is going to reuse and extend this module, so I'd not consider it a waste of time to make it right in advance. Though indeed, as you suggest, it's harmless as it is at this point.

::: toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js
@@ +6,5 @@
> +    this.results = [];
> +    return result => this.results.push(result);
> +  }
> +}
> +

missing ;

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8507194 [details] [diff] [review]
patch v1

patch merged into Bug 1068009.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

fixed by Bug 1068009.