1392248 - Bookmark onItemAdded could avoid 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 onItemAdded, Activity Stream is performing a fetch to the database.

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

It looks like it uses the fetch to get:

- title
- url
- lastModified

However, apart from lastModified, these are all in the onItemAdded notification. dateAdded is in the notification, but may not be the right thing to use - we could add lastModified if necessary.

So we should be able to replace the fetch call to the database, with straight information from onItemAdded.

Comment 1

[Mark Banner (:standard8)]

Kate, can you help here with the lastModified vs dateAdded?

When a bookmark is imported or synced, it could have an dateAdded that's different to the lastModified.

Does Activity Stream really depend on the lastModified here, rather than the dateAdded?

Comment 2

[Ed Lee :Mardak]

It does look like the reducer is just using `lastModified` for the `bookmarkDateCreated` state, so it would maybe seem like using `dateAdded` instead should be okay.

The original code was from k88hudson for Test Pilot version that interestingly SELECTS both `lastModified` and `dateAdded` but updates state with only `lastModified`:

https://github.com/mozilla/activity-stream/blame/master/common/reducers/SetRowsOrError.js#L43

Comment 3

[Ed Lee :Mardak]

Actually, the original usage of `lastModified` was from https://github.com/mozilla/activity-stream/pull/110/files last February. My guess is that it used that field because that was also what was provided to the observer's `onItemChanged`.

The desired functionality of showing bookmarks is more to make sure recently created bookmarks are shown, so I would think just using `dateAdded` is good enough.

Comment 4

[Kate Hudson :k88hudson]

Using dateAdded sounds fine to me!

Comment 5

[Ed Lee :Mardak]

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