Replace nsINavBookmarksService::onItemMoved with bookmark-moved
Categories
(Toolkit :: Places, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox90 | --- | fixed |
People
(Reporter: standard8, Assigned: daisuke)
References
Details
(Whiteboard: [fxsearch] )
Attachments
(4 files, 1 obsolete file)
As part of replacing the observer notification system for places we should replace onItemMoved. For now, I think we can keep most of the API the same but drop the parent id parameter. The only place this is used is the tag service, but that can compare with PlacesUtils.bookmarks.tagsGuid instead.
Reporter | ||
Updated•5 years ago
|
Updated•4 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
Hello Mark!
(In reply to Mark Banner (:standard8) from comment #0)
For now, I think we can keep most of the API the same but drop the parent id
parameter. The only place this is used is the tag service, but that can
compare with PlacesUtils.bookmarks.tagsGuid instead.
It has already been two years since you mentioned this..
I think the code is around below, but I could not find STR that passes the condition.
https://searchfox.org/mozilla-central/rev/014fe72eaba26dcf6082fb9bbaf208f97a38594e/toolkit/components/places/TaggingService.jsm#446-452
Just in case, I threw a patch that fires an error if the condition passed to try-server, it didn’t seem that the error happens..
https://treeherder.mozilla.org/jobs?repo=try&revision=b656c59e57be5a1d9a00729a129f7fc4170c2353&selectedTaskRun=EnF39qeRT5qXAH1OQJqNhw.0
So, I want to ask one question. Do you know how we can pass the condition? Or, the code is no longer used already?
Thanks!
Assignee | ||
Comment 2•3 years ago
|
||
I checked the usage of onItemMoved
, it seems that we use all of data defined in nsINavBookmarksService
.
https://searchfox.org/mozilla-central/rev/014fe72eaba26dcf6082fb9bbaf208f97a38594e/toolkit/components/places/nsINavBookmarksService.idl#129-139
So, we may not be able to drop the parent id (oldParentId, newParentId).
Reporter | ||
Comment 3•3 years ago
|
||
Hi Daisuke.
I vaguely wonder if I was looking at onItemChanged when I wrote that, but it doesn't quite apply the same.
If you look at code coverage (available in searchfox in the navigation box under other tools), you can see that those lines you commented out aren't covered by tests. Unfortunately there are gaps in our coverage.
What I think I was trying to say is that you could use the guids instead of the ids, e.g.
if (
this._tagFolders[aItemId] &&
PlacesUtils.bookmarks.tagsGuid == aOldParentGuid &&
PlacesUtils.bookmarks.tagsGuid != aNewParentGuid
) {
delete this._tagFolders[aItemId];
}
There are some references in the C++ to the id as well: https://searchfox.org/mozilla-central/rev/014fe72eaba26dcf6082fb9bbaf208f97a38594e/toolkit/components/places/nsNavHistoryResult.cpp#3347
However mTargetFolderItemId
could be changed to use the already present mTargetFolderGuid
.
So I'm pretty sure we don't need the aOldParent
and aNewParent
arguments, and we can use the guid forms instead.
Assignee | ||
Comment 4•3 years ago
•
|
||
Thank you very much for your helpful advice. Mark.
I try that!
Assignee | ||
Comment 5•3 years ago
|
||
Assignee | ||
Comment 6•3 years ago
|
||
Depends on D102571
Assignee | ||
Comment 7•3 years ago
|
||
Depends on D102572
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D102573
Assignee | ||
Comment 9•3 years ago
|
||
Depends on D102574
Updated•3 years ago
|
Comment 10•3 years ago
|
||
Pushed by dakatsuka.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/87b0bbbcb0e6 Remove old/new parent id from onItemMoved in nsINavBookmarkObserver. r=mak https://hg.mozilla.org/integration/autoland/rev/2cf6fca33bf3 Implement a mechanism to fire bookmark-moved event. r=mak https://hg.mozilla.org/integration/autoland/rev/dc8c9488cf1a Apply bookmark-moved event. r=mak https://hg.mozilla.org/integration/autoland/rev/ed9ac8c3097b Remove onItemMoved from nsINavBookmarkObserver. r=mak
Comment 11•3 years ago
|
||
Backed out for causing bustage on nsNavHistoryResult.cpp
-
backout: https://hg.mozilla.org/integration/autoland/rev/973389a03655b338bcb93178344d31ceccc5fa10
-
failure log: https://treeherder.mozilla.org/logviewer?job_id=338894858&repo=autoland&lineNumber=84950
[task 2021-05-06T02:47:50.247Z] 02:47:50 INFO - In file included from Unified_cpp_components_places0.cpp:128:
[task 2021-05-06T02:47:50.248Z] 02:47:50 ERROR - /builds/worker/checkouts/gecko/toolkit/components/places/nsNavHistoryResult.cpp:2507:30: error: function declared 'stdcall' here was previously declared without calling convention
[task 2021-05-06T02:47:50.248Z] 02:47:50 INFO - nsNavHistoryQueryResultNode::OnItemMoved(int64_t aFolder, int32_t aOldIndex,
[task 2021-05-06T02:47:50.248Z] 02:47:50 INFO - ^
[task 2021-05-06T02:47:50.249Z] 02:47:50 INFO - /builds/worker/checkouts/gecko/toolkit/components/places/nsNavHistoryResult.h:691:12: note: previous declaration is here
[task 2021-05-06T02:47:50.249Z] 02:47:50 INFO - nsresult OnItemMoved(int64_t aFolder, int32_t aOldIndex, int32_t aNewIndex,
[task 2021-05-06T02:47:50.249Z] 02:47:50 INFO - ^
[task 2021-05-06T02:47:50.249Z] 02:47:50 INFO - In file included from Unified_cpp_components_places0.cpp:128:
[task 2021-05-06T02:47:50.249Z] 02:47:50 ERROR - /builds/worker/checkouts/gecko/toolkit/components/places/nsNavHistoryResult.cpp:3326:31: error: function declared 'stdcall' here was previously declared without calling convention
[task 2021-05-06T02:47:50.249Z] 02:47:50 INFO - nsNavHistoryFolderResultNode::OnItemMoved(int64_t aItemId, int32_t aOldIndex,
[task 2021-05-06T02:47:50.249Z] 02:47:50 INFO - ^
[task 2021-05-06T02:47:50.249Z] 02:47:50 INFO - /builds/worker/checkouts/gecko/toolkit/components/places/nsNavHistoryResult.h:794:12: note: previous declaration is here
[task 2021-05-06T02:47:50.251Z] 02:47:50 INFO - nsresult OnItemMoved(int64_t aFolder, int32_t aOldIndex, int32_t aNewIndex,
[task 2021-05-06T02:47:50.251Z] 02:47:50 INFO - ^
[task 2021-05-06T02:47:50.251Z] 02:47:50 INFO - 2 errors generated.
Assignee | ||
Comment 12•3 years ago
|
||
Thanks. I will take a look at this issue.
Comment 13•3 years ago
|
||
Pushed by dakatsuka.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fe8bfcd35e2b Remove old/new parent id from onItemMoved in nsINavBookmarkObserver. r=mak https://hg.mozilla.org/integration/autoland/rev/99d7c325bf06 Implement a mechanism to fire bookmark-moved event. r=mak https://hg.mozilla.org/integration/autoland/rev/423f553dd2d5 Apply bookmark-moved event. r=mak https://hg.mozilla.org/integration/autoland/rev/52eff397be23 Remove onItemMoved from nsINavBookmarkObserver. r=mak
Comment 14•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fe8bfcd35e2b
https://hg.mozilla.org/mozilla-central/rev/99d7c325bf06
https://hg.mozilla.org/mozilla-central/rev/423f553dd2d5
https://hg.mozilla.org/mozilla-central/rev/52eff397be23
Description
•