Closed Bug 1293853 Opened 8 years ago Closed 7 years ago

Add support for separators to bookmarks API

Categories

(WebExtensions :: Untriaged, defect, P3)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: sheppy, Assigned: bsilverberg, NeedInfo)

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved]triaged[bookmarks][outreach][awe:firefoxdav@icloud.com])

Attachments

(3 files)

The WebExtensions bookmarks API currently doesn't let you create separators, and leaves separators out from result lists. These capabilities should be implemented.
Yeah, this seems like something we should do. I wonder if we should just implement this as bookmark nodes without titles or URLs, since folders are implemented with the same sort of hack: nodes with titles but no URLs. (Ugh.)
Whiteboard: [design-decision-needed]
Probably not, while testing the bookmarks API I came across extensions that assumed that no URL means a folder node.
Yeah, unfortunately, since the "no URL means a folder" rule is already established (it's how both our and Google's docs say to identify a folder), that ship has probably sailed. We'll have to find another way.

Realistically, we should add a |type| field to the BookmarkTreeNode object which can be one of bookmarks.TYPE_BOOKMARK, bookmarks.TYPE_FOLDER, or bookmarks.TYPE_SEPARATOR. That could be set and provide the information without breaking existing add-ons which assume no URL means a folder, but we can then remove the recommendation to do it that way so that bad design pattern can fade away in favor of using the type.
Fair enough, but I don't think that solves the compatibility problem. Even if we add a type field (which I think is a good idea), separator nodes will still have no URL or title, and will still not be folders.
Whiteboard: [design-decision-needed] → [design-decision-needed]triaged
(In reply to Kris Maglione [:kmag] from comment #4)
> Fair enough, but I don't think that solves the compatibility problem. Even
> if we add a type field (which I think is a good idea), separator nodes will
> still have no URL or title, and will still not be folders.

The title is easy enough: just report "-" as the title for separators; that actually has precedent here and there anyway. As for the URL, maybe you can do something like "data:text/plain;-". Attempts to open it just get a "-" character, but it's a valid URL.
Priority: -- → P3
Whiteboard: [design-decision-needed]triaged → [design-decision-approved]triaged
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Assignee: bob.silverberg → mstriemer
Whiteboard: [design-decision-approved]triaged → [design-decision-approved]triaged[bookmarks]
Regarding the two viable alternatives proposed in the comments above, I would not recommend title = "-" to indicate a separator, for the obvious reason that a user might use that as a legitimate name.  I just tried it, and it works.  I have seen many users assign short 1-3 character titles to folders in their Toolbar, so they can squeeze more into that horizontal space.

The other viable alternative, introducing a `type` property, should work.  I think Comment #4 may be regarding breaking extensions designed to the current WebExtensions API.  Indeed, on output (Firefox to Extension), separators are currently skipped.  So if there are any extensions designed to the current API, they will be getting an empty, untitled folder instead of nothing, which may be unexpected.  But how can you replace nothing with something and still have it recognized as nothing?  I think this is an unavoidable breakage.  On input, the absence of `type` should default to the current (Google Chrome) behavior.

But it seems shameful to introduce another property, a significant difference with Google's Chrome, just for this quite special case, so I'd offer a third viable alternative: Undefined `title` means Separator.

There are no ambiguities with undefined `title` on output.  If I delete the title of a bookmark or folder in Firefox, WebExtensions browser.bookmarks functions return a BookmarksTreeNode whose `title` is an empty string, not undefined.

On input, there is an ambiguity which would require a minor change to the API.  Currently, documentation of browser.bookmarks.create() says that the `title` property is optional, and if not given, the title of the new bookmark becomes an empty string.  Google Chrome documentation is more forgiving, though: it simply says that the `title` is optional but does not specify what happens if not given.  But I cannot think of how or why anyone would actually use the behavior as currently documented.  Creating a folder, or a bookmark, with no title kind of defeats the purpose.

All that being said, I can live with any of the three alternatives: undefined `title`, "-" `title`, or `type`, or any other viable alternative.  How do we get a design decision so we can proceed?
Flags: needinfo?(mstriemer)
(In reply to Jerry Krinock from comment #7)

> On input, there is an ambiguity which would require a minor change to the
> API.  Currently, documentation of browser.bookmarks.create() says that the
> `title` property is optional, and if not given, the title of the new
> bookmark becomes an empty string.  Google Chrome documentation is more
> forgiving, though: it simply says that the `title` is optional but does not
> specify what happens if not given.  But I cannot think of how or why anyone
> would actually use the behavior as currently documented.  Creating a folder,
> or a bookmark, with no title kind of defeats the purpose.

Certainly the UI in Firefox automatically subs in the URL even if you specify an empty title, or try to leave it out entirely (such as bookmarking a page with no <title> and deleting the contents of the Name box in the new bookmark pane).

The no-title option is viable... but not necessarily clear.

> All that being said, I can live with any of the three alternatives:
> undefined `title`, "-" `title`, or `type`, or any other viable alternative. 
> How do we get a design decision so we can proceed?

Good question.

I feel like the `type` property addition is the wisest move for the long term; instead of being a hack or gaming an existing field, it's a clean solution that offers room to expand in the future if necessary. For instance, perhaps someday it's useful to have a type indicating that a bookmark is a "virtual" bookmark, generated by the browser to represent something useful, or a "cloud" bookmark, meaning one that's not on the device itself but is on the user's cloud account. Things like that. The best time to add something like a new property is now, before WebExtensions are any more widely used than they already are.

As someone who documents these things, and has to explain how they work, the `type` notion appeals to me greatly, as the solution which is the clearest to use. But, I agree that I could in principle live with any of those options.
Thank you, Eric.  I think you've stated the design tradeoffs quite well.  I would add that, although the new `type` property would simplify development of a new Firefox-only extension, one of the "gaming" alternatives would simplify development of a universal Firefox/Chrome/Opera extension, or updating an existing extension.

This morning, I thought of a fourth viable alternative which seems to have no conflicts with the existing API: Indicate Separator by assigning to `title` a JavaScript null.  On input, testing in the current nightly, passing an object with title = null to browser.bookmarks.create() or browser.bookmarks.update() causes the promise to reject with an "Invalid bookmark" message.  So we know no one is currently doing that.  To verify that this will work on output, I just patched one of the `convert()` functions in mozilla-central/browser/components/extensions/ext-bookmarks.js to return a null title for a certain bookmark.  It worked.  My extension got it and converted to a JSON null, as expected.  I just searched the API for all of the browser.bookmarks functions and, although null `url` has a meaning, none of them mention anything about null `title`.

We just need a decision now :)
I hate to be a nag, but since Firefox 53 crashes when running my XUL-based extension which supports separators, I need a design decision on the API soon so that I can finish my WebExtension.  There are four alternatives, fairly well considered in the comments above.  I presume that someone in the CC list has the authority to make the decision.  I apologize if this is not an appropriate comment here.
I think making it look like a bookmark is likely the best way to go here. It should be compatible with existing extensions as far as not breaking them goes, but they might not behave correctly for separators.

Bookmark: { title: "My bookmark", url: "http://example.com", type: bookmarks.TYPE_BOOKMARK }
Folder: { title: "My folder", type: bookmarks.TYPE_FOLDER }
Separator: { title: "-", url: "data:", type: bookmarks.TYPE_SEPARATOR }

I figure the types will be Symbols and using bookmarks.Bookmark, bookmarks.Folder, bookmarks.Separator looks nicer to me but could cause some naming collisions? The values for `title` and `url` can be something else but they should be present to make it look like a bookmark.

Does this seem good to implement, Kris?
Flags: needinfo?(mstriemer) → needinfo?(kmaglione+bmo)
Bob, does the above API seem good to you?
Flags: needinfo?(bob.silverberg)
(In reply to Mark Striemer [:mstriemer] from comment #11)
> I think making it look like a bookmark is likely the best way to go here. It
> should be compatible with existing extensions as far as not breaking them
> goes, but they might not behave correctly for separators.
> 
> Bookmark: { title: "My bookmark", url: "http://example.com", type:
> bookmarks.TYPE_BOOKMARK }
> Folder: { title: "My folder", type: bookmarks.TYPE_FOLDER }
> Separator: { title: "-", url: "data:", type: bookmarks.TYPE_SEPARATOR }
> 

If we're going to add a type, which I think is a good idea, then we don't need a title for separators, do we? I think the suggestion of using a "-" was in case we wanted to be able to create a separator using the existing method signature, but if we introduce type then it becomes unnecessary. We can just return an empty string for title.

> I figure the types will be Symbols and using bookmarks.Bookmark,
> bookmarks.Folder, bookmarks.Separator looks nicer to me but could cause some
> naming collisions? 

We can introduce a new enumerated type for the bookmarkType in bookmarks.json and use "friendlier" names like "bookmark", "folder" and "separator". I'm not sure what sorts of collisions you are anticipating.

For the create() function, as suggested above, we can make type optional and if it is missing assume that no url means a folder, just as the code does now.
Flags: needinfo?(bob.silverberg)
Whiteboard: [design-decision-approved]triaged[bookmarks] → [design-decision-approved]triaged[bookmarks][outreach]
Whiteboard: [design-decision-approved]triaged[bookmarks][outreach] → [design-decision-approved]triaged[bookmarks][outreach][awe:firefoxdav@icloud.com]
(In reply to Bob Silverberg [:bsilverberg] from comment #13)

> > Separator: { title: "-", url: "data:", type: bookmarks.TYPE_SEPARATOR }
> 
> If we're going to add a type, which I think is a good idea, then we don't
> need a title for separators, do we? I think the suggestion of using a "-"
> was in case we wanted to be able to create a separator using the existing
> method signature, but if we introduce type then it becomes unnecessary. We
> can just return an empty string for title.

The value to including title and url fields on the separator object is that it means that existing extensions that don't know to check the type will not break. They'll just see what looks like a bookmark with the title "-" and the URL "data:" or "about:blank" (which would be my suggestion).
(In reply to Eric Shepherd [:sheppy] from comment #15)
> (In reply to Bob Silverberg [:bsilverberg] from comment #13)
> 
> > > Separator: { title: "-", url: "data:", type: bookmarks.TYPE_SEPARATOR }
> > 
> > If we're going to add a type, which I think is a good idea, then we don't
> > need a title for separators, do we? I think the suggestion of using a "-"
> > was in case we wanted to be able to create a separator using the existing
> > method signature, but if we introduce type then it becomes unnecessary. We
> > can just return an empty string for title.
> 
> The value to including title and url fields on the separator object is that
> it means that existing extensions that don't know to check the type will not
> break. They'll just see what looks like a bookmark with the title "-" and
> the URL "data:" or "about:blank" (which would be my suggestion).

I can see why we need something in the url field so that existing code won't think the the separators are folders, but why do we need a title? Isn't an empty title valid? How will that break existing extensions?
Flags: needinfo?(kmaglione+bmo)
Assignee: mstriemer → bob.silverberg
(In reply to Bob Silverberg [:bsilverberg] from comment #16)
> (In reply to Eric Shepherd [:sheppy] from comment #15)
> > (In reply to Bob Silverberg [:bsilverberg] from comment #13)
> > 
> > > > Separator: { title: "-", url: "data:", type: bookmarks.TYPE_SEPARATOR }
> > > 
> > > If we're going to add a type, which I think is a good idea, then we don't
> > > need a title for separators, do we? I think the suggestion of using a "-"
> > > was in case we wanted to be able to create a separator using the existing
> > > method signature, but if we introduce type then it becomes unnecessary. We
> > > can just return an empty string for title.
> > 
> > The value to including title and url fields on the separator object is that
> > it means that existing extensions that don't know to check the type will not
> > break. They'll just see what looks like a bookmark with the title "-" and
> > the URL "data:" or "about:blank" (which would be my suggestion).
> 
> I can see why we need something in the url field so that existing code won't
> think the the separators are folders, but why do we need a title? Isn't an
> empty title valid? How will that break existing extensions?

Eric, the above question was directed at you, but I neglected to needinfo you, so you may not have noticed it. I am about to start implementing this, and my inclination it to just leave the title blank for separators, as I don't think we need the "-" in the title. You suggested otherwise though. Can you take a look at my last comment and let me know if you still think we need a "-" in the title?
Flags: needinfo?(eshepherd)
Marco, I've come across an inconsistency which is going to require some extra code in our bookmarks API to accommodate, so I'd like to deal with it in PlacesUtils.jsm instead. It has to do with the data that is returned for the `type` of a places node from different functions in PlacesUtils.jsm and Bookmarks.jsm.

We call two different methods in Bookmarks.jsm to retrieve bookmark information: search() and fetch(). Both of those return node types in numeric format (e.g., PlacesUtils.bookmarks.TYPE_BOOKMARK). We also call a method in PlacesUtils.jsm when we need to return a tree or partial tree: PlacesUtils.promiseBookmarksTree(). That returns node types in string format (e.g., PlacesUtils.TYPE_X_MOZ_PLACE). I am trying to avoid having to have our API code (in ext-bookmarks.js) know about these two different types of types. Most of our code works with the numeric types (because most of it interacts with Bookmarks.jsm), so I'd like to have promiseBookmarksTree() also return a numeric code. I imagine that I cannot just switch the format of the type it returns as there must be other consumers, but I was thinking that maybe I could add an additional property to the nodes returned by promiseBookmarksTree which contains the type in numeric format.

What do you think of that idea? If you don't think that's a good approach, what else might you suggest?
Flags: needinfo?(mak77)
(In reply to Bob Silverberg [:bsilverberg] from comment #18)
> Marco, I've come across an inconsistency which is going to require some
> extra code in our bookmarks API to accommodate, so I'd like to deal with it
> in PlacesUtils.jsm instead. It has to do with the data that is returned for
> the `type` of a places node from different functions in PlacesUtils.jsm and
> Bookmarks.jsm.

Yes the original plan was/is to provide a fetchTree API from Bookmarks.jsm that provides an API consistent with the rest of Bookmarks.jsm, but internally (for now) it just wrap promiseBookmarksTree (doing a translation of the output). Long term the things will be the opposite, fetchTree will be the official API, and PlacesUtils will provide some sort of translation.
Considered add-ons now can't use internals, I'd also be fine with a quick implementation that doesn't take into account all of the possible edge cases if we're in a hurry.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #19)
> (In reply to Bob Silverberg [:bsilverberg] from comment #18)
> > Marco, I've come across an inconsistency which is going to require some
> > extra code in our bookmarks API to accommodate, so I'd like to deal with it
> > in PlacesUtils.jsm instead. It has to do with the data that is returned for
> > the `type` of a places node from different functions in PlacesUtils.jsm and
> > Bookmarks.jsm.
> 
> Yes the original plan was/is to provide a fetchTree API from Bookmarks.jsm
> that provides an API consistent with the rest of Bookmarks.jsm, but
> internally (for now) it just wrap promiseBookmarksTree (doing a translation
> of the output). Long term the things will be the opposite, fetchTree will be
> the official API, and PlacesUtils will provide some sort of translation.
> Considered add-ons now can't use internals, I'd also be fine with a quick
> implementation that doesn't take into account all of the possible edge cases
> if we're in a hurry.

Thanks Marco. I've made a small change to support this and have flagged you for review on that, and also a change to Bookmarks.jsm.
Attachment #8902354 - Flags: review?(mak77) → review?(standard8)
Attachment #8902355 - Flags: review?(mak77) → review?(standard8)
Comment on attachment 8902354 [details]
Bug 1293853 - Part 1: Add numeric typeCode to PlacesUtils.promiseBookmarksTree,

https://reviewboard.mozilla.org/r/173908/#review179992
Attachment #8902354 - Flags: review?(standard8) → review+
Comment on attachment 8902355 [details]
Bug 1293853 - Part 2: Include separators in results from bookmarks.search,

https://reviewboard.mozilla.org/r/173910/#review179994

Looks good, thanks.
Attachment #8902355 - Flags: review?(standard8) → review+
Comment on attachment 8902356 [details]
Bug 1293853 - Part 3: Add support for separators to bookmarks API,

https://reviewboard.mozilla.org/r/173912/#review179666

r+ with suggested changes, otherwise ping me back.

::: browser/components/extensions/ext-bookmarks.js:13
(Diff revision 1)
>  XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
>                                    "resource://gre/modules/PlacesUtils.jsm");
>  
> +const BOOKMARK_TYPE = PlacesUtils.bookmarks.TYPE_BOOKMARK;
> +const FOLDER_TYPE = PlacesUtils.bookmarks.TYPE_FOLDER;
> +const SEPARATOR_TYPE = PlacesUtils.bookmarks.TYPE_SEPARATOR;

Can we use the same name?

const {
  TYPE_BOOKMARK,
  TYPE_FOLDER,
  TYPE_SEPARATOR,
} = PlacesUtils.bookmarks;

::: browser/components/extensions/ext-bookmarks.js:24
(Diff revision 1)
> +]);
> +
> +let API_TYPES_TO_BOOKMARKS_TYPES_MAP = new Map();
> +for (let [code, name] of BOOKMARKS_TYPES_TO_API_TYPES_MAP) {
> +  API_TYPES_TO_BOOKMARKS_TYPES_MAP.set(name, code);
> +}

its small, but it could be a lazygetter

::: browser/components/extensions/ext-bookmarks.js:51
(Diff revision 1)
>          treenode.children = node.children
>            ? node.children.map(child => convert(child, node))
>            : [];
>        }
> +    } else {
> +      treenode.url = node.typeCode == BOOKMARK_TYPE ? node.uri : "data:";

Can we make a const for the "data:" string with a descriptive name?  IIRC "data:" is a seperator.  So perhaps BOOKMARK_SEPERATOR_URL = "data:".  

Also, are there other types that would be "data:"?  The logic seems to indicate that any type (future) other than folder or bookmark has a data url.  Maybe be explicit:

if (folder) ...
else if (seperator) ...
else ...expect it has url...

there are a few occurances of this.

::: browser/components/extensions/ext-bookmarks.js:90
(Diff revision 1)
> -  if (result.type == PlacesUtils.bookmarks.TYPE_BOOKMARK) {
> +  if (result.type == FOLDER_TYPE) {
> -    node.url = result.url.href; // Output is always URL object.
> -  } else {
>      node.dateGroupModified = result.lastModified.getTime();
> +  } else {
> +    node.url = result.type == BOOKMARK_TYPE ? result.url.href : "data:";

again

::: browser/components/extensions/ext-bookmarks.js:120
(Diff revision 1)
> -    if (itemType == PlacesUtils.bookmarks.TYPE_BOOKMARK) {
> +    if (itemType == FOLDER_TYPE) {
> -      bookmark.url = uri.spec;
> -    } else {
>        bookmark.dateGroupModified = bookmark.dateAdded;
> +    } else {
> +      bookmark.url = itemType == BOOKMARK_TYPE ? uri.spec : "data:";

again

::: browser/components/extensions/ext-bookmarks.js:241
(Diff revision 1)
> -            info.type = PlacesUtils.bookmarks.TYPE_FOLDER;
> +              info.type = FOLDER_TYPE;
> +            }
> +          }
> +
> +          if (info.type === BOOKMARK_TYPE) {
> +            info.url = bookmark.url || "";

Should the url be empty here?  You're using data: other palces.
Attachment #8902356 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8902356 [details]
Bug 1293853 - Part 3: Add support for separators to bookmarks API,

https://reviewboard.mozilla.org/r/173912/#review179666

> Can we use the same name?
> 
> const {
>   TYPE_BOOKMARK,
>   TYPE_FOLDER,
>   TYPE_SEPARATOR,
> } = PlacesUtils.bookmarks;

Good idea. Thanks!

> Should the url be empty here?  You're using data: other palces.

We only use "data:" to populate the url of the BookmarkTreeNode object for output. This method is creating a bookmark and we do not want to create a bookmark in the places database with a URL of "data:" for a separator, hence "" and not "data:".
Attachment #8902354 - Flags: review?(mak77)
Attachment #8902355 - Flags: review?(mak77)
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d38f59d311e
Part 1: Add numeric typeCode to PlacesUtils.promiseBookmarksTree, r=standard8
https://hg.mozilla.org/integration/autoland/rev/3d5a624bfa82
Part 2: Include separators in results from bookmarks.search, r=standard8
https://hg.mozilla.org/integration/autoland/rev/89c9b17b0cfc
Part 3: Add support for separators to bookmarks API, r=mixedpuppy
Flags: needinfo?(eshepherd)
Is manual testing required on this bug? If yes, please provide some STR and the proper webextension.
Flags: needinfo?(eshepherd)
(In reply to Will Bamberg [:wbamberg] from comment #34)

Thanks Will, these look good for the most part. I've added a few comments below...

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

This page opens with "An object of type bookmarks.BookmarkTreeNode represents a node in the bookmark tree, where each node is a bookmark or bookmark folder." It should state that a node could also be a separator.

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

Here as well: "The CreateDetails type is used to describe the properties of a new bookmark or bookmark folder when calling the bookmarks.create() method." Separator should be added to that list.

I'm not sure what the note that says "At this time, this object doesn't have a way to describe a separator within a bookmark list." actually means. Is this no longer accurate?
Flags: needinfo?(bob.silverberg)
Thanks Bob.

> I'm not sure what the note that says "At this time, this object doesn't have a way to describe a separator within a bookmark list." actually means. Is this no longer accurate?

It means I forgot to remove this note. I got it in one page, but not the additional one. It's gone now. Thanks!
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.