1149488 - PlacesUtils.keywords.fetch should be able to fetch by url

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

We must modify fetch() to allow it to take a keywordOrEntry argument that can be either a string (the keyword) or an object in the form { keyword: "a string" } or { url: URL or string.
If keywordOrEntry is a string or an object with a keyword property (that is a string), then we execute the current code.
Otherwise, if it's an object with a url property (that is a URL or a string), we must loop all the entries in gKeywordsCachePromise, compare their url with the provided one, and return the found entries.

Since we can have more than one entry for that url, we'll return just one of them, but we'll callback all the found ones to an "onResult=null" optional argument.
This works similarly to Bookmarks.jsm::fetch API (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#478)

Comment 1

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

Attachment: patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +2037,5 @@
> +    if (!("keyword" in keywordOrEntry) && !("url" in keywordOrEntry))
> +      throw new Error("At least keyword or url must be provided");
> +    if (onResult && typeof onResult != "function")
> +      throw new Error("onResult callback must be a valid function");
> +    if ("url" in keywordOrEntry) {

We check this so often, I wonder if local variables would make sense?

let hasUrl = "url" in keywordOrEntry;
let hasKeyword = "keyword" in keywordOrEntry;

@@ +2054,5 @@
> +
> +    return gKeywordsCachePromise.then(cache => {
> +      let entries = [];
> +      if ("keyword" in keywordOrEntry) {
> +        keywordOrEntry.keyword = keywordOrEntry.keyword.trim().toLowerCase();

Can we normalize this up where we normalize the url?

@@ +2060,5 @@
> +        if (entry)
> +          entries.push(entry);
> +      }
> +      if ("url" in keywordOrEntry) {
> +        for (let [ keyword, entry ] of cache) {

for (let entry of cache.entries()) {

@@ +2068,5 @@
> +      }
> +
> +      entries = entries.filter(e => {
> +        return (!("url" in keywordOrEntry) || e.url.href == keywordOrEntry.url.href) &&
> +               (!("keyword" in keywordOrEntry) || e.keyword == keywordOrEntry.keyword);

Does it really make sense to be able to specify a keyword AND url? The comments above the function don't mention that you can pass both but it's certainly possible. I mean, is there any real use-case for that?

If we keep that we should at least add a comment here explaining that here we get the intersection of both the "url" and "keyword" search in case both properties were given.

Comment 3

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

(In reply to Tim Taubert [:ttaubert] (on PTO, back April 14th) from comment #2)
> Does it really make sense to be able to specify a keyword AND url? The
> comments above the function don't mention that you can pass both but it's
> certainly possible. I mean, is there any real use-case for that?

I did that for coherence mostly, since it's likely one might check if a given url has a given keyword. For example the test does that. The fact is that I expect the opposite would be confusing (I give a keyword and a url I know I have, and I get back nothing?)

> If we keep that we should at least add a comment here explaining that here
> we get the intersection of both the "url" and "keyword" search in case both
> properties were given.

Sure I will make the comment clearer.

Comment 4

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

Attachment: patch v1.1

Comment 5

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

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

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e2be272be1bc

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Fixed on Aurora via the roll-up patch in bug 1148466.
https://hg.mozilla.org/releases/mozilla-aurora/rev/c705cd9ab5a2