1436966 - Remove the bookmark-observers & history-observers category listeners

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Mark Banner (:standard8)]

Since bug 1371677, the bookmark-observers category listener is no longer used except for one test.

We should remove support for it as we no longer need it, especially with notification rework on the horizon.

Comment 1

[Mark Banner (:standard8)]

Turns out from a c++ perspective it is slightly easier to remove the history-observer as well at the same time.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1436966 - Remove the bookmark-observers & history-observers category listeners.

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

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8949690 [details]
Bug 1436966 - Remove the bookmark-observers & history-observers category listeners.

https://reviewboard.mozilla.org/r/219030/#review224776

Thanks, this may help with observers refactoring (and likely should block that bug)

::: toolkit/components/places/nsNavBookmarks.cpp:2847
(Diff revision 1)
>    nsCOMArray<nsINavBookmarkObserver> observers;
>  
> -  // First add the category cache observers.
> -  mCacheObservers.GetEntries(observers);
> -
>    // Then add the other observers.

this comment should likely go away

::: toolkit/components/places/nsPlacesExpiration.js:442
(Diff revision 1)
>        return;
>      }
>      this._shuttingDown = true;
>      this.expireOnIdle = false;
> +
> +    PlacesUtils.history.removeObserver(this);

Let's just add this as a weak observer, we don't need the observer service to keep us alive, and the history service will stop notifying by itself on shutdown.
so we can avoid this removal.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8949690 [details]
Bug 1436966 - Remove the bookmark-observers & history-observers category listeners.

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

Comment 5

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s cbe0c2cf72778c660c3d0ec7b070b062396605f4 -d 1f07c08daa41: rebasing 446568:cbe0c2cf7277 "Bug 1436966 - Remove the bookmark-observers & history-observers category listeners. r=mak" (tip)
merging toolkit/components/places/nsNavBookmarks.cpp
merging toolkit/components/places/nsNavBookmarks.h
warning: conflicts while merging toolkit/components/places/nsNavBookmarks.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8949690 [details]
Bug 1436966 - Remove the bookmark-observers & history-observers category listeners.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64ec4273a0d7
Remove the bookmark-observers & history-observers category listeners. r=mak

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/64ec4273a0d7

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/64ec4273a0d7