1432435 - Remove synchronous Bookmarks::getBookmarkURI

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Marco Bonardo [:mak]]

Some tests need to be refactored
https://searchfox.org/mozilla-central/search?q=getBookmarkURI(&case=false&regexp=false&path=

Comment 1

[Marco Bonardo [:mak]]

For now we'll keep the underlying code because nsNavHistoryResult.cpp uses it, but we can remove the js API.

Comment 2

[Marco Bonardo [:mak]]

To do a complete removal, we'd have to add uri to onItemChanged and onItemMoved.
For onItemChanged it's only needed for the "tags" case, we could temporary abuse aOldValue to pass the href, like we did for keywords.

Comment 3

[Mark Banner (:standard8)]

Note: I ended up rewriting browser_library_views_liveupdate.js as it didn't seem completely reliable (assuming we get notifications at the right time), and was slightly confusing as to what some of the expectations were. Hopefully the newer version is now clearer, and I also added some better and more consistent checks in.

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1432435 - Remove synchronous Bookmarks::getBookmarkURI.

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

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8961852 [details]
Bug 1432435 - Remove synchronous Bookmarks::getBookmarkURI.

https://reviewboard.mozilla.org/r/230672/#review236580

Please file a bug to actually modify the notifications and completely remove GetBookmarkURI... Probably we should have a new meta to completely remove nsNavBookmarks.cpp, merging the remaining pieces into nsNavHistory and Bookmarks.jsm

::: browser/components/places/tests/browser/browser_library_views_liveupdate.js:17
(Diff revision 1)
> +  gLibrary = await promiseLibrary();
> +
> +  await PlacesUtils.bookmarks.eraseEverything();
> +
> +  registerCleanupFunction(async () => {
> +    await promiseLibraryClosed(gLibrary);

shouldn't we eraseEverything on cleanup as well?

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8961852 [details]
Bug 1432435 - Remove synchronous Bookmarks::getBookmarkURI.

https://reviewboard.mozilla.org/r/230672/#review236580

Filed bug 1448913 and bug 1448916

> shouldn't we eraseEverything on cleanup as well?

Well the test should be removing everything it has created, but just in case...

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8961852 [details]
Bug 1432435 - Remove synchronous Bookmarks::getBookmarkURI.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31a88710f63d
Remove synchronous Bookmarks::getBookmarkURI. r=mak

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/31a88710f63d