1125115 - Write a new keywords pseudo-API in PlacesUtils

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

We need a new api for keywords in PlacesUtils.keywords, it needs to be able to set and retrieve them through a cache, and also allow to read the whole cache at once.

Comment 1

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

Attachment: patch v1

First reviewable version.

Note, I still have to do a proof-reading of this, I spent most time figuring out test failures so now it passes all the tests.

Comment 2

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

I must recall to remove __noSuchMethod__ since it's now deprecated.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8573510 [details] [diff] [review]
patch v1

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +100,5 @@
> + *        the url to notify about.
> + * @param keyword
> + *        The keyword to notify, or empty string if a keyword was removed.
> + */
> +function* notifyKeywordChange(url, keyword) {

Nit: indentation for the whole function is a little off.

@@ +297,5 @@
>          }
> +
> +        // Start observing bookmarks changes.  This is needed as far as we
> +        // support both old and new APIs at the same time.
> +        gKeywordsCachePromise.catch(Cu.reportError);

Took me a while to understand it just initializes the keywords cache, can you maybe mention that in the comment? :)

@@ +888,5 @@
>        stmt.finalize();
>      }
> +
> +    // Update the cache.
> +    return Task.spawn(function* () {

The function didn't return anything before, now it returns a promise. It's going away sooner or later but maybe we should return the task?

@@ +895,5 @@
> +      let cache = yield gKeywordsCachePromise;
> +      let entries = yield PlacesUtils.keywords.fetchByHref(bm.url.href);
> +      // Set the POST data on keywords not having it.
> +      for (let entry of entries) {
> +        if (!entry.postData) {

Are there other callers of fetchByHref()? From the one single caller here it seems that we could either just filter entries ourselves by checking href and postData, or modify the function to allow fetching keywords byHrefAndPostData?

@@ +1990,5 @@
> +    }
> +    catch(ex) {
> +      // It's too late to block shutdown, just close the connection.
> +      return conn.close();
> +      throw (ex);

We never reach the throw statement, do we?

@@ +1992,5 @@
> +      // It's too late to block shutdown, just close the connection.
> +      return conn.close();
> +      throw (ex);
> +    }
> +    return Promise.resolve();

Why exactly do we return a fresh Promise?

@@ +1993,5 @@
> +      return conn.close();
> +      throw (ex);
> +    }
> +    return Promise.resolve();
> +  }).then(null, Cu.reportError);

.catch(Cu.reportError) ?

@@ +1994,5 @@
> +      throw (ex);
> +    }
> +    return Promise.resolve();
> +  }).then(null, Cu.reportError);
> +  return connPromised;

Couldn't we just do |return connPromised.then(conn => {...|?

