The default bug view has changed. See this FAQ.

Remove getURIForKeyword API

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
Places
P3
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: mak, Assigned: Mayank, Mentored)

Tracking

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

Trunk
mozilla53
addon-compat, good-first-bug
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 months ago
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

3 months 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
(Reporter)

Comment 2

3 months ago
(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

3 months 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?
(Reporter)

Comment 4

3 months ago
(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

3 months ago
Created attachment 8826703 [details] [diff] [review]
bug-1329926.patch

Attaching a patch which removes the deprecated API getURIForKeyword and uses PlacesUtils.keywords.fetch
Attachment #8826703 - Flags: review?(mak77)
(Reporter)

Comment 6

3 months ago
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+
(Reporter)

Comment 7

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf69de0301596583ea8a8b3956ea6b97b04bcd4
(Assignee)

Comment 8

3 months ago
Created attachment 8826847 [details] [diff] [review]
bug-1329926_fixed-spaces.patch

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

Comment 9

3 months ago
Also could you please assign this bug to me?
Thanks
(Reporter)

Comment 10

2 months ago
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)
(Reporter)

Updated

2 months ago
Assignee: nobody → mayanksri18
Status: NEW → ASSIGNED
(Assignee)

Comment 11

2 months ago
Created attachment 8827186 [details] [diff] [review]
bug-1329926.patch

Created patch for removing deprecated getURIForKeyword and updating tests for PlacesUtils.keywords.fetch()
Attachment #8827186 - Flags: review?(mak77)
(Reporter)

Updated

2 months ago
Attachment #8827186 - Flags: review?(mak77) → review+
(Reporter)

Updated

2 months ago
Attachment #8826703 - Attachment is obsolete: true
(Reporter)

Updated

2 months ago
Attachment #8826847 - Attachment is obsolete: true
(Reporter)

Comment 12

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

Comment 13

2 months 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 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7feb3cffb4a2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Updated

2 months ago
Keywords: addon-compat
You need to log in before you can comment on or make changes to this bug.