1388250 - Remove old sync isBookmarked and getBookmarkedURIFor APIs from nsINavBookmarksService

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement)

Description

[Mark Banner (:standard8)]

isBookmarked and getBookmarkedURIFor are only used in tests, and have been that way for a few releases.

As part of the work to move to the async APIs, and start removing the old ones (bug 1083465), we can drop these now.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1388250 - Remove old sync isBookmarked and getBookmarkedURIFor APIs from nsINavBookmarksService.

Also remove IsBookmarkedInDatabase(), mItemCount, RecursiveFindRedirectedBookmark(), UpdateKeywordsForRemovedBookmark() from nsNavBookmarks as they aren't used anywhere.

Review commit: https://reviewboard.mozilla.org/r/165942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165942/

Comment 2

[Mark Banner (:standard8)]

Paolo: if you're not comfortable reviewing this, feel free to forward to Drew.

Comment 3

[:Paolo Amadini]

Comment on attachment 8894773 [details]
Bug 1388250 - Remove old sync isBookmarked and getBookmarkedURIFor APIs from nsINavBookmarksService.

https://reviewboard.mozilla.org/r/165942/#review171122

::: toolkit/components/places/tests/bookmarks/test_changeBookmarkURI.js:34
(Diff revision 1)
>    // Ensure that the URI corresponding to aBookmarkId equals aBookmarkedUri
>    do_check_true(bmsvc.getBookmarkURI(aBookmarkId).equals(aBookmarkedUri));
>  
>    // Ensure that aUnbookmarkedUri does not equal any URI that is bookmarked
> -  uri = bmsvc.getBookmarkedURIFor(aUnbookmarkedUri);
> -  do_check_eq(uri, null);
> +  bm = await PlacesUtils.bookmarks.fetch({url: aUnbookmarkedUri});
> +  do_check_eq(bm, null);

Sounds like you should choose between do_check_true/false(...) or do_check_eq/neq(..., null) throughout the tests. Since "fetch" returns null or the bookmark, probably the latter is better as it self-documents the function when reading the test.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8894773 [details]
Bug 1388250 - Remove old sync isBookmarked and getBookmarkedURIFor APIs from nsINavBookmarksService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165942/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f09351b3672
Remove old sync isBookmarked and getBookmarkedURIFor APIs from nsINavBookmarksService. r=Paolo

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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