Closed Bug 1064365 Opened 5 years ago Closed 5 years ago

Deprecate GetKeywordForURI

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mak, Assigned: akshendra521994, Mentored)

References

Details

(Keywords: perf, Whiteboard: [good second bug][lang=js])

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 1064360
No longer depends on: 1064360
Whiteboard: [good second bug] → [good second bug][lang=js]
What will be the alternative method that can be used?
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.
>convert all internal consumers to alternative code
I meant to ask about this alternative code.
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.
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
Attached patch 1064365.patch (obsolete) — Splinter Review
Attachment #8511031 - Flags: review?(mak77)
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.
Attachment #8511031 - Flags: review?(mak77) → review-
Attached patch 1064365.patch (obsolete) — Splinter Review
Attachment #8511031 - Attachment is obsolete: true
Attachment #8512674 - Flags: feedback?(mak77)
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
Attachment #8512674 - Flags: feedback?(mak77) → feedback-
Can you assign that to me please :)
Attachment #8512674 - Attachment is obsolete: true
Attachment #8514940 - Flags: review?(mak77)
oh yes, sorry.
Assignee: nobody → akshendra521994
Status: NEW → ASSIGNED
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
Attachment #8514940 - Flags: review?(mak77) → review+
mak: I do not have try server access.
Attachment #8514940 - Attachment is obsolete: true
Attachment #8514977 - Flags: review?(mak77)
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
Attachment #8514977 - Flags: review?(mak77) → review+
Flags: needinfo?(mak77)
Flags: needinfo?(mak77)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/6a7f104d30c8
Keywords: checkin-needed
Whiteboard: [good second bug][lang=js] → [good second bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6a7f104d30c8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good second bug][lang=js][fixed-in-fx-team] → [good second bug][lang=js]
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.