Closed Bug 1511062 Opened 3 years ago Closed 7 months ago

Replace nsINavBookmarksService::onItemMoved with bookmark-moved

Categories

(Toolkit :: Places, enhancement, P3)

enhancement
Points:
3

Tracking

()

RESOLVED FIXED
90 Branch
Iteration:
86.3 - Jan 11 - Jan 24
Tracking Status
firefox90 --- fixed

People

(Reporter: standard8, Assigned: daisuke)

References

(Blocks 1 open bug)

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.
Blocks: 1473530
No longer blocks: 1426245
Summary: Replace nsINavBookmarksService::onItemMoved with bookmark-item-moved → Replace nsINavBookmarksService::onItemMoved with bookmark-moved
Assignee: nobody → daisuke
Status: NEW → ASSIGNED
Iteration: --- → 86.3 - Jan 11 - Jan 24
Points: --- → 3

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!

Flags: needinfo?(standard8)

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).

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.

Flags: needinfo?(standard8)

Thank you very much for your helpful advice. Mark.
I try that!

Depends on D102573

Depends on: 1689275
Attachment #9198384 - Attachment is obsolete: true
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

Backed out for causing bustage on nsNavHistoryResult.cpp

[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.

Flags: needinfo?(daisuke)

Thanks. I will take a look at this issue.

Flags: needinfo?(daisuke)
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
You need to log in before you can comment on or make changes to this bug.