@@ +2014,5 @@
> +   * @return {Promise}
> +   * @resolves to an object in the form: { href, postData }, or { null, null }
> +   *           if a keyword was not found.
> +   */
> +  fetch(keyword) {

Would it make sense to be consistent with the Bookmarks API and allow to pass an object to fetch that's either {keyword: "foobar"} or {href: "http://example.com"}, instead of having the separate fetchByHref() method (that we can still use internally of course)?

@@ +2021,5 @@
> +    keyword = keyword.trim().toLowerCase();
> +    return Task.spawn(function* () {
> +      let cache = yield gKeywordsCachePromise;
> +      return cache.get(keyword) || { href: null, postData: null };
> +    }.bind(this));

Looks like we could just do without Task.spawn():

return gKeywordsCachePromise.then(cache => {
  return cache.get(keyword) || { href: null, postData: null };
});

@@ +2031,5 @@
> +   * @param aHref
> +   *        The href to fetch.
> +   * @return {Promise}
> +   * @resolves to an array containing keyword entries for the given href, in the
> +   *           form { keyword, postData }

So other than fetch() this returns {keyword, postData} instead of {href, postData}. Although either keyword or href is known when calling one of those functions, should we combine those to have a consistent format for "keyword objects"?

@@ +2035,5 @@
> +   *           form { keyword, postData }
> +   */
> +  fetchByHref(href) {
> +    href = new URL(href).href;
> +    return Task.spawn(function* () {

Maybe define fetchByHref() using Task.async()? Or just use the cache promise directly like suggested above?

@@ +2058,5 @@
> +   *        The optional postData for this keyword.
> +   * @return {Promise}
> +   * @resolves when the addition is complete.
> +   */
> +  insert(keyword, href, postData = null) {

Same about consistency, should this accept an object {keyword, href, postData?}? Can we re-use some of the argument checking functionality from Bookmarks.jsm?

@@ +2076,5 @@
> +      let db = yield PlacesUtils.promiseWrappedConnection();
> +      if (cache.has(keyword)) {
> +        yield db.executeCached(
> +          `UPDATE moz_keywords
> +           SET place_id = (SELECT id FROM moz_places WHERE url = :url),

Should this be a no-op in case the keyword is already associated with the given URL? Might not be worth it.

@@ +2080,5 @@
> +           SET place_id = (SELECT id FROM moz_places WHERE url = :url),
> +               post_data = :post_data
> +           WHERE keyword = :keyword
> +          `, { url: href, keyword: keyword, post_data: postData });
> +        let { href: oldHref, postData: oldPostData } = cache.get(keyword);

Nit: oldPostData seems unused.

@@ +2119,5 @@
> +    return Task.spawn(function* () {
> +      let cache = yield gKeywordsCachePromise;
> +      if (!cache.has(keyword))
> +        return;
> +      let { href, postData } = cache.get(keyword);

Nit: looks like |postData| is unused here.

@@ +2134,5 @@
> +};
> +
> +// Set by the keywords API to distinguish notifications fired by the old API.
> +// Once the old API will be gone, we can remove this and stop observing.
> +let gIgnoreKeywordNotifications = false;

Not pretty but yeah, guess we need this for now. The only thing that bugs me is that we would ignore keyword modifications that are done in response to a keywords notification and then we might miss those. Naive question, is there any way to differentiate between notifications from the new API and those from the old API?

Should gIgnoreKeywordNotifications maybe be a set with specific notifications to ignore other than just ignoring everything to not fail in the case described above?

@@ +2166,5 @@
> +  // relation for backwards compatibility reasons, but mostly because we are
> +  // lacking a UI to manage keywords directly.
> +  let observer = {
> +    QueryInterface: XPCOMUtils.generateQI(Ci.nsINavBookmarkObserver),
> +    __noSuchMethod__: () => {}, // Catch all all onItem* methods.

(Just a reminder to remove this.)

@@ +2181,5 @@
> +
> +      Task.spawn(function* () {
> +        // If the uri is not bookmarked anymore, we can remove this keyword.
> +        let bookmarks = [];
> +        yield PlacesUtils.bookmarks.fetch({ url: uri }, b => bookmarks.push(b));

Do we really need to collect all bookmarks for that url? Wouldn't just fetching a single be enough to tell us that we don't need to remove keywords?

@@ +2184,5 @@
> +        let bookmarks = [];
> +        yield PlacesUtils.bookmarks.fetch({ url: uri }, b => bookmarks.push(b));
> +        if (bookmarks.length == 0) {
> +          for (let keyword of keywords) {
> +            yield PlacesUtils.keywords.remove(keyword);

Don't we need to update the cache as well and remove all kws from it? We could modify keywordsForHref() to be removeKeywordsForHref() and do that for us and reuse it below?

::: toolkit/components/places/tests/unit/test_keywords.js
@@ +1,1 @@
> +function* check_keyword(aExpect, aHref, aKeyword, aPostData = null) {

Please add "use strict"; above.

@@ +2,5 @@
> +  // Check case-insensitivity.
> +  aKeyword = aKeyword.toUpperCase();
> +
> +  let { href, postData } = yield PlacesUtils.keywords.fetch(aKeyword);
> +  if (!aExpect) {

Should probably give it a better name - what do we not expect? Maybe aExpectExists or something?

@@ +12,5 @@
> +                                    entry.postData == aPostData), "Keyword found");
> +  }
> +}
> +
> +function check_orphans() {

Maybe "check_no_orphans" or the like?

@@ +13,5 @@
> +  }
> +}
> +
> +function check_orphans() {
> + let db = yield PlacesUtils.promiseDBConnection();

Nit: indentation

@@ +33,5 @@
> +      }
> +
> +      if (name.startsWith("onItemChanged")) {
> +        return () => {
> +          if (arguments[1] != "keyword")

Please give the first two arg names for the arrow function and let's not use arguments.

@@ +444,5 @@
> +  PlacesUtils.bookmarks.setKeywordForBookmark(itemId, "keyword");
> +  // The cache update is async, we must poll for the keyword.
> +  let href = null;
> +  do {
> +    yield new Promise(resolve => do_timeout(1000, resolve));

1s seems like a long poll interval, maybe reduce it to 100ms?

@@ +450,5 @@
> +  } while (href != "http://example.com/");
> +  // Remove the keyword.
> +  PlacesUtils.bookmarks.setKeywordForBookmark(itemId, "");
> +  do {
> +    yield new Promise(resolve => do_timeout(1000, resolve));

Same here, maybe we could move the polling do-while loop to a helper function?

@@ +460,5 @@
> +  Assert.equal(PlacesUtils.bookmarks.getURIForKeyword("keyword").spec, "http://example.com/");
> +  yield PlacesUtils.bookmarks.remove(bookmark);
> +
> +  check_orphans();
> +});  

Nit: trailing white space

Comment 4

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

(In reply to Tim Taubert [:ttaubert] from comment #3)
> @@ +888,5 @@
> > +
> > +    // Update the cache.
> > +    return Task.spawn(function* () {
> 
> The function didn't return anything before, now it returns a promise. It's
> going away sooner or later but maybe we should return the task?

Considered it was returning nothing, nobody should rely on a return value, so abusing it temporarily before we remove the API, should be fine.

> @@ +895,5 @@
> 
> Are there other callers of fetchByHref()?

I removed that API.

> Would it make sense to be consistent with the Bookmarks API and allow to
> pass an object to fetch that's either {keyword: "foobar"} or {href:
> "http://example.com"}, instead of having the separate fetchByHref() method
> (that we can still use internally of course)?

now the insert API takes a { keyword, url, postData }
fetch returns a { keyword, url, postData }
url can be URL or href (not nsIURI though cause that would have complicated the code more than needed)

both fetch and remove still just take a keyword string... I evaluated to make them also accept an object but the gain is uncertain, and it's something we could still do in future, if needed.

> @@ +2134,5 @@
> > +// Set by the keywords API to distinguish notifications fired by the old API.
> > +// Once the old API will be gone, we can remove this and stop observing.
> > +let gIgnoreKeywordNotifications = false;
> 
> Not pretty but yeah, guess we need this for now. The only thing that bugs me
> is that we would ignore keyword modifications that are done in response to a
> keywords notification and then we might miss those. Naive question, is there
> any way to differentiate between notifications from the new API and those
> from the old API?
> 
> Should gIgnoreKeywordNotifications maybe be a set with specific
> notifications to ignore other than just ignoring everything to not fail in
> the case described above?

I don't think I understood the question, but notifications are synchronous, so the risk of missing notifications should be very low or non-existent. Especially cause we ignore only onItemChanged for "keyword", it's unlikely this notification must do any complicate task that could fire more notifications.

> @@ +2184,5 @@
> > +        let bookmarks = [];
> > +        yield PlacesUtils.bookmarks.fetch({ url: uri }, b => bookmarks.push(b));
> > +        if (bookmarks.length == 0) {
> > +          for (let keyword of keywords) {
> > +            yield PlacesUtils.keywords.remove(keyword);
> 
> Don't we need to update the cache as well and remove all kws from it? We
> could modify keywordsForHref() to be removeKeywordsForHref() and do that for
> us and reuse it below?

This is calling the external API (keywords.remove) that already takes care of updating the cache.

I have fixed all of the other comments.

Comment 5

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

Attachment: patch v2

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8578977 [details] [diff] [review]
patch v2

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

A few comments but nothing critical. LGTM!

::: toolkit/components/places/PlacesUtils.jsm
@@ +1976,5 @@
> +        // It's too late to block shutdown, just close the connection.
> +        conn.close();
> +        throw ex;
> +      }
> +      resolve(conn);

Couldn't we just return |conn| here and then remove the |new Promise| call? Like...

XPCOMUtils.defineLazyGetter(this, "gAsyncDBConnPromised", () => {
  return Sqlite.cloneStorageConnection({
    // ...
  }).then(conn => {
    // ...
    return conn;
  });
});

@@ +1983,5 @@
> +);
> +
> +XPCOMUtils.defineLazyGetter(this, "gAsyncDBWrapperPromised",
> +  () => new Promise((resolve) => {
> +    Sqlite.wrapStorageConnection({

Same here?

@@ +2002,5 @@
> +);
> +
> +/**
> + * Keywords management API.
> + * Soon or later these keywords will merge with search keywords, this is an

nit: Sooner or later

@@ +2015,5 @@
> +   * @param keyword
> +   *        The keyword to fetch.
> +   * @return {Promise}
> +   * @resolves to an object in the form: { keyword, url, postData },
> +   *           or { keyword, null, undefined } if a keyword was not found.

Hmmm, why do we not just resolve to |null|? That seems behavior seems a little unconventional.

@@ +2047,5 @@
> +    if (!("keyword" in keywordEntry) || !keywordEntry.keyword ||
> +        typeof(keywordEntry.keyword) != "string")
> +      throw new Error("Invalid keyword");
> +    if (("postData" in keywordEntry) && (!keywordEntry.postData ||
> +                                        typeof keywordEntry.postData != "string"))

Nit: move one space to the right to be properly aligned?

@@ +2051,5 @@
> +                                        typeof keywordEntry.postData != "string"))
> +      throw new Error("Invalid POST data");
> +    if (!("url" in keywordEntry))
> +      throw new Error("undefined is not a valid URL");
> +    let { keyword, url } = keywordEntry;

(too bad we don't support default values for destructuring, postData=null would be nice to put in there :)

@@ +2053,5 @@
> +    if (!("url" in keywordEntry))
> +      throw new Error("undefined is not a valid URL");
> +    let { keyword, url } = keywordEntry;
> +    keyword = keyword.trim().toLowerCase();
> +    let postData = ("postData" in keywordEntry) ? keywordEntry.postData : null;

let postData = keywordEntry.postData || null;

@@ +2055,5 @@
> +    let { keyword, url } = keywordEntry;
> +    keyword = keyword.trim().toLowerCase();
> +    let postData = ("postData" in keywordEntry) ? keywordEntry.postData : null;
> +    // This also checks href for validity
> +    url = url instanceof URL ? url : new URL(url);

|url = new URL(url)| would actually just work even when |url instanceof URL|.

@@ +2170,5 @@
> +    onBeginUpdateBatch: function() {},
> +    onEndUpdateBatch: function() {},
> +    onItemAdded: function() {},
> +    onItemVisited: function() {},
> +    onItemMoved: function() {},

nit: could use the fancy |onItemMove() {}| notation for all of those.

@@ +3244,5 @@
>  PlacesEditBookmarkPostDataTransaction.prototype = {
>    __proto__: BaseTransaction.prototype,
>  
>    doTransaction: function EBPDTXN_doTransaction()
>    {

Maybe |doTransaction() {| and |undoTransaction() {| while you're here? :)

@@ +3245,5 @@
>    __proto__: BaseTransaction.prototype,
>  
>    doTransaction: function EBPDTXN_doTransaction()
>    {
> +    // Setting null postData is not supported by current schema.

nit: by the current

@@ +3254,5 @@
>    },
>  
>    undoTransaction: function EBPDTXN_undoTransaction()
>    {
> +    // Setting null postData is not supported by current schema.

nit: same

::: toolkit/components/places/tests/unit/test_keywords.js
@@ +26,5 @@
> +    url = (yield PlacesUtils.keywords.fetch(entry.keyword)).url;
> +  } while (url != entry.url && url.href != entry.url.href);
> +}
> +
> +function check_no_orphans() {

nit: function* check_no_orphans() {

@@ +27,5 @@
> +  } while (url != entry.url && url.href != entry.url.href);
> +}
> +
> +function check_no_orphans() {
> + let db = yield PlacesUtils.promiseDBConnection();

nit: indentation

@@ +35,5 @@
> +    `);
> +  Assert.equal(rows.length, 0);
> +}
> +
> +function expectNotifications() {

Can we rename this to expectBookmarkNotifications()? I got confused because I didn't understand for a moment why there would be no notification when inserting a new keyword.

@@ +62,5 @@
> +      }
> +
> +      if (name in target)
> +        return target[name];
> +      return undefined;

nit: just return target[name]? should be undefined when |name| is not in |target|?

@@ +434,5 @@
> +add_task(function* test_deleteKeywordMultipleBookmarks() {
> +  yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/" });
> +  yield check_keyword(true, "http://example.com/", "keyword");
> +  // Update the keyword with POST data.
> +  yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com/", postData: "postData1" });

Should probably not use insert() to update a keyword as discussed on IRC.

Comment 7

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

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Couldn't we just return |conn| here and then remove the |new Promise| call?
> Like...

Yes, but I kept this identical to the getter in history.jsm cause we should merge all of them together in bug 1091851 and I don't want to fork too many versions.

> @@ +2015,5 @@
> > +   * @param keyword
> > +   *        The keyword to fetch.
> > +   * @return {Promise}
> > +   * @resolves to an object in the form: { keyword, url, postData },
> > +   *           or { keyword, null, undefined } if a keyword was not found.
> 
> Hmmm, why do we not just resolve to |null|? That seems behavior seems a
> little unconventional.

we could but then instead of 

let { keyword, url } = yield fetch;
if (keyword)

you should do

let keyword, url
let entry = yield fetch;
if (entry)
  ...

Or use entry.keyword everywhere.

I'm not sure what's the best approach, but maybe I should just be coherent with Bookmarks.jsm and return null.
Also with the latest changes I lost some of the advantages of deconstruction, so I will change it to just null.

> @@ +2053,5 @@
> > +    if (!("url" in keywordEntry))
> > +      throw new Error("undefined is not a valid URL");
> > +    let { keyword, url } = keywordEntry;
> > +    keyword = keyword.trim().toLowerCase();
> > +    let postData = ("postData" in keywordEntry) ? keywordEntry.postData : null;
> 
> let postData = keywordEntry.postData || null;

I don't remember if in strict mode this will warn cause postData doesn't exist...

> @@ +62,5 @@
> > +      }
> > +
> > +      if (name in target)
> > +        return target[name];
> > +      return undefined;
> 
> nit: just return target[name]? should be undefined when |name| is not in
> |target|?

ditto about warning, everytime I forget when it warns and when it doesn't, and maybe it also changed recently.

Btw will check that.

Comment 8

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

yes, this fires the undefined property warning

"use strict"

function test() {
  let o = {}
  return o["nonexistent"];
}
test();

Comment 9

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

Attachment: patch v3

Comment 10

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

There are  a couple failures on Try, I must look at
test_browserGlue_corrupt_nobackup_default.js (not sure what's up)
browser_getshortcutoruri.js (for sure keywords related)

Comment 11

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

Attachment: additional test fixes

I will merge this into the main patch, it fixes some browser-chrome tests using annotations instead of the keywords API.

I'm still ivnestigating the failure in test_browserGlue_corrupt.js, it looks very fancy, it will be in a separate patch.

Comment 12

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

Attachment: more fixes

So, while investigating the failure in browserGlue tests I ended up figuring out we had a bug in PlacesCategoriesStarter that was causing us to try to init the keywords cache multiple times.
I then found the tests were failing cause they were not properly waiting for initialization, just by adding the keywords cache init, some storage operation was delayed and the tests started failing.
Properly waiting before the init notification should also help porting more stuff to the async API.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8580381 [details] [diff] [review]
more fixes

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

::: toolkit/components/places/PlacesCategoriesStarter.js
@@ +50,4 @@
>        // Directly notify PlacesUtils, to ensure it catches the notification.
>        PlacesUtils.observe(null, "bookmarks-service-ready", null);
>      }
> +  }

Nit: missing semicolon

Comment 14

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

Attachment: merged patch

Comment 15

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

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

Comment 16

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

and a follow-up to remove a test checking lastModified changes with keywords, since now we don't use bookmarks, lastModified won't change.

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/296d8da26609
https://hg.mozilla.org/mozilla-central/rev/8aede8703d12