1208907 - Implement bookmarks API for open extension API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Tom S [:evilpie]]

https://developer.chrome.com/extensions/bookmarks

API is listed on https://wiki.mozilla.org/WebExtensions#List_of_APIs_we_will_likely_support_in_the_future

Comment 1

[Tom S [:evilpie]]

Attachment: v1 - Partially implement chrome.bookmarks

I am going to implement to whole API soon, but I would like to land the most important bits first. Especially "search" is going to take a while still.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

As a request for Whimsy, could you implement the onCreated event next?  (It would enable the second-most loved feature… :)

Comment 3

[Tom S [:evilpie]]

Comment on attachment 8667861 [details] [diff] [review]
v1 - Partially implement chrome.bookmarks

Marco can you take a look as well, you seem to be the bookmarks expert.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8667861 [details] [diff] [review]
v1 - Partially implement chrome.bookmarks

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

This all looks good to me. We need a test before we can check it in though.

::: toolkit/components/extensions/ext-bookmarks.js
@@ +41,5 @@
> +    excludeItemsCallback: aItem => {
> +      if (aItem.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
> +        return true;
> +      return aItem.annos &&
> +             aItem.annos.find(a => a.name == PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);

This looks weird. Hopefully Marco knows whether it makes sense.

@@ +43,5 @@
> +        return true;
> +      return aItem.annos &&
> +             aItem.annos.find(a => a.name == PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);
> +    },
> +    includeItemIds: true

Do we use the IDs? It seems like a deprecated feature. It looks like you're using the GUIDs instead, which is the right thing to do.

Comment 5

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

Comment on attachment 8667861 [details] [diff] [review]
v1 - Partially implement chrome.bookmarks

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

it mostly looks good, but the original API is very badly documented and missing a lot of edge cases, error handling, behavior (recursive or not), so it's hard to figure the 1:1 compatibility.

::: toolkit/components/extensions/ext-bookmarks.js
@@ +23,5 @@
> +      treenode.parentId = parent.guid;
> +    }
> +
> +    if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE) {
> +      // This isn't quite correct. Recently Bookmarked ends up here ...

Yes, Places queries ARE bookmarks, not folders. I don't know if you want to filter them out or what... their URIs start with "place:"

@@ +28,5 @@
> +      treenode.url = node.uri;
> +    } else {
> +      treenode.dateGroupModified = node.lastModified / 1000;
> +
> +      if (node.children && !onlyChildren) {

is getChildren recursive? cause this is not.

@@ +41,5 @@
> +    excludeItemsCallback: aItem => {
> +      if (aItem.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
> +        return true;
> +      return aItem.annos &&
> +             aItem.annos.find(a => a.name == PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);

I don't know if in this case you care about EXCLUDE_FROM_BACKUP_ANNO.
This annotation is used to avoid backing up some bookmarks (like smart queries) that we built automatically and are not created by the user, cause in a backup they may create issues (double creation on restore, broken shortcuts and so on). For an API like this you likely don't care to exclude them.

@@ +43,5 @@
> +        return true;
> +      return aItem.annos &&
> +             aItem.annos.find(a => a.name == PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);
> +    },
> +    includeItemIds: true

yes, you likely don't want itemIds, we need them in some internal code until we finish transitioning to guids.

@@ +68,5 @@
> +    node.parentId = result.parentGuid;
> +  }
> +
> +  if (result.type == Bookmarks.TYPE_BOOKMARK) {
> +    node.url = result.url.toString(); // Output is always URL object.

.href is a little bit more specific than toString

@@ +70,5 @@
> +
> +  if (result.type == Bookmarks.TYPE_BOOKMARK) {
> +    node.url = result.url.toString(); // Output is always URL object.
> +  } else {
> +    if (result.type != Bookmarks.TYPE_FOLDER) throw "HOW?!";

so basically, one can pass in a separator guid...

@@ +71,5 @@
> +  if (result.type == Bookmarks.TYPE_BOOKMARK) {
> +    node.url = result.url.toString(); // Output is always URL object.
> +  } else {
> +    if (result.type != Bookmarks.TYPE_FOLDER) throw "HOW?!";
> +    node.dateGroupModified = result.lastModified.valueOf();

and .getTime() is a bit more specific too.

@@ +74,5 @@
> +    if (result.type != Bookmarks.TYPE_FOLDER) throw "HOW?!";
> +    node.dateGroupModified = result.lastModified.valueOf();
> +  }
> +
> +  return node;

I wonder if we should set BookmarkTreeNodeUnmodifiable = "managed" at least for the roots, since it's dangerous to edit/remove those and in some cases Bookmarks.jsm itself throws (for example if you try to remove a root)

@@ +82,5 @@
> +  return {
> +    bookmarks: {
> +      get: function(idOrIdList, callback) {
> +        let list = Array.isArray(idOrIdList) ? idOrIdList : [idOrIdList];
> +        Promise.all(list.map(id => Bookmarks.fetch({guid: id}))).then(results => {

I couldn't see what the API expects when some/all of the passed in ids do not exist.
As it is now, you are using promise.all, that means this will return something only if all the promises resolve, if any rejects this won't return anything. You may rather want to use a Task and try/catch.
I assume if no id is found this should return an empty array? I honestly think such an API would be prone to hide coding mistakes, but if we are aiming at compatibility we should act the same.

@@ +83,5 @@
> +    bookmarks: {
> +      get: function(idOrIdList, callback) {
> +        let list = Array.isArray(idOrIdList) ? idOrIdList : [idOrIdList];
> +        Promise.all(list.map(id => Bookmarks.fetch({guid: id}))).then(results => {
> +          runSafe(context, callback, results.map(convert));

If you get a simple id, and that id is a folder, does this expect to get the .children property? The documentation is not clear about that.
Cause in such a case it's more likely most APIs will need to use promiseBookmarksTree...

@@ +89,5 @@
> +      },
> +
> +      getChildren: function(id, callback) {
> +        // Todo: We should optimize this.
> +        getTree(id, true).then(result => {

Same question about an invalid id, what's expected to happen? I'm assuming at this point the question is valid for all APIs and some sort of error management is needed all around here.

@@ +111,5 @@
> +
> +      create: function(bookmark, callback) {
> +        let info = {
> +          title: bookmark.title || "",
> +        };

I think Bookmarks.jsm is able to manage an undefined/null title.

@@ +116,5 @@
> +
> +        // If url is NULL or missing, it will be a folder.
> +        if ("url" in bookmark && bookmark.url !== null) {
> +          info.type = Bookmarks.TYPE_BOOKMARK;
> +          info.url = bookmark.url || "";

TYPE_BOOKMARK is the default, there's no need to do anything if an url is defined.
So I think this whole if/else can just become something like if (!url) { /* folder */ }

@@ +131,5 @@
> +        } else {
> +          info.parentGuid = Bookmarks.unfiledGuid;
> +        }
> +
> +        // Todo: error handling?

yeah :)

@@ +139,5 @@
> +          }
> +        });
> +      },
> +
> +      // move

our update can also move

Comment 6

[Tom S [:evilpie]]

Thanks Macro, your feedback has been very useful.

(In reply to Marco Bonardo [::mak] from comment #5)
> Comment on attachment 8667861 [details] [diff] [review]
> v1 - Partially implement chrome.bookmarks
> 
> Review of attachment 8667861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> it mostly looks good, but the original API is very badly documented and
> missing a lot of edge cases, error handling, behavior (recursive or not), so
> it's hard to figure the 1:1 compatibility.
> 
> ::: toolkit/components/extensions/ext-bookmarks.js
> @@ +23,5 @@
> > +      treenode.parentId = parent.guid;
> > +    }
> > +
> > +    if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE) {
> > +      // This isn't quite correct. Recently Bookmarked ends up here ...
> 
> Yes, Places queries ARE bookmarks, not folders. I don't know if you want to
> filter them out or what... their URIs start with "place:"
> 
> @@ +28,5 @@
> > +      treenode.url = node.uri;
> > +    } else {
> > +      treenode.dateGroupModified = node.lastModified / 1000;
> > +
> > +      if (node.children && !onlyChildren) {
> 
> is getChildren recursive? cause this is not.
> 
It is not recursive from what I can tell. Otherwise it would not really be different from getSubTree.
> @@ +41,5 @@
> > +    excludeItemsCallback: aItem => {
> > +      if (aItem.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR)
> > +        return true;
> > +      return aItem.annos &&
> > +             aItem.annos.find(a => a.name == PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);
> 
> I don't know if in this case you care about EXCLUDE_FROM_BACKUP_ANNO.
> This annotation is used to avoid backing up some bookmarks (like smart
> queries) that we built automatically and are not created by the user, cause
> in a backup they may create issues (double creation on restore, broken
> shortcuts and so on). For an API like this you likely don't care to exclude
> them.
> 
Chrome only exposes normal bookmarks and tabs, nothing else. So I wanted to exclude everything that is not a normal bookmark. Just exposing them as normal bookmarks with a special url works, however that looks weird in the addons that I tried. They look like normal bookmarks and can't be clicked, while in Firefox they usually look more like a folder.
> @@ +43,5 @@
> > +        return true;
> > +      return aItem.annos &&
> > +             aItem.annos.find(a => a.name == PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);
> > +    },
> > +    includeItemIds: true
> 
> yes, you likely don't want itemIds, we need them in some internal code until
> we finish transitioning to guids.
> 
Sorry just forgot about that. I assumed using guids for everything from the beginning would be the right decision.
> @@ +68,5 @@
> > +    node.parentId = result.parentGuid;
> > +  }
> > +
> > +  if (result.type == Bookmarks.TYPE_BOOKMARK) {
> > +    node.url = result.url.toString(); // Output is always URL object.
> 
> .href is a little bit more specific than toString
> 
> @@ +70,5 @@
> > +
> > +  if (result.type == Bookmarks.TYPE_BOOKMARK) {
> > +    node.url = result.url.toString(); // Output is always URL object.
> > +  } else {
> > +    if (result.type != Bookmarks.TYPE_FOLDER) throw "HOW?!";
> 
> so basically, one can pass in a separator guid...
> 
Oh right, coming up with a separator guid is probably not that easy. Could we use something like an assert here?
> @@ +71,5 @@
> > +  if (result.type == Bookmarks.TYPE_BOOKMARK) {
> > +    node.url = result.url.toString(); // Output is always URL object.
> > +  } else {
> > +    if (result.type != Bookmarks.TYPE_FOLDER) throw "HOW?!";
> > +    node.dateGroupModified = result.lastModified.valueOf();
> 
> and .getTime() is a bit more specific too.
> 
> @@ +74,5 @@
> > +    if (result.type != Bookmarks.TYPE_FOLDER) throw "HOW?!";
> > +    node.dateGroupModified = result.lastModified.valueOf();
> > +  }
> > +
> > +  return node;
> 
> I wonder if we should set BookmarkTreeNodeUnmodifiable = "managed" at least
> for the roots, since it's dangerous to edit/remove those and in some cases
> Bookmarks.jsm itself throws (for example if you try to remove a root)
> 
Chrome has two special folders as well that can't be modified, but they don't set it for those either. Sidenote: Maybe we should map Chrome's "Other Bookmarks" to our "Unsorted Bookmarks" and vice versa?
> @@ +82,5 @@
> > +  return {
> > +    bookmarks: {
> > +      get: function(idOrIdList, callback) {
> > +        let list = Array.isArray(idOrIdList) ? idOrIdList : [idOrIdList];
> > +        Promise.all(list.map(id => Bookmarks.fetch({guid: id}))).then(results => {
> 
> I couldn't see what the API expects when some/all of the passed in ids do
> not exist.
> As it is now, you are using promise.all, that means this will return
> something only if all the promises resolve, if any rejects this won't return
> anything. You may rather want to use a Task and try/catch.
> I assume if no id is found this should return an empty array? I honestly
> think such an API would be prone to hide coding mistakes, but if we are
> aiming at compatibility we should act the same.
> 
Ugh, I didn't realize that. We probably need to use .catch and invoke the callback with an empty array. And look into implement runtime.lastError.
> @@ +83,5 @@
> > +    bookmarks: {
> > +      get: function(idOrIdList, callback) {
> > +        let list = Array.isArray(idOrIdList) ? idOrIdList : [idOrIdList];
> > +        Promise.all(list.map(id => Bookmarks.fetch({guid: id}))).then(results => {
> > +          runSafe(context, callback, results.map(convert));
> 
> If you get a simple id, and that id is a folder, does this expect to get the
> .children property? The documentation is not clear about that.
> Cause in such a case it's more likely most APIs will need to use
> promiseBookmarksTree...
> 
At least Chrome doesn't expose it in that case. And again it would make getChildren pointless. Good for us here :)
> @@ +89,5 @@
> > +      },
> > +
> > +      getChildren: function(id, callback) {
> > +        // Todo: We should optimize this.
> > +        getTree(id, true).then(result => {
> 
> Same question about an invalid id, what's expected to happen? I'm assuming
> at this point the question is valid for all APIs and some sort of error
> management is needed all around here.
> 
It seems like chrome usually invokes the callback with some sort of empty result and sets lastError.
> @@ +111,5 @@
> > +
> > +      create: function(bookmark, callback) {
> > +        let info = {
> > +          title: bookmark.title || "",
> > +        };
> 
> I think Bookmarks.jsm is able to manage an undefined/null title.
> 
> @@ +116,5 @@
> > +
> > +        // If url is NULL or missing, it will be a folder.
> > +        if ("url" in bookmark && bookmark.url !== null) {
> > +          info.type = Bookmarks.TYPE_BOOKMARK;
> > +          info.url = bookmark.url || "";
> 
> TYPE_BOOKMARK is the default, there's no need to do anything if an url is
> defined.
> So I think this whole if/else can just become something like if (!url) { /*
> folder */ }
> 
I think you are right, but the comment for insert wrongly talks about it as a required property.
> @@ +131,5 @@
> > +        } else {
> > +          info.parentGuid = Bookmarks.unfiledGuid;
> > +        }
> > +
> > +        // Todo: error handling?
> 
> yeah :)
> 
> @@ +139,5 @@
> > +          }
> > +        });
> > +      },
> > +
> > +      // move
> 
> our update can also move

Comment 7

[Tom S [:evilpie]]

Attachment: v2 - Partially implement chrome.bookmarks

Comment 8

[Tom S [:evilpie]]

Comment on attachment 8671835 [details] [diff] [review]
v2 - Partially implement chrome.bookmarks

I promise to work on this more in future, but it would be awesome if we could get this landed first.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8671835 [details] [diff] [review]
v2 - Partially implement chrome.bookmarks

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

Yeah, this seems good enough to me. Please file a follow-up on using lastError and make it depend on bug 1190680.

::: toolkit/components/extensions/ext-bookmarks.js
@@ +62,5 @@
> +  let node = {
> +    id: result.guid,
> +    title: result.title || "",
> +    index: result.index,
> +    dateAdded: result.dateAdded.valueOf(),

Should this also use getTime()?

@@ +91,5 @@
> +            let bookmark;
> +            try {
> +              bookmark = yield Bookmarks.fetch({guid: id});
> +              if (!bookmark) {
> +                // ToDo: set lastError, not found

Please use TODO rather than ToDo or Todo.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54b2262a04e5

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54b2262a04e5

Comment 12

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks