1329926 - Remove getURIForKeyword API

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

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

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.

Comment 1

[Mayank [:mayank]]

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

Comment 2

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

(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".

Comment 3

[Mayank [:mayank]]

(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?

Comment 4

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

(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

Comment 5

[Mayank [:mayank]]

Attachment: bug-1329926.patch

Attaching a patch which removes the deprecated API getURIForKeyword and uses PlacesUtils.keywords.fetch

Comment 6

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

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

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf69de0301596583ea8a8b3956ea6b97b04bcd4

Comment 8

[Mayank [:mayank]]

Attachment: bug-1329926_fixed-spaces.patch

Thanks for the feedback Mak. Created another patch after removing trailing spaces.
Please review.
Thanks

Comment 9

[Mayank [:mayank]]

Also could you please assign this bug to me?
Thanks

Comment 10

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

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?

Comment 11

[Mayank [:mayank]]

Attachment: bug-1329926.patch

Created patch for removing deprecated getURIForKeyword and updating tests for PlacesUtils.keywords.fetch()

Comment 12

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

note for checkin-needed: the failures in the previous try run (trailing spaces) have been solved.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7feb3cffb4a2
Remove deprecated API getURIForKeyword and updated test cases for PlacesUtils.keywords.fetch(). r=mak

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7feb3cffb4a2