1511062 - Replace nsINavBookmarksService::onItemMoved with bookmark-moved

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P3)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Daisuke Akatsuka (:daisuke)]

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!

Comment 2

[Daisuke Akatsuka (:daisuke)]

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

Comment 3

[Mark Banner (:standard8)]

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.

Comment 4

[Daisuke Akatsuka (:daisuke)]

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

Comment 5

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1511062: Change key of bookmark observer to bookmark GUID.

Comment 6

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1511062: Remove old/new parent id from onItemMoved in nsINavBookmarkObserver.

Depends on D102571

Comment 7

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1511062: Implement a mechanism to fire bookmark-moved event.

Depends on D102572

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1511062: Apply bookmark-moved event.

Depends on D102573

Comment 9

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1511062: Remove onItemMoved from nsINavBookmarkObserver.

Depends on D102574

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Natalia Csoregi [:nataliaCs]]

Backed out for causing bustage on nsNavHistoryResult.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/973389a03655b338bcb93178344d31ceccc5fa10

• push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=ed9ac8c3097b6680f99a28f64d6f1e7e96c6ed2b&group_state=expanded

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

Comment 12

[Daisuke Akatsuka (:daisuke)]

Thanks. I will take a look at this issue.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

[Cristian Brindusan [:cbrindusan]]

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