Closed Bug 1068009 Opened 5 years ago Closed 5 years ago

Implement the async bookmarking API for single items

Categories

(Toolkit :: Places, defect)

defect
Not set
Points:
8

Tracking

()

RESOLVED FIXED
mozilla36
Iteration:
36.1

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 5 obsolete files)

once we have an API design, we should implement it and new xpcshell tests for it.
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Flags: qe-verify?
Flags: firefox-backlog+
Flags: qe-verify? → qe-verify-
Depends on: 1068015
Depends on: 1068620
Depends on: 1069973
Iteration: 35.2 → 35.3
The scope of this part is to make this API more coherent with History.jsm
input will also accept URL objects, output will always contain URL objects.

dates will always be Date objects.

update and remove will now take an optional callback and resolve to a bool. While this may look strange for update, since it acts on a single item, it will allow us to make it batch multiple updates in future, without touching the API signature.

on invalid input, all methods will throw immediately rather than rejecting later.
Attachment #8498838 - Flags: review?(mano)
Depends on: 1077097
Comment on attachment 8498838 [details] [diff] [review]
Part 1 - Update the API to be coherent with History.jsm

moved to bug 1077097 to simplify backlog management
Attachment #8498838 - Attachment is obsolete: true
Attachment #8498838 - Flags: review?(mano)
Depends on: 1079180
Attached patch WIP (obsolete) — Splinter Review
just saving work.
So, i'm actually thinking to split out insert from update.
The cases where we want to insert a bookmark and at the same time update another one are quite limited or nonexistent, and the code is basically separated regardless.

What I'm worries about is the fact on insert you can enforce a guid, but suppose you want to update a bookmark uri instead, you pass a guid and the new uri, unfortunately you did something wrong and you pass a nonexistent guid. now you are creating a new bookmark with the given guid and the new uri. And you see no error...
update should instead throw if a non existing guid is passed to it.

I'm also going to split out new follow-ups from this cause monolithic implementation is not working. here I'll provide a patch able to act on a single bookmark, included the basic pieces (input validation and such).
In separate bugs I'll implement multiple output operations (for remove and fetch), and notifications.
an on check-in, I should fix CRLF line endings :/
Summary: Implement the async bookmarking API → Implement the async bookmarking API for single items
Attached patch patch v1 (obsolete) — Splinter Review
This is a first reviewable version.

It implements insert, fetch and remove for single items, plus input validation and some helpers.

It also includes the changes to the API discussed on IRC (but move() that we can add in future)

Other methods will be implemented in follow-ups.
Attachment #8501082 - Attachment is obsolete: true
Attachment #8503200 - Flags: review?(mano)
Iteration: 35.3 → 36.1
Comment on attachment 8503200 [details] [diff] [review]
patch v1

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

I'll review the test in the next revision.

