1064365 - Deprecate GetKeywordForURI

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Before removing the API we should convert all internal consumers to alternative code, and mark the API as deprecated (using  PLACES_WARN_DEPRECATED()).

http://mxr.mozilla.org/mozilla-central/search?string=GetKeywordForURI

Checking add-ons, the only one with a meaningful population is newsfox (18000 users), the other 3 using this API are well below 1000 users. Newsfox is using it in a strange way, can't tell if it's using to migrate from an old version or for something Sync related... Definitely not for its scope.

This requires most js, minimal cpp knowledge.

Comment 1

[Akshendra Pratap Singh]

What will be the alternative method that can be used?

Comment 2

[Marco Bonardo [:mak]]

no alternative. We didn't find a good use for this API, in most cases you want to get the keyword associated to a bookmark, or the bookmark associated to a keyword. Keywords are not a uri feature.

Comment 3

[Akshendra Pratap Singh]

>convert all internal consumers to alternative code
I meant to ask about this alternative code.

Comment 4

[Akshendra Pratap Singh]

MXR shows that most of the internal consumers are tests. What should I do with those? As they seems to be particularly aimed at testing the function GetKeyWordForUri and its counterpart GetUriForKeyword.

Comment 5

[Marco Bonardo [:mak]]

the one in test_bookmarks.js can be removed, as well as the one in browser_bug248970.js

check_uri_keyword in /test_keywords.js should first getBookmarksForURI and then getKeywordForBookmark

in the end that should achieve the same result.

Comment 6

[Akshendra Pratap Singh]

Attachment: 1064365.patch

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8511031 [details] [diff] [review]
1064365.patch

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

Thanks for the patch, there's something I don't understand though:

::: toolkit/components/places/tests/bookmarks/test_keywords.js
@@ +11,5 @@
>  function check_uri_keyword(aURI, aKeyword)
>  {
>    let keyword = aKeyword ? aKeyword.toLowerCase() : null;
> +  let itemIdArray = PlacesUtils.getBookmarksForURI(aURI);
> +  let itemId = -1; 

please remove trailing space

@@ +14,5 @@
> +  let itemIdArray = PlacesUtils.getBookmarksForURI(aURI);
> +  let itemId = -1; 
> +  for(let i=0; i < itemIdArray.length; ++i) {
> +    if(PlacesUtils.bookmarks.getBookmarkURI(itemIdArray[i]).spec === aURI.spec) {
> +      itemId = i;

if you run getBookmarkURI on a result returned by getBookmarksForURI, you'll surely get back the original uri...  what's the purpose of this check then?

I think you should rather go through each of the found bookmarks, getKeywordForBookmark(ItemId), and check if any of the found keywords equals the expected one. If not, either keyword is null or the test should fail (you can return early from the loop, so that if you exit the loop, either keyword is null, or the test failed). I'd suggest using for...of.

Comment 8

[Akshendra Pratap Singh]

Attachment: 1064365.patch

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8512674 [details] [diff] [review]
1064365.patch

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

this is better, but not correct yet.

::: toolkit/components/places/tests/bookmarks/test_keywords.js
@@ +10,5 @@
>  
>  function check_uri_keyword(aURI, aKeyword)
>  {
>    let keyword = aKeyword ? aKeyword.toLowerCase() : null;
> +  for(let bm of PlacesUtils.getBookmarksForURI(aURI)) {

add whitespace after "for"

@@ +12,5 @@
>  {
>    let keyword = aKeyword ? aKeyword.toLowerCase() : null;
> +  for(let bm of PlacesUtils.getBookmarksForURI(aURI)) {
> +    let kid = PlacesUtils.bookmarks.getKeywordForBookmark(bm);
> +    if(kid === keyword) {

add whitespace after "if"

unfortunately if keyword is null, and this bookmark doesn't have a keyword, this condition will be true (null === null) and we'd pass the test. Instead we must ensure none of the found bookmarks has a keyword.

the right check would be something like:

if (!keyword && kid) {
  Assert.ok(false, `${aURI.spec} should not have a keyword`);
} else if (keyword && kid === keyword) {
  Assert.equal(keyword, kid, "Found the expected keyword");
  break;
}

@@ +14,5 @@
> +  for(let bm of PlacesUtils.getBookmarksForURI(aURI)) {
> +    let kid = PlacesUtils.bookmarks.getKeywordForBookmark(bm);
> +    if(kid === keyword) {
> +      Assert.equal(keyword, kid,  "found the keyword");
> +      return;

this function is doing more later, you should not early return, break will do.

@@ +15,5 @@
> +    let kid = PlacesUtils.bookmarks.getKeywordForBookmark(bm);
> +    if(kid === keyword) {
> +      Assert.equal(keyword, kid,  "found the keyword");
> +      return;
> +  	}

please ensure proper indentation

@@ +22,5 @@
> +  if(!keyword) {
> +    Assert.equal(keyword, null, "Keyword was null");
> +  } else {
> +    Assert.ok(false, "Failed test");
> +  }

this should not be needed since we do this test in the for loop

Comment 10

[Akshendra Pratap Singh]

Attachment: 1064365_deprecate_getKeywordForUri.patch

Can you assign that to me please :)

Comment 11

[Marco Bonardo [:mak]]

oh yes, sorry.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8514940 [details] [diff] [review]
1064365_deprecate_getKeywordForUri.patch

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

thanks.

once the final patch is ready, you should push it to the Try server. if you don't have commit access to the try server, just needinfo or feedback me and I'll take care of that.

r=me with the following changes:

::: toolkit/components/places/tests/bookmarks/test_keywords.js
@@ +10,5 @@
>  
>  function check_uri_keyword(aURI, aKeyword)
>  {
>    let keyword = aKeyword ? aKeyword.toLowerCase() : null;
> +  

please remove trailing spaces

@@ +14,5 @@
> +  
> +  for (let bm of PlacesUtils.getBookmarksForURI(aURI)) {
> +    let kid = PlacesUtils.bookmarks.getKeywordForBookmark(bm);
> +    if (kid && !keyword) {
> +      Assert.ok(false, "${aURI.spec} should not have a keyword");

you must use backticks here, not apices.
see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_strings

Comment 13

[Akshendra Pratap Singh]

Attachment: 1064365_deprecate_getKeywordForUri.patch

mak: I do not have try server access.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8514977 [details] [diff] [review]
1064365_deprecate_getKeywordForUri.patch

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5c2146305ee6

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a7f104d30c8