1276817 - Support bookmark keywords

Status: RESOLVED DUPLICATE of bug 1268401

(WebExtensions :: General, defect, P5)

Description

[Jerry Krinock]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

I read the API documentation for WebExtensions > chrome.bookmarks,  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks.


Actual results:

I did not find any capability to *get* or *set* bookmark Keyword attribute.


Expected results:

Because WebExtensions are slated to replace the Add-Ons SDK, and because bookmarks extensions which sync bookmarks require access to all bookmark attribute, either

(1) The chrome.bookmarks API implemented by WebExtensions for Firefox should contain additional functions to *get* and *set* bookmark Keyword, or,

(2) There should be a link to some Mozilla policy statement which publicly states that bookmark Keyword is obsolete and will be removed in some future version of Firefox.

Comment 1

[Jerry Krinock]

I am working on a patch to fix this bug.

Comment 2

[Tomislav Jovanovic :zombie]

Hey Jerry, assigned the bug to you. Feel free to ask if you need help with anything, either here or in irc #webextensions.

Comment 3

[Jerry Krinock]

Nice to have a mentor on this :)

I think that the core task here is to add several methods in mozilla-central/browser/components/extensions/ext-bookmarks.js.  Each method shall wrap around one of the methods of PlacesUtils.keywords.  For example, I imagine a new chrome.bookmarks.getKeywords() method.  It shall wrap around PlacesUtils.keywords.fetch(), similar to how the existing method chrome.bookmarks.get() wraps around PlacesUtils.bookmarks.fetch().

Is this correct?

Comment 4

[Tomislav Jovanovic :zombie]

That sound like a good start, but you will also need to add the new methods/properties to the api schema \extensions/schemas/bookmarks.json.

Though I'm not too familiar with bookmarks, to me it seems a keyword should be a property of the bookmark item.  What do you propose getKeywords() method should do?  Can the same functionality be achieved by extending the existing search() method?

Maybe you should start by explaining the usecase for the api in your addon, and work backwards from there?

Comment 5

[Jerry Krinock]

