1392267 - Bookmark onItemChanged triggers unnecessary fetching from the places database

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement)

Description

[Mark Banner (:standard8)]

When profiling bulk actions for Places Bookmarks, we're seeing that Activity Stream is showing up in the data quite a bit.

One instance is for every onItemChanged, Activity Stream is performing a fetch to the database.

http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/browser/extensions/activity-stream/lib/PlacesFeed.jsm#129
http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/toolkit/modules/NewTabUtils.jsm#1029

From what I can tell, PLACES_BOOKMARK_CHANGED isn't listened to/used by the content process, so at the moment, this is unnecessary code that's impacting our performance.

If it is going to be used, then we should see if we can avoid the fetch - for example, we could only communicate the changes to the content process.

Comment 1

[Mark Banner (:standard8)]

Kate, should be clearing out the onItemChanged listener here, or is it likely to be used for something?

Comment 2

[Ed Lee :Mardak]

It'll be needed to implement highlights https://github.com/mozilla/activity-stream/issues/3147

Although that is blocked on implementing metadata fetching and storage, so it might be dropped from 57. In the meantime it would seem like this code could be removed or skipped.

Comment 3

[Ed Lee :Mardak]

Fixed by https://github.com/mozilla/activity-stream/pull/3322 to be uplifted via bug 1395642.