Remove getURIForKeyword API

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: mayank, Mentored)

Tracking

(Blocks 1 bug, {addon-compat, good-first-bug})

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

This legacy API has been replaced all around with PlacesUtils.keywords.fetch and can now be removed.

http://searchfox.org/mozilla-central/search?q=getURIForKeyword

There are 3 tests that should still be updated to the new async API and then the API itself can be removed from the idl and cpp file.
(Assignee)

Comment 1

2 years ago
Hi Marco, I am working on this bug. In tests/bookmarks/, test have "PlacesUtils.bookmarks.getURIForKeyword". Wondering if only "getURIForKeyword" will be replaced by "keywords.fetch()", or it should be completely replaced with "PlacesUtils.keywords.fetch" (not needing "bookmarks" property now?).

Could you please guide on this?

Thanks
(In reply to mayanksri18 from comment #1)
> Hi Marco, I am working on this bug. In tests/bookmarks/, test have
> "PlacesUtils.bookmarks.getURIForKeyword". Wondering if only
> "getURIForKeyword" will be replaced by "keywords.fetch()", or it should be
> completely replaced with "PlacesUtils.keywords.fetch" (not needing
> "bookmarks" property now?).

it should be replaced with yield PlacesUtils.keywords.fetch(... with the appropriate arguments (it's documented in PlacesUtils.jsm, just search for the "keywords" property).
The yield it's because fetch is an asynchronous API that returns a promise, while the old API was synchronous, we need to retain the same behavior for the test to work correctly.
That means some of the tests may need changes to make them "async", that usually involves yielding on a promise, using Task.spawn, and generator functions.
If it's inside an add_task call, it's already in a Task and thus yield is enough to interrupt the task and wait for the promise to be resolved.
To execute the test after your modifications you can "./mach build faster" and then "./mach test path_to_test".
(Assignee)

Comment 3

2 years ago
(In reply to Marco Bonardo [::mak] from comment #2)
> (In reply to mayanksri18 from comment #1)
> > Hi Marco, I am working on this bug. In tests/bookmarks/, test have
> > "PlacesUtils.bookmarks.getURIForKeyword". Wondering if only
> > "getURIForKeyword" will be replaced by "keywords.fetch()", or it should be
> > completely replaced with "PlacesUtils.keywords.fetch" (not needing
> > "bookmarks" property now?).
> 
> it should be replaced with yield PlacesUtils.keywords.fetch(... with the
> appropriate arguments (it's documented in PlacesUtils.jsm, just search for
> the "keywords" property).
> The yield it's because fetch is an asynchronous API that returns a promise,
> while the old API was synchronous, we need to retain the same behavior for
> the test to work correctly.
> That means some of the tests may need changes to make them "async", that
> usually involves yielding on a promise, using Task.spawn, and generator
> functions.
> If it's inside an add_task call, it's already in a Task and thus yield is
> enough to interrupt the task and wait for the promise to be resolved.
> To execute the test after your modifications you can "./mach build faster"
> and then "./mach test path_to_test".

Hi Marco, i have made the changes in code and 2 out of 3 test files are passing. The one which is failing is /toolkit/components/places/tests/bookmarks/test_keywords.js in the function "test_invalid_input".
I modified the  Assert.throws(() => PlacesUtils.bookmarks.getURIForKeyword(null), /NS_ERROR_ILLEGAL_VALUE/); to

Assert.throws(() => PlacesUtils.keywords.fetch(null), /NS_ERROR_ILLEGAL_VALUE/); after making "test_invalid_input" a generator function. 

The error it is displaying is:
ERROR Unexpected exception Error: Invalid keyword at resource://gre/modules/PlacesUtils.jsm:2149
fetch@resource://gre/modules/PlacesUtils.jsm:2149:13

It should pass the test for "null" in PlacesUtils.jsm, I guess.
Could you please help here?
(In reply to mayanksri18 from comment #3)
> Hi Marco, i have made the changes in code and 2 out of 3 test files are
> passing. The one which is failing is
> /toolkit/components/places/tests/bookmarks/test_keywords.js in the function
> "test_invalid_input".
> I modified the  Assert.throws(() =>
> PlacesUtils.bookmarks.getURIForKeyword(null), /NS_ERROR_ILLEGAL_VALUE/); to
> 
> Assert.throws(() => PlacesUtils.keywords.fetch(null),
> /NS_ERROR_ILLEGAL_VALUE/); after making "test_invalid_input" a generator
> function. 

The APIs input/output is different.
So the output in this case is not an nsError value, but a classic javascript Error.
That said, you can remove this test since we already have it here:
http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/toolkit/components/places/tests/unit/test_keywords.js#83
(Assignee)

Comment 5

2 years ago
Posted patch bug-1329926.patch (obsolete) — Splinter Review
Attaching a patch which removes the deprecated API getURIForKeyword and uses PlacesUtils.keywords.fetch
Attachment #8826703 - Flags: review?(mak77)
Comment on attachment 8826703 [details] [diff] [review]
bug-1329926.patch

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

Mostly leftover trailing spaces.
Will push to Try for you.

Please also add ". r=mak" to the commit message.

::: toolkit/components/places/nsINavBookmarksService.idl
@@ +668,4 @@
>     *
>     * @deprecated Use PlacesUtils.keywords.fetch() API instead.
>     */
> +  AString getKeywordForBookmark(in long long aItemId); 

there's a trailing space here.

::: toolkit/components/places/tests/bookmarks/test_keywords.js
@@ +15,4 @@
>      }
>    }
>  
> +  if (aKeyword) {    

trailing spaces

@@ +21,3 @@
>      // Check case insensitivity.
> +    uri = yield PlacesUtils.keywords.fetch(aKeyword.toUpperCase());
> +    Assert.equal(uri.url, aURI.spec);    

trailing spaces

@@ +65,4 @@
>    return observer;
>  }
>  
> +add_task(function test_invalid_input() {  

trailing spaces

::: toolkit/components/places/tests/unit/test_keywords.js
@@ +519,4 @@
>  
>    yield PlacesUtils.keywords.insert({ keyword: "keyword", url: "http://example.com" });
>    Assert.equal(PlacesUtils.bookmarks.getKeywordForBookmark(itemId), "keyword");
> + 

trailing space
Attachment #8826703 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 8

2 years ago
Thanks for the feedback Mak. Created another patch after removing trailing spaces.
Please review.
Thanks
Attachment #8826847 - Flags: review?(mak77)
(Assignee)

Comment 9

2 years ago
Also could you please assign this bug to me?
Thanks
Comment on attachment 8826847 [details] [diff] [review]
bug-1329926_fixed-spaces.patch

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

Could you please merge the patches?
Attachment #8826847 - Flags: review?(mak77)
Assignee: nobody → mayanksri18
Status: NEW → ASSIGNED
(Assignee)

Comment 11

2 years ago
Created patch for removing deprecated getURIForKeyword and updating tests for PlacesUtils.keywords.fetch()
Attachment #8827186 - Flags: review?(mak77)
Attachment #8827186 - Flags: review?(mak77) → review+
Attachment #8826703 - Attachment is obsolete: true
Attachment #8826847 - Attachment is obsolete: true
note for checkin-needed: the failures in the previous try run (trailing spaces) have been solved.
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7feb3cffb4a2
Remove deprecated API getURIForKeyword and updated test cases for PlacesUtils.keywords.fetch(). r=mak
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7feb3cffb4a2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.