(In reply to Tomislav Jovanovic :zombie from comment #4)
> you will also need to add the new methods/properties to the api schema ... bookmarks.json.

Indeed.

> Though I'm not too familiar with bookmarks, to me it seems a keyword should
> be a property of the bookmark item.

Almost.  Apparently, that was changed a few years ago.  Currently, a keyword is a property of a uri ("place"), and there may be multiple keywords per uri.

* * *

> What do you propose getKeywords() method should do?

To fetch the keywords for a given uri.

However, yesterday, after successfully implementing getKeywords() and considering your later comments, I realized that this is maybe not the correct approach for the proposed API.  More later.

> Can the same functionality be achieved by extending the
> existing search() method?

Probably not, but let's defer that discussion since getKeywords() may not be the correct approach.

> Maybe you should start by explaining the use case for the api in your addon,
> and work backwards from there?

My extension synchronizes bookmarks and places with the user interface in my app, and optionally with other browsers and web services (Pinboard, etc.).  One thing this requires is to import bookmarks, with all of their properties, from Firefox.

* * *

To proceed further, I need a technical pointer.  In ext-bookmarks.js, line 89 is:

      let bookmark = yield PlacesUtils.bookmarks.fetch({guid: id});

I want to see the implementation of PlacesUtils.bookmarks, and this fetch().  It seems like it should be in this file:

      mozilla-central/toolkit/components/places/PlacesUtils.jsm

In that file, I can find PlacesUtils.keywords (line 2131) and PlacesUtils.keywords.fetch() (2145), but I cannot find PlacesUtils.bookmarks.  How could I find that?

Comment 6

[Bob Silverberg [:bsilverberg]]

(In reply to Jerry Krinock from comment #5)
> 
> To proceed further, I need a technical pointer.  In ext-bookmarks.js, line
> 89 is:
> 
>       let bookmark = yield PlacesUtils.bookmarks.fetch({guid: id});
> 
> I want to see the implementation of PlacesUtils.bookmarks, and this fetch().
> It seems like it should be in this file:
> 
>       mozilla-central/toolkit/components/places/PlacesUtils.jsm
> 
> In that file, I can find PlacesUtils.keywords (line 2131) and
> PlacesUtils.keywords.fetch() (2145), but I cannot find
> PlacesUtils.bookmarks.  How could I find that?

Hey Jerry, thanks for taking this on! I believe what you are looking for can be found at https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#544.

Comment 7

[Jerry Krinock]

Thank you, Bob.  That is what I was looking for.

Comment 8

[Tomislav Jovanovic :zombie]

Thanks Bob.

(In reply to Jerry Krinock from comment #5)
> > Though I'm not too familiar with bookmarks, to me it seems a keyword should
> > be a property of the bookmark item.
> 
> Almost.  Apparently, that was changed a few years ago.  Currently, a keyword
> is a property of a uri ("place"), and there may be multiple keywords per uri.
I see.  I tried editing keywords on two different bookmarks pointing to the same url, and indeed, they seem to be shared.  But that still sounds like implementation detail.  I think we can keep things sane if we mimic what the user can do to bookmarks/keywords through the Firefox UI.  So I still suggest to try to make a keyword a property of the bookmark item, that work out the updating under the hood.

Comment 9

[Tomislav Jovanovic :zombie]

> that work out the updating under the hood.
*then work out the updating under the hood.

Also, with my suggested approach, keyword property may need to be `keywords` (an array), though I couldn't figure out how to set multiple keywords for different bookmarks pointing to the same url, they keep overwriting each other.

Comment 10

[Jerry Krinock]

(In reply to Tomislav Jovanovic :zombie from comment #8)
> I think we can keep things sane if we mimic what the
> user can do to bookmarks/keywords through the Firefox UI.  So I still
> suggest to try to make a keyword a property of the bookmark item, then work
> out the updating under the hood.

Indeed, this was the approach I was alluding to in Comment #5.  There are, I think, two broad approaches to creating the API for keywords (and its "sister" "Firefox-only" properties: tags, descriptions, live bookmarks).

Approach #1 is "The Full Mozilla".  This is what I suggested in Comment #3.  We would add methods to chrome.bookmarks which are wrappers around methods in PlacesUtils.  This will enable extension developers to do all of the wonderful things that are possible only in Firefox such as, for example, assigning keywords to URLs ("places") instead of bookmarks.  

Approach #2 is "Simple Like Chrome".  In this approach, we may not add any new methods at all, but will instead add several properties to chrome.bookmarks.BookmarkTreeNode.  As you stated, `keywords` (an array), would be one of them.  This approach may be slightly "lossy" – there may be some operations that are possible within Firefox that are not possible via the chrome.bookmarks API, because some of the hidden implementation details may in fact have a few external effects.

In fact, Approach #2 was already used when someone added the `url` property to BookmarkTreeNode.  This simplification hides the implementation detail that, in Firefox, urls are in fact not properties of moz_bookmarks but are in a related table, moz_places.

After looking at some of the code and considering the learning curve presented to extension developers, I also now advocate the "Simple Like Chrome" Approach #2.  I plan to abandon the `getKeywords()` method.

Comment 11

[Jerry Krinock]

(In reply to Tomislav Jovanovic :zombie from comment #9)
> I couldn't figure out how to set multiple keywords for different bookmarks pointing to the
> same url, they keep overwriting each other.


Yes, when I noticed that the other day, I was hoping it was a "feature" which I was too ignorant to understand.  But also, notice that the label in the user interface is still the singular "Keyword".  Sadly, I think that the implications of the change in keywords, Bug 1125113, were never propagated to the user interface.

And maybe no one has noticed, possibly because that "Keyword" field is so well hidden!  I would be surprised if more than 1% of Firefox users are currently using Keywords, which is why I suggested the alternative resolution in my original User Story (top of this page), that maybe the Keywords feature should just be dropped.  That would still be fine with me :)

On this same topic, I just noticed a slight ambiguity in Bug 1125113.  In the User Story at the top, Marco says that we need to change to "1 uri - 1 keyword … to simplify management and caching".  Great!  But then later, in Bug 1125113 Comment #1, "we can only have multiple keywords per uri", and "multiple" seems to have prevailed.  Certainly the PlacesUtils.keywords API supports multiple keywords per uri – see comment "Keywords management API", currently line 1963, in https://dxr.mozilla.org/mozilla-beta/source/toolkit/components/places/PlacesUtils.jsm.

Comment 12

[Jerry Krinock]

At ext-bookmarks.js:66 there is a function named `convert`.  It converts a PlacesUtils bookmark item into a WebExtensions BookmarkTreeNode, mapping property names, converting the date format, etc.  It is trivial and synchronous.

Logically, I think that support for reading the `keywords` property (and later `description` and probably `tags` and livemarks) should be added into this `convert` function.   It is close to the "surface" of Firefox.  But the property `keywords` does not exist in PlacesUtils bookmark items.  Its value must be fetched using the `PlacesUtils.Keywords.fetch` function, which is asynchronous.  Some of those other new properties will require asynchronous fetches also.  Therefore, my modified `convert` function will be asynchronous.

Does anyone have any objection to that, or see a better alternative?

Comment 13

[Tomislav Jovanovic :zombie]

(In reply to Jerry Krinock from comment #12)
> Logically, I think that support for reading the `keywords` property (and
> later `description` and probably `tags` and livemarks) should be added into
> this `convert` function.

Then this function wouldn't just be doing the "converting", but also "fetching" of bookmark keywords.  So you should at least rename it, or maybe better, add an explicit async helper function to fetch the keywords, since you would be editing the code around every usage of convert() anyway, if you make it async.

Comment 14

[Jerry Krinock]

(In reply to Tomislav Jovanovic :zombie from comment #13)

Happy Saturday, Tomislav!

> better, add an explicit async helper function to fetch the keywords, since
> you would be editing the code around every usage of convert() anyway, if you
> make it async

Let's see.  This `convert` is called by six `browser.bookmarks` API functions:

•  `get`
•  `search`
•  `getRecent`
•  `create`
•  `move`
•  `update`

In all six of these, `convert` is called to provide either a BookmarkTreeNode object, or an array of such, resolving the Promise that these functions return.  Since the new properties `keywords`, `description`, `tags` etc. are possibly in any BookmarkTreeNode, all six functions would need to call these helper(s) after calling `convert`, in the same way.

So I think that, to avoid 6x code repetition, the new asynchronous fetches should be put into a renamed asynchronous `convert`.  What do you think?

Comment 15

[Tomislav Jovanovic :zombie]

Yeah, sounds good to me.

Comment 16

[Tomislav Jovanovic :zombie]

Hey Jerry, how are you doing with this?  Do you need help if you are stuck with something?

Comment 17

[Jerry Krinock]

Hello, Tomislav.  Good question.

I have not worked on it since November, when I had to set it aside in favor of other business.  Before then, I learned enough of the fancy leading-edge asynchronous JavaScript that y'all are using that I was able to modify a version of Firefox with which I could read and write keywords in a demo extension.  It was fun, and I plan to get back to it within the next couple weeks and have some pull requests by the end of the month.  The one thing that was not fun was Mercurial, which was giving me some ****, and is likely to give me more **** when I pull the latest nightly.  I'll ask if I need help :)

Comment 18

[Tomislav Jovanovic :zombie]

Great, no rush, just wanted to see if you needed help.

(and yeah, hg can be a pain, especially if you learn git first. fairly similar, subtle differences)

Comment 19

[Jerry Krinock]

An API design issue:

`keywords` is a to-many property of a bookmark.  So one might expect me to add functions such as `removeKeyword(id, keyword)`, `insertKeyword(id, keyword)`, `removeAllKeywords(id)`, etcetera to the API.  (id would be a bookmark identifier.)

However, I think that, since 99% of extension developers, and 99.9% of users do not use keywords, and since the Firefox user interface does not even support multiple keywords, we should not complicate the API with these functions.  I am therefore only modifying the existing functions `create(bookmark)`, `get(idOrIdList)` and `update(id, changes)` to support properties `keywords` and `keywordObjects` (keywords with postData).  Both properties are of array type.

This means that if an extension developer wants to, for example, add a third keyword to an existing bookmark which already has two keywords, developer must call `update(id, changes)` and pass an array of all three keywords including the two existing keywords.

Comment 20

[Jerry Krinock]

Regarding the `convert()` function we discussed back in November, actually there are two `convert()` functions.  The first one in the file, defined inside getTree() (which gets an entire tree or subtree of bookmark items) is more interesting.  It is called recursively, once for each child bookmark item of each parent.  So now we're going to be creating Promises recursively.  I wrote code to do it as follows.  It works using a small test bookmarks set on my MacBook Air:

• For each child, create a Promise which resolves to push the "converted" child into the converted parent's children.
• Add each such Promise to an array of promises.
• Pass that array of Promises to Promise.all().
• In the resolve method of that .all, sort the parent's children by index.

(The last step is necessary because the children were pushed asynchronously, in unknown order.)

Theoretically, if a user has the capacity to run N threads simultaneously, and M items in a folder, if M < N, they might all process simultaneously and everyone will smile.  But if a user has folders with many children, and small N, it might be cheaper to begin by only creating the first Promise, to convert child 0, then when that resolves, create a Promise to convert child 1, etc.; because then we would not need the big sort.  But this means: force everything to run on a single thread, 20th-Century style.

I think any difference will be small, and the way I wrote it is fine.  Do code reviewers worry about stuff like this?  Are there any performance tests?

Comment 21

[Jerry Krinock]

Attachment: 1276817-01.diff

I think that the attached proposed patch, 1276817-01.diff, completely fixes this bug.  The WebExtensions bookmark object, BookmarkTreeNode, has a new `keywords` property, which in turn has two sub-properties `keyword` and `postData`; all functions in browser.bookmarks.API which pass bookmarks support this property. 

Two source files are affected.  The file ext-bookmarks.js grows from 10 KB to 22 KB.  All of the API which pass bookmarks now have spliced into them a call to one of the asynchronous PlacesUtils.keywords functions.   The two synchronous `convert()` functions have been replaced with two asynchronous functions, `makeBookmarkTreeNode()` and `makeBookmarkTreeNode_recursive()`.  (The long names are derived from the BookmarkTreeNode name in the WebExtensions API.)  In the future, to fix the siblings of this bug (Bug 1225916, Bug 1276819,  Bug 1276821), which support the sibling Firefox-only "bonus" properties not supported by Chromium (tags, descriptions, livemarks), it should be straightforward to `.then` in other asynchronous calls.  Comments in the code indicate where to do this.

The alternative approach was to modify the PlacesUtils bookmark object to have a `keywords` property and somehow keep this synced to moz_keywords, but that would have been much more deep and complicated.  My approach is to *adapt* to WebExtensions, with "adapters" close to the  "surface" of Firefox.

So far, I've only done manually-verified ”smoke" testing.  Test code in my extension can create a bookmark with keywords and optional postData, update it to different keywords/postData, move it, search for and find it, delete it, etc.  I was prepared to write actual unit tests, but after reviewing Mozilla procedures, I got the impression that MozReview comes first.

I’ve also done a smoke test for performance.  Not surprisingly, the performance hit to fetch keywords for each item before sending it out the door, even if none are found, is not zero.  I loaded a typical tree of about 1300 bookmarks, with no keywords, into Firefox on my 2-core MacBook Air.  Running a debug build of Nightly 2017-02-10 unpatched, the time it takes to get the whole bookmarks tree with browser.bookmarks.getTree() averages 800 milliseconds.  After my patch, it averages 1600 milliseconds.  I think that this performance hit is inherent not so much in the existence of the extra Firefox-only properties, but in the way that Firefox keeps these properties in several related database tables.  It's fetch, fetch, fetch.

If anyone can suggest how to improve performance, please advise.  The alternative approach, which I rejected as explained above, would have had a much lower performance hit on getTree(), but the performance hit(s) would have popped up elsewhere, affecting other internal bookmarks functions, instead of just WebExtensions.  Are there any controlled performance tests we must pass?  What about other platforms and hardware?

Although my patch, and future patches to fix the sibling bugs (desctibed above), can and do spin off promises in parallel to fetch children, and later, to fetch each of the "bonus" properties, such parallelism will not help the typical user who has only two cores available.  Therefore, I expect additional performance hits when we fix the sibling bugs.

Deprecating some of the little-used features such as Keywords was discussed in Bug 1276731, but the answer was that we want to continue to continue to support all of them.  Performance will acceptable for purposes of my bookmarks syncing extension, since it usually works in the background anyhow.  But if the decision-makers want to reconsider deprecating some of these little-used features in light of the performance hits, I’ll be more than happy to shelve this work I’ve done and move on to something else. :)

Comment 22

[Tomislav Jovanovic :zombie]

Comment on attachment 8838107 [details] [diff] [review]
1276817-01.diff

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

This is just a first review pass through your patch.  Honestly, it is currently a bit complicated and hard to read, so I didn't even review the actual algorithm for fetching/updating keywords, regarding the performance issues and all.  Instead, I focused on things that would make the code shorter, simpler, and thus easier to review in the next pass.

General comments:

 * Please use mozreview, it's much easier to follow up with reviews
   http://reviewboard.mozilla.org/

 * Refrain from code changes unrelated to the bug

 * Please run ESLint on your code
   https://developer.mozilla.org/en-US/docs/ESLint

 * Unless you have a good reason, let's leave out the postData from keywords.
   This should reduce the size and thus complexity of the patch significantly.

 * Use async for all new functions that require it, 
   and possibly convert old ones where appropriate.

 * Your comments and variable names are a bit on the verbose side.
   Assume people reading the code can lookup existing APIs, 
   and comment when something non-obvious is required.

::: browser/components/extensions/ext-bookmarks.js
@@ +34,5 @@
> +* @resolves to the passed-in node ("passes thru"), now featuring a .keywords
> +*           property, if any keywords were found
> +*/
> +function syncKeywords(node) {
> +  return Task.spawn(function* () {

Please use async functions (with await), makes code easier to read.

@@ +47,5 @@
> +          gKeywordsCachePromise.cache in PlacesUtils.jsm, which is in turn a
> +          cache of moz_keywords database table.  The `entry` object  has three
> +          properties: keyword, postData, url.  That last one is a URL object,
> +          as in https://developer.mozilla.org/en-US/docs/Web/API/URL.
> +          Of course we don't want that. */

Comments are helpful, but this one (and several others below) are too wordy, and possibly unneeded.  For example, from the signature of the keywords.fetch() function, it's fairly obvious what this simple callback does.

@@ +66,5 @@
> +function getTree(requestedRootGuid) {
> +  /* The following function works like makeBookmarkTreeNode() in the global
> +    namespace below, except this implementation is recursive, returning all
> +    descendants of the given item. */
> +  function makeBookmarkTreeNode_recursive(item) {

I don't understand why you made this change, separating the single getChild from the general getTree method, duplicating code in the process.

@@ +84,5 @@
> +      treenode.dateGroupModified = item.lastModified / 1000;
> +
> +      if (item.children) {
> +        let treenodeChildren = [];
> +        item.children.forEach(child => {

Instead of `forEach`, try using `map`, where each array item is a promise for one child.  This makes the code simpler, and you don't need to sort the items later, as Promise.all() is guaranteed to keep the results in the same order as promises.

(Both here and below).

@@ +187,3 @@
>      excludeItemsCallback: item => {
>        if (item.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) {
> +        /* We ignore separators.  This is a documented limitation. */

Please don't add unrelated comments to existing code.

@@ +190,4 @@
>          return true;
>        }
>        return item.annos &&
> +          item.annos.find(a => a.name == PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);

No unnecessary indentation changes please.

@@ +304,5 @@
> +* equivalent chrome.bookmarks.BookmarkTreeNode (Web Extensions) object
> +* Asnychronous return is necessary because, in addition to the regular
> +* properties id, title, index, etc. which exist in some form in the given
> +* Places.Utils bookmark, there are also "indirect" properties such as
> +* keyword, whose values must be looked up in the Places database.

Comments like this is useful to me, during the process of review, but is unnecessary once this is committed.  So better to keep it outside the code, and add it as a comment to your review request (if you still think it is needed, which in this case I don't think it is)

@@ +331,4 @@
>    }
>  
> +  // Sync in indirect properties
> +  let syncIndirectPropsPromises = [];

The length of the variable name should be proportional to the size of its scope.  Since this is used in just the few lines of code below, something like `indirectProps` would be enough.

@@ +336,5 @@
> +  /* Later (Bugs 1225916, 1276819, 1276821, etc.) we shall add several more
> +  lines of code here, creating and pushing promises to sync the other
> +  indirect properties (tags, description, etc.). */
> +
> +  return Promise.all(syncIndirectPropsPromises)

No need for comments/code for "intent" to add something later, unless it would affect public API.  Since this is all local code, please remove both the comment and the complication with Promise.all.

@@ +350,5 @@
> +*   second Array
> +* @returns {Array}
> +    which may be empty
> +*/
> +function joinArrays(a, b) {

This seems like overkill for something simple as:

  [].concat(a || [], b || []);

@@ +522,5 @@
> +            .then(node => { nodes.push(node); })
> +            );
> +          });
> +          return Promise.all(makeItemPromises).then(() => nodes);
> +        });

This is the same code as above.

@@ +562,3 @@
>              .catch(error => Promise.reject({message: error.message}));
>          } catch (e) {
> +          return Promise.reject({

Unless you have a very good reason to (for example, you are convinced the existing code is wrong), please don't make unrelated error string changes, here or below.

Existing code might be comparing against current error messages, and this could break that.

@@ +605,5 @@
> +          let promises = [];
> +          let updatedItem;
> +          return PlacesUtils.bookmarks.update(info)
> +          .then(x => {
> +            updatedItem = x;

This should be the argument name, instead of `x`.

@@ +624,5 @@
> +
> +            /* Later (Bugs 1225916, 1276819, 1276821, etc.) we shall add
> +            several more sections here, similar to the above section, which
> +            shall create and pushe promises to sync in the other indirect
> +            properties (tags, description, etc.). */

The code is perfectly valid without the comment, you don't need to explain that you intend to do more later.

::: browser/components/extensions/schemas/bookmarks.json
@@ +67,5 @@
> +                  "type": "string",
> +                  "optional": false,
> +                  "description": "A string representing the keyword"
> +                },
> +                "postData": {

I can't find anywhere in the Firefox UI where this is exposed.  That means it might be available in the database, but if it is not exposed through the UI, I don't think we need to expose it through the API.

@@ +106,5 @@
>          "properties": {
>            "parentId": {
>              "type": "string",
>              "optional": true,
> +            "description": "Defaults to the Unfiled (Unsorted) Bookmarks folder"

Why this change?  The folder is actually called "Other Bookmarks" in the UI.

@@ +130,5 @@
> +                "description": "A string representing the keyword.  Any uppercase characters will be changed to lowercase, leading or trailing spaces will be removed."
> +            },
> +            "description": "A list of keywords, which do not have POST data, to be associated with this bookmark"
> +          },
> +          "keywordObjects": {

Even if wanted to expose the postData, having two properties to add keywords seems inelegant.  But I don't think we wanna do this at all, so let's get rid of it.

@@ +414,5 @@
>                "url": {
>                  "type": "string",
>                  "optional": true
> +              },
> +			  "keywords": {

Please don't use tabs, only spaces for indentation.

Comment 23

[Jerry Krinock]

Thank you for the very thoughtful review, Tomislav.  After following your recommendations, I have reduced the size of my patch file from 27.4 KB to 16.8 KB, and it still works.  I think all of your recommendations have been implemented, but I want to review this patch further before submitting to MozReview.

Comment 24

[Jerry Krinock]

Update: While reviewing my revised patch, I discovered that I'd inadvertently fixed two other tiny bugs, so I reported Bug 1341541 and Bug 1343473.  After the latter lands, I shall generate MozReview, now including unit tests.

Comment 25

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

As I noted in bug 1225916, anything that binds keywords to bookmarks is likely to break.
We are unbinding keywords from bookmarks, the current UI wasn't updated yet, but the plan is basically bug 648398, where keywords will become custom search engines.
I see a risk here where you are trying to standardize something that is already being dropped/changed.

Comment 26

[Tim Nguyen :ntim]

(In reply to Marco Bonardo [::mak] from comment #25)
> As I noted in bug 1225916, anything that binds keywords to bookmarks is
> likely to break.
> We are unbinding keywords from bookmarks, the current UI wasn't updated yet,
> but the plan is basically bug 648398, where keywords will become custom
> search engines.
> I see a risk here where you are trying to standardize something that is
> already being dropped/changed.


There's bug 1268401 about search engine management as I mentioned in the other bug.

Comment 27

[Jerry Krinock]

Hello, Marco.  Thank you for your attention to this bug.

Given that "Keywords as Bookmarks Shortcuts" is being deprecated (or was never intended), could you simply resolve this bug as a duplicate of Bug 1268401?

Comment 30

[jtagcat]

Pardon for the necro.

'Duplicate of this bug' ends up in recursion. The previous one finally, after many jumps, leads to an unrelated bug.

This bug is no resolved. I can't get, modify nor set a bookmark's tags (Bug 1225916) and keywords in the library. Please reopen.