Closed
Bug 1064365
Opened 7 years ago
Closed 6 years ago
Deprecate GetKeywordForURI
Categories
(Toolkit :: Places, defect)
Toolkit
Places
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)
4.25 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Whiteboard: [good second bug] → [good second bug][lang=js]
Assignee | ||
Comment 1•7 years ago
|
||
What will be the alternative method that can be used?
Reporter | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
>convert all internal consumers to alternative code
I meant to ask about this alternative code.
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mak77)
Reporter | ||
Comment 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8511031 -
Flags: review?(mak77)
Reporter | ||
Comment 7•6 years ago
|
||
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-
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #8511031 -
Attachment is obsolete: true
Attachment #8512674 -
Flags: feedback?(mak77)
Reporter | ||
Comment 9•6 years ago
|
||
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-
Assignee | ||
Comment 10•6 years ago
|
||
Can you assign that to me please :)
Attachment #8512674 -
Attachment is obsolete: true
Attachment #8514940 -
Flags: review?(mak77)
Reporter | ||
Comment 11•6 years ago
|
||
oh yes, sorry.
Assignee: nobody → akshendra521994
Status: NEW → ASSIGNED
Reporter | ||
Comment 12•6 years ago
|
||
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+
Assignee | ||
Comment 13•6 years ago
|
||
mak: I do not have try server access.
Attachment #8514940 -
Attachment is obsolete: true
Attachment #8514977 -
Flags: review?(mak77)
Reporter | ||
Comment 14•6 years ago
|
||
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+
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(mak77)
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(mak77)
Keywords: checkin-needed
Comment 15•6 years ago
|
||
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]
Comment 16•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6a7f104d30c8
Status: ASSIGNED → RESOLVED
Closed: 6 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.
Description
•