Closed
Bug 1432435
Opened 7 years ago
Closed 7 years ago
Remove synchronous Bookmarks::getBookmarkURI
Categories
(Toolkit :: Places, enhancement, P1)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: mak, Assigned: standard8)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxsearch])
Attachments
(1 file)
Some tests need to be refactored
https://searchfox.org/mozilla-central/search?q=getBookmarkURI(&case=false®exp=false&path=
Reporter | ||
Comment 1•7 years ago
|
||
For now we'll keep the underlying code because nsNavHistoryResult.cpp uses it, but we can remove the js API.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Priority: P2 → P1
Reporter | ||
Updated•7 years ago
|
Whiteboard: [fx-search] → [fxsearch]
Reporter | ||
Updated•7 years ago
|
Summary: Refactor tests using getBookmarkURI → Remove synchronous Bookmarks::getBookmarkURI
Reporter | ||
Comment 2•7 years ago
|
||
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.
Reporter | ||
Updated•7 years ago
|
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•7 years ago
|
||
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 hidden (mozreview-request) |
Reporter | ||
Comment 5•7 years ago
|
||
mozreview-review |
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?
Attachment #8961852 -
Flags: review?(mak77) → review+
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31a88710f63d
Remove synchronous Bookmarks::getBookmarkURI. r=mak
Comment 9•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•