Tim Taubert [:ttaubert] (inactive) Assignee
	Description • 10 years ago

Sometimes when converting tests to the new Bookmarks API I would forget specifying a type property when calling Bookmarks.insert() to create a bookmark. When passing a URI as one does when inserting a bookmark it will fail with:

Invalid value for property 'url': http://example.com/

The |validIf| for |url| fails before we even start checking for required properties. Knowing that |type| is required would however be much more helpful than telling me the value for |url| is invalid :)

	Marco Bonardo [:mak]
	Comment 1 • 10 years ago

yeah, actually we wanted to make type optional if url is provided, since it's clear it's a TYPE_BOOKMARK

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 2 • 10 years ago

That sounds like a good idea too!

	Marco Bonardo [:mak]
	Comment 4 • 9 years ago

Comment on attachment 8575983 [details] [diff] [review]
0001-Bug-1127277-Default-to-TYPE_BOOKMARK-when-no-type-is.patch

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

::: toolkit/components/places/Bookmarks.jsm
@@ +148,2 @@
>      let insertInfo = validateBookmarkObject(info,
>        { type: { required: true }

wouldn't be the same if you'd just replace required: true here with defaultValue: this.TYPE_BOOKMARK ?

::: toolkit/components/places/tests/bookmarks/test_bookmarks_insert.js
@@ +251,5 @@
> +add_task(function* create_bookmark_without_type() {
> +  let bm = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
> +                                                url: "http://example.com/",
> +                                                title: "a bookmark" });
> +  checkBookmarkObject(bm);

additionally check it's a TYPE_BOOKMARK and the url?

	Marco Bonardo [:mak]
	Comment 6 • 9 years ago

Comment on attachment 8576117 [details] [diff] [review]
0001-Bug-1127277-Default-to-TYPE_BOOKMARK-when-no-type-is.patch, v2

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

::: toolkit/components/places/Bookmarks.jsm
@@ +139,5 @@
>      // Ensure to use the same date for dateAdded and lastModified, even if
>      // dateAdded may be imposed by the caller.
>      let time = (info && info.dateAdded) || new Date();
>      let insertInfo = validateBookmarkObject(info,
> +      { type: { required: true, defaultValue: this.TYPE_BOOKMARK }

defaultValue makes required pointless.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 7 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/d31ed2b38332

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 9 years ago

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