1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get()

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Mark Banner (:standard8)]

As seen in bug 1460579, it would be helpful if we could automatically store objects and arrays in Places' metadata.

Additionally, it would be useful for the get() functions to take a default value to return should the item not exist in the database.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get().

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

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8981811 [details]
Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get().

Oops, looks like I broke something.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8981811 [details]
Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get().

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

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8981811 [details]
Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get().

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

Comment 5

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

Comment on attachment 8981811 [details]
Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get().

https://reviewboard.mozilla.org/r/247866/#review254454

::: toolkit/components/places/PlacesSyncUtils.jsm:86
(Diff revision 3)
>  
>    /**
>     * Returns the current history sync ID, or `""` if one isn't set.
>     */
>    async getSyncId() {
> -    let syncId = await PlacesUtils.metadata.get(
> +    return PlacesUtils.metadata.get(

the function doesn't need to be async anymore

::: toolkit/components/places/PlacesSyncUtils.jsm:412
(Diff revision 3)
>     */
>    async getSyncId() {
> -    let syncId = await PlacesUtils.metadata.get(
> -      BookmarkSyncUtils.SYNC_ID_META_KEY);
> +    return PlacesUtils.metadata.get(
> +      BookmarkSyncUtils.SYNC_ID_META_KEY, "");
> -    return syncId || "";
>    },

ditto

::: toolkit/components/places/PlacesUtils.jsm:2001
(Diff revision 3)
>     * @param  {String} key
>     *         The metadata key to look up.
> +   * @param  {String|Object|Array} defaultValue
> +   *         Optional. The default value to return if the value is not present,
> +   *         or cannot be parsed.
>     * @return {*}

this should actually be a @resolves

::: toolkit/components/places/PlacesUtils.jsm:2002
(Diff revision 3)
>     *         The metadata key to look up.
> +   * @param  {String|Object|Array} defaultValue
> +   *         Optional. The default value to return if the value is not present,
> +   *         or cannot be parsed.
>     * @return {*}
>     *         The value associated with the key, or `null` if not set.

please add a @rejects

::: toolkit/components/places/PlacesUtils.jsm:2079
(Diff revision 3)
>        return;
>      }
> +
> +    let cacheValue = value;
> +    if (typeof value == "object" && ChromeUtils.getClassName(value) != "Uint8Array") {
> +      value = `${this.jsonPrefix}${this._base64Encode(JSON.stringify(value))}`;

looks like you could just + the 2 strings instead of using a template string

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8981811 [details]
Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247866/diff/3-4/

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8981811 [details]
Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247866/diff/4-5/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b489bcf8e20d
Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get(). r=mak

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b489bcf8e20d