Closed Bug 1149488 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
mozilla40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 1 obsolete file)

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)
Flags: qe-verify-
Flags: in-testsuite?
Flags: firefox-backlog+
Blocks: 1140395
Blocks: 1148467
Iteration: --- → 40.1 - 13 Apr
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8586997 - Flags: review?(ttaubert)
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.
Attachment #8586997 - Flags: review?(ttaubert) → review+
(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.
Attached patch patch v1.1Splinter Review
Attachment #8586997 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/e2be272be1bc
Blocks: 1148466
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla40
https://hg.mozilla.org/mozilla-central/rev/e2be272be1bc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.