::: toolkit/components/places/Bookmarks.jsm
@@ +98,5 @@
>     */
>    DEFAULT_INDEX: -1,
>  
>    /**
> +   * Inserts a bookmarked item in the bookmarks tree.

"Inserts a bookmark_ed_ item" is somewhat nonsense. I think "Insert a bookmark-item" is a fine compromise.

@@ +103,2 @@
>     *
> +   * For creating a bookmark, a minimum set of properties must be provided:

I think "the following set of properties are required" reads better.

@@ +105,3 @@
>     *  - type
>     *  - parentGuid
> +   *  - url, only for bookmarked pages

"bookmarked pages" here is even worse... Maybe "page bookmarks"? Or you could just leave the previous wording as is.

@@ +111,4 @@
>     * an undo manager implementation or synchronization, you should not do that.
>     *
> +   * Note that any known properties that don't apply to the specific item type
> +   * cause an exception.

Please file a bug about introducing these semantics in PlacesTransactions. Right now it just ignores such properties.

Alternatively, you could do what PT does. I considered both options for PT, and figured there's no clear cut.

@@ +113,5 @@
> +   * Note that any known properties that don't apply to the specific item type
> +   * cause an exception.
> +   *
> +   * @param info
> +   *        object representing a bookmarked item.

bookmark-item, or whatever wording you'll use elsewhere at last.

@@ +116,5 @@
> +   * @param info
> +   *        object representing a bookmarked item.
> +   *
> +   * @return {Promise} resolved when the creation is complete.
> +   * @resolves to an object representing the created bookmark.

Note the following also applies to all other methods.

It's not "an object", it's the input object, modified. That must be noted somewhere.

Having said that, considering code-pattern characteristics of Promise/Task consumers, which "inline" the callbacks and thus rely on the javascript context for figuring out the callback "source", or (with Task) just avoid the direct callback altogether, the value of reusing the input object is somewhat limited. "Doesn't hurt," I would say too, but maybe that's not true.

Consider, for example:
  let myBookmark = { url: "https..." ... };
  yield Bookmarks.insert(myBookmark)

  // OOPS, myBookmark.url isn't a string   anymore!
  if ('^https:/.test(myBookmark.url))

Well, actually, that happens due to the "inline" input validation behavior rather than the resolution behavior, but I guess these behaviors make sense only together, because you want to return a "proper" bookmark object.

FWIW, In PT, I don't modify the input object at all. However, there I don't need to return it to the consumer, so it's not the same use case.

Note that if you do decide not to reuse the input object (here and elsewhere), an alternative for the Object.assign trick should be found. May I suggest Symbols (see later comment)?

@@ +117,5 @@
> +   *        object representing a bookmarked item.
> +   *
> +   * @return {Promise} resolved when the creation is complete.
> +   * @resolves to an object representing the created bookmark.
> +   * @rejects JavaScript exception if it's not possible to create the

s/JavaScript exception/

@@ +119,5 @@
> +   * @return {Promise} resolved when the creation is complete.
> +   * @resolves to an object representing the created bookmark.
> +   * @rejects JavaScript exception if it's not possible to create the
> +   *          requested bookmark.
> +   * @throws if input is invalid.

the input/|info|

@@ +125,5 @@
> +   * @note Any unknown property in the info object is ignored.  Known properties
> +   *       may be overwritten.
> +   */
> +  insert: function (info) {
> +    // Ensure to use the same date for dateAdded and lastModified, even if

insert(info) {

here and elsewhere!

:)

@@ +126,5 @@
> +   *       may be overwritten.
> +   */
> +  insert: function (info) {
> +    // Ensure to use the same date for dateAdded and lastModified, even if
> +    // dateAdded is imposed by the consumer.

s/is/may be/

@@ +128,5 @@
> +  insert: function (info) {
> +    // Ensure to use the same date for dateAdded and lastModified, even if
> +    // dateAdded is imposed by the consumer.
> +    let time = (info && info.dateAdded) || new Date();
> +    validateBookmarkObject(info, { type: { requiredIf: () => true }

Let's make required: true work too,

I think it's worth making this optional if url is set (could be done in a follow-up).

@@ +129,5 @@
> +    // Ensure to use the same date for dateAdded and lastModified, even if
> +    // dateAdded is imposed by the consumer.
> +    let time = (info && info.dateAdded) || new Date();
> +    validateBookmarkObject(info, { type: { requiredIf: () => true }
> +                                 , index: { defaultValue: Bookmarks.DEFAULT_INDEX }

s/Bookmarks/this here and elsewhere.

@@ +137,5 @@
> +                                 , keyword: { validIf: b => b.type ==  Bookmarks.TYPE_BOOKMARK }
> +                                 , title: { validIf: b => [ Bookmarks.TYPE_BOOKMARK
> +                                                          , Bookmarks.TYPE_FOLDER ].indexOf(b.type) != -1 }
> +                                 , dateAdded: { defaultValue: time }
> +                                 , lastModified: { defaultValue: time }

Isn't last-modified supposed to be 0 initially?

@@ +140,5 @@
> +                                 , dateAdded: { defaultValue: time }
> +                                 , lastModified: { defaultValue: time }
> +                                 });
> +    return Task.spawn(function* () {
> +      // We must ensure the parent exists.

s/We must ensure/Ensure/

@@ +159,5 @@
> +
> +  /**
> +   * Updates a bookmarked item.
> +   *
> +   * You should only set the properties which should be changed, undefined

s/You should only/Only/

@@ +160,5 @@
> +  /**
> +   * Updates a bookmarked item.
> +   *
> +   * You should only set the properties which should be changed, undefined
> +   * properties won't be taken into account for the update.

s/for the update/

and I think this second sentence should be parenthesised.

@@ +190,5 @@
>    // void changeBookmarkURI(in long long aItemId, in nsIURI aNewURI);
>    // void setKeywordForBookmark(in long long aItemId, in AString aKeyword);
> +  update: function (info) {
> +    validateBookmarkObject(info, { type: { requiredIf: b => b.guid == null }
> +                                 , index: { defaultValue: Bookmarks.DEFAULT_INDEX }

1. Why is defaultValue set here? Wouldn't it result in the item being moved to the bottom for, say, a title update?

2. Shouldn't it also be required when guid isn't set (as with parentGuid)?

All of this update-not-by-guid feature needs to be documented, btw.

@@ +325,5 @@
> +        results = yield fetchBookmarksByURL(info);
> +      else if (info.keyword)
> +        results = yield fetchBookmarksByKeyword(info);
> +
> +      // Assign will ignore any non-enumerable properties.

Mention one or two such properties in parenthesis so the reader knows what this is all about.

@@ +488,5 @@
> +         VALUES (:keyword)
> +        `, { keyword: info.keyword });
> +    }
> +
> +    // Insert bookmark in database.

the ; into the

@@ +532,5 @@
> +// Fetch implementation.
> +
> +function* fetchBookmark(info) {
> +  let db = yield DBConnPromised;
> +  if (info.guid) {

I prefer info.hasOwnProperty or just an "in" check.

@@ +554,5 @@
> +      for (let prop of ["guid", "index", "type"]) {
> +        item[prop] = row.getResultByName(prop);
> +      }
> +      for (let prop of ["dateAdded", "lastModified"]) {
> +        item[prop] = new Date(parseInt(row.getResultByName(prop) / 1000));

You've toPRTime, should you have toDate?

@@ +565,5 @@
> +      for (let prop of ["_id", "_parentId", "_childCount", "_grandParentId"]) {
> +        let val = row.getResultByName(prop);
> +        if (val !== null) {
> +          Object.defineProperty(item, prop, { value: val
> +                                            , writable: true

You may omit |writeable: true|, unless I overlooked something about it.

@@ +567,5 @@
> +        if (val !== null) {
> +          Object.defineProperty(item, prop, { value: val
> +                                            , writable: true
> +                                            , enumerable: false
> +                                            , configurable: false });

Same goes for |configurable: false|.

Technically speaking, |enumerable: false| is the default. But don't omit it because it makes your intentions clear.

However, at this point you should probably use Symbols for this purpose. It would also make the code cleaner.

@@ +593,5 @@
> +  if (!item)
> +    throw new Error("No bookmarks found for the provided GUID.");
> +
> +  // Disallow removing root folders.
> +  if (item._parentId == PlacesUtils.placesRoot)

This won't do for the places root itself.

@@ +612,5 @@
> +      `UPDATE moz_bookmarks SET position = position - 1 WHERE
> +       parent = :parentId AND position > :index
> +      `, { parentId: item._parentId, index: item.index });
> +
> +    // Set parent lastModified.

s/Set/Update/

parent/parent's?

@@ +614,5 @@
> +      `, { parentId: item._parentId, index: item.index });
> +
> +    // Set parent lastModified.
> +    // TODO (bug 408991): doing this for all the ancestors would be slow
> +    // without a nested tree structure, so for now we just update the parent.

Bug 408991 is about the general ability to do so. Its the dependency for the (not yet filed, I guess) bug here.

@@ +665,5 @@
> + */
> +function simpleValidateFunc(boolValidateFn) {
> +  return (v, input) => {
> +    if (!boolValidateFn(v, input))
> +      throw new Error("Invalid value");

...for property 'theProperty'

@@ +695,5 @@
> +      return undefined;
> +    return v.slice(0, TITLE_LENGTH_MAX);
> +  },
> +  url: v => {
> +    simpleValidateFunc(val => (typeof(val) == "string" && val.length <= URL_LENGTH_MAX) ||

Won't "new URL" done below catch the URL_LENGTH_MAX case? There are other reasons it could fail, and I'm not sure this one is worth the pre-validation.

@@ +702,5 @@
> +                      ).call(this, v);
> +    if (typeof(v) === "string")
> +      return new URL(v);
> +    if (v instanceof Ci.nsIURI)
> +      return new URL(v.spec);

Is the URL object any better than the plain string internally? Shouldn't you use "new URL" only as a validation agent?
Attachment #8503200 - Flags: review?(mano) → feedback+
(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #7)
> > +   * Note that any known properties that don't apply to the specific item type
> > +   * cause an exception.
> 
> Please file a bug about introducing these semantics in PlacesTransactions.
> Right now it just ignores such properties.
> 
> Alternatively, you could do what PT does. I considered both options for PT,
> and figured there's no clear cut.

Well, if someone passes into a known property with a broken value, it's very likely a bug is hiding somewhere in his code, so I think it's worth to notify the problem.

> Having said that, considering code-pattern characteristics of Promise/Task
> consumers, which "inline" the callbacks and thus rely on the javascript
> context for figuring out the callback "source", or (with Task) just avoid
> the direct callback altogether, the value of reusing the input object is
> somewhat limited. "Doesn't hurt," I would say too, but maybe that's not true.

Well, I did it cause I remember you originally complained updatePlaces was not returning the original object, I'm fine either way and it's likely more complication to return the original object than to return a new one.

> Note that if you do decide not to reuse the input object (here and
> elsewhere), an alternative for the Object.assign trick should be found. May
> I suggest Symbols (see later comment)?

Actually, there's nothing wrong with Object.assign({}, my_object) and it's still a good way to return a clean object.
Symbol in this case would just be an alternative to setting a non-enumerable property, I'm not sure there's additional benefit in using it for this code?
Why do you think it will make the code cleaner?

> @@ +128,5 @@
> > +  insert: function (info) {
> > +    // Ensure to use the same date for dateAdded and lastModified, even if
> > +    // dateAdded is imposed by the consumer.
> > +    let time = (info && info.dateAdded) || new Date();
> > +    validateBookmarkObject(info, { type: { requiredIf: () => true }
> 
> Let's make required: true work too,

do you mean adding also a "required" property or making "requiredIf" accept either a bool or a function?

> Isn't last-modified supposed to be 0 initially?
nope, last modified is never null or 0, initially it's dateAdded. this way we can more easily ORDER BY last modified and use an index.

> @@ +190,5 @@
> >    // void changeBookmarkURI(in long long aItemId, in nsIURI aNewURI);
> >    // void setKeywordForBookmark(in long long aItemId, in AString aKeyword);
> > +  update: function (info) {
> > +    validateBookmarkObject(info, { type: { requiredIf: b => b.guid == null }
> > +                                 , index: { defaultValue: Bookmarks.DEFAULT_INDEX }
> 
> 1. Why is defaultValue set here? Wouldn't it result in the item being moved
> to the bottom for, say, a title update?

yep, it's fixed in the update bug.

> 2. Shouldn't it also be required when guid isn't set (as with parentGuid)?

guid must always be set, fixed in the update bug.

> All of this update-not-by-guid feature needs to be documented, btw.

they don't exist, it's just that I fixed everything in the update bug...

> You've toPRTime, should you have toDate?

Yes, I think I added it in another one of these related bugs...

> You may omit |writeable: true|, unless I overlooked something about it.
> Same goes for |configurable: false|.

I remember someone in the past said that when using defineProperty is better to always be explicit to avoid confusing readers (that may not recall the default values)

> However, at this point you should probably use Symbols for this purpose. It
> would also make the code cleaner.

I'd first would like to understand if the only advantage is being able to not use defineProperty... cause it seems a very small gain.

> @@ +614,5 @@
> > +      `, { parentId: item._parentId, index: item.index });
> > +
> > +    // Set parent lastModified.
> > +    // TODO (bug 408991): doing this for all the ancestors would be slow
> > +    // without a nested tree structure, so for now we just update the parent.
> 
> Bug 408991 is about the general ability to do so. Its the dependency for the
> (not yet filed, I guess) bug here.

I actually fixed this in the update patch...

> @@ +695,5 @@
> > +      return undefined;
> > +    return v.slice(0, TITLE_LENGTH_MAX);
> > +  },
> > +  url: v => {
> > +    simpleValidateFunc(val => (typeof(val) == "string" && val.length <= URL_LENGTH_MAX) ||
> 
> Won't "new URL" done below catch the URL_LENGTH_MAX case? There are other
> reasons it could fail, and I'm not sure this one is worth the pre-validation.

Why should new URL() catch URL_LENGTH_MAX in a pure href string?

> @@ +702,5 @@
> > +                      ).call(this, v);
> > +    if (typeof(v) === "string")
> > +      return new URL(v);
> > +    if (v instanceof Ci.nsIURI)
> > +      return new URL(v.spec);
> 
> Is the URL object any better than the plain string internally? Shouldn't you
> use "new URL" only as a validation agent?

Well, we decided to return URL objects in the output, it's simpler for me to bring that on...

If we revert that decision and decide to return hrefs, well we should do globally for Bookmarks.jsm and History.jsm, and at that point I'd leave that to a follow-up. Thoughts?
Flags: needinfo?(mano)
ah, the other question is about unset properties, David pointed out unsetting properties could be less performant than setting them to undefined due to js shapes. So in History he is not unsetting properties.
Flags: needinfo?(mano)
Attached patch tests v2 (obsolete) — Splinter Review
tests, going to land with r=post-facto or rs=mano
Attachment #8503200 - Attachment is obsolete: true
Attached patch merge v2 (obsolete) — Splinter Review
merge of all of the implemented APIs (so far)
Attachment #8508756 - Attachment is obsolete: true
Attachment #8508757 - Flags: review?(mano)
Attachment #8508756 - Attachment is obsolete: false
Comment on attachment 8508757 [details] [diff] [review]
merge v2

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

::: toolkit/components/places/Bookmarks.jsm
@@ +35,4 @@
>   *
>   *  - title (string)
>   *      The item's title, if any.  Empty titles and null titles are considered
>   *      the same and the property is unset on retrieval in such a case.

same, and

@@ +35,5 @@
>   *
>   *  - title (string)
>   *      The item's title, if any.  Empty titles and null titles are considered
>   *      the same and the property is unset on retrieval in such a case.
> + *      Title cannot be longer than DB_TITLE_LENGTH_MAX, or it will be truncated.

Titles? A title? "Title" seems strange.

@@ +48,1 @@
>   *      longer value is provided

same same.

@@ +82,2 @@
>  
> +// Imposed by us to limit database size.

s/by us/

@@ +99,5 @@
>     */
>    DEFAULT_INDEX: -1,
>  
>    /**
> +   * Inserts a bookmark-item in the bookmarks tree.

into/to

@@ +195,5 @@
> +               , validIf: b => b.index >= 0 }
> +      , parentGuid: { requiredIf: b => b.hasOwnProperty("index") }
> +      });
> +
> +    if (Object.getOwnPropertyNames(updateInfo).length < 2)

"There should be at last one more property in addition to guid" or something like that.

BTW, please switch to Object.keys everywhere.

@@ +326,5 @@
> +      let rows = yield db.executeCached(
> +        `WITH RECURSIVE
> +         descendants(did) AS (
> +           SELECT folder_id FROM moz_bookmarks_roots
> +           WHERE root_name IN ("toolbar", "menu", "unfiled")

Please file a bug for the tags-root issue we discussed over IRC, and reference it here.

@@ +334,5 @@
> +         )
> +         SELECT b.id AS _id, b.parent AS _parentId, b.position AS 'index',
> +                b.type, url, b.guid, p.guid AS parentGuid, b.dateAdded,
> +                b.lastModified, b.title, NULL AS _grandParentId,
> +                NULL AS _childCount, NULL AS keyword

:(, so I understand you need that for onItemRemoved.

File a bug for adding something similar to onClearHistory for bookmarks.

@@ +417,5 @@
> +    if (!info)
> +      throw new Error("Input should be a valid object");
> +    if (typeof(info) != "object") {
> +      info = { guid: guidOrInfo };
> +    }

nit: either remove these braces, or add them to the block above.

@@ +665,5 @@
> +    if (info.hasOwnProperty("keyword") && info.keyword === "")
> +      yield removeOrphanKeywords(db);
> +  });
> +
> +  let updatedItem = Object.assign({}, item, info);

This won't copy non-enumerable properties, which your callers seem to assume that might be there.

@@ +865,5 @@
> +    updateFrecency(db, [item.url]).then(null, Cu.reportError);
> +  }
> +
> +  // Notify onItemRemoved.
> +  // TODO

TODO in eraseEverything as well please.

btw, you may want to move the implementation of eraseEverything to its own "private" global function, just for consistency.

@@ +889,5 @@
> + * @return a cleaned up bookmark object.
> + * @note "guid" is never removed.
> + */
> +function removeSameValueProperties(dest, src) {
> +  for (let prop of Object.getOwnPropertyNames(dest)) {

since you're going to switch to Object.keys, here you should just do |for (let prop of dest)|

@@ +968,5 @@
> +      if (val) {
> +        if (prop == "url")
> +          item[prop] = new URL(val);
> +        else
> +          item[prop] = val;

item[prop] = ... ? ...
Attachment #8508757 - Flags: review?(mano) → review+
Comment on attachment 8508756 [details] [diff] [review]
tests v2

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

::: toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js
@@ +116,5 @@
> +
> +  Assert.deepEqual(bm1, bm2);
> +  Assert.equal(bm2.parentGuid, unfiledGuid);
> +  Assert.equal(bm2.index, 0);
> +  Assert.equal(bm2.dateAdded.getTime(), bm2.lastModified.getTime());

Here and elsewhere: It looks like Assert.equal supports Date objects, so you don't need to call getTime.
(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #13)
> Here and elsewhere: It looks like Assert.equal supports Date objects, so you
> don't need to call getTime.

Ah. it's deepEqual that supports it, equal just does object comparison... that's why it didn't work when I tried. I will switch to deepEqual for Date.
Attached patch tests v3Splinter Review
Attachment #8508756 - Attachment is obsolete: true
Attached patch merge v3Splinter Review
Attachment #8508757 - Attachment is obsolete: true
Blocks: 1081101
Blocks: 1081102
Blocks: 1081254
You need to log in before you can comment on or make changes to this bug.