1426245 - Replace nsINavBookmarksService::onItemAdded with "page-bookmarked"

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement, P3)

Description

[Alex Thayer [:alexical] (she/her)]

Currently when importing a large number of bookmarks, a good deal of time is spent notifying activity stream via onItemAdded. Activity stream then sends an async IPC message so that the new tab page in the content process can update its state. In the content process we then iterate over what I believe is all of our bookmarks to check if any of them match the URL of the added bookmark. So we end up with something like O(n) IPC messages and O(n^2) URL comparisons in the content process. If we sent one, or a small number of onItemsAdded messages each with an array of bookmarks, we could reduce that to O(1) IPC messages and O(n) URL comparisons.

Comment 1

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1426245 - eslint changes for bookmark-item-added

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

Comment 2

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1426245 - Replace OnItemAdded with bookmark-item-added

See https://docs.google.com/document/d/1G45vfd6RXFXwNz7i4FV40lDCU0ao-JX_bZdgJV4tLjk/edit#
for further info. This essentially follows the same philosophy as
the onVisits migration.

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

Comment 3

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1426245 - Test changes

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

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8990464 [details]
Bug 1426245 - Replace OnItemAdded with bookmark-item-added

https://reviewboard.mozilla.org/r/255504/#review263394

::: dom/base/PlacesBookmarkItem.h:46
(Diff revision 1)
> +    return PlacesBookmarkItem_Binding::Wrap(aCx, this, aGivenProto);
> +  }
> +
> +  const PlacesBookmarkItem* AsPlacesBookmarkItem() const override { return this; }
> +
> +  PlacesBookmarkItemType ItemType() { return mItemType; }

How cheap is this enum, is it passing around strings?
I'd prefer to keep using the old integer types exposed by Bookmarks.jsm

The problem there is tags, but it's a temporary problem because tags won't keep being bookmarks.
I think you added tags to the notifications because the old notification has skipTags that this one doesn't have.
The tags conversion is likely to happen in the next few months, but I don't have a clear ETA.
I suppose we could introduce temporary types for tags used only in these notifications, and in the future remove them.

Otherwise the only other plausible solution would be to wait for tags to move :(

::: dom/base/PlacesBookmarkItem.h:52
(Diff revision 1)
> +  int64_t ItemId() { return mItemId; }
> +  int64_t ParentItemId() { return mParentItemId; }
> +  int32_t Index() { return mIndex; }
> +  void GetUrl(nsString& aUrl) { aUrl = mUrl; }
> +  void GetTitle(nsString& aTitle) { aTitle = mTitle; }
> +  uint64_t DateAdded() { return mDateAdded; }

maybe we should expose a full bookmark object as indicated by Bookmarks.jsm, that additionally also includes lastModified.

::: dom/base/PlacesBookmarkItem.h:54
(Diff revision 1)
> +  int32_t Index() { return mIndex; }
> +  void GetUrl(nsString& aUrl) { aUrl = mUrl; }
> +  void GetTitle(nsString& aTitle) { aTitle = mTitle; }
> +  uint64_t DateAdded() { return mDateAdded; }
> +  void GetItemGuid(nsCString& aItemGuid) { aItemGuid = mItemGuid; }
> +  void GetParentItemGuid(nsCString& aParentItemGuid) { aParentItemGuid = mParentItemGuid; }

Let's simplify naming a little bit: id, parentId, guid, parentGuid

::: dom/chrome-webidl/PlacesEvent.webidl:11
(Diff revision 1)
>     */
>    "page-visited",
> +  /**
> +   * data: PlacesBookmarkItem. Fired whenever a bookmark folder is created.
> +   */
> +  "bookmark-item-added",

why not just bookmark-added?

Also, the comment looks wrong, since it only says "a bookmark folder", but this happens for urls, separators and folders

::: dom/chrome-webidl/PlacesEvent.webidl:97
(Diff revision 1)
> +  required ByteString parentItemGuid;
> +  required unsigned short source;
> +};
> +
> +[ChromeOnly, Exposed=(Window,System), Constructor(PlacesBookmarkItemInit initDict)]
> +interface PlacesBookmarkItem : PlacesEvent {

How will we name removals and edits?

PlacesBookmarkItem is a too generic name for addition, maybe PlacesInsertBookmarkEvent?

Is there a way to have a base PlacesBookmark with these basic properties, and extend it with specific name and special properties for each future event (remove, change)

::: dom/chrome-webidl/PlacesEvent.webidl:107
(Diff revision 1)
> +  readonly attribute DOMString url;
> +  readonly attribute DOMString title;
> +  readonly attribute unsigned long long dateAdded;
> +  readonly attribute ByteString itemGuid;
> +  readonly attribute ByteString parentItemGuid;
> +  readonly attribute unsigned short source;

the attributes should be documented, like for history

::: services/sync/modules/engines/bookmarks.js:1404
(Diff revision 1)
> +      this._log.trace("'bookmark-item-added': " + event.itemId);
> +      this._upScore();
> +    }
> +  },
> +
>    onItemAdded: function BMT_onItemAdded(itemId, folder, index,

who is invoking this now?

::: toolkit/components/places/Bookmarks.jsm:286
(Diff revision 1)
>        // Pass tagging information for the observers to skip over these notifications when needed.
>        let isTagging = parent._parentId == PlacesUtils.tagsFolderId;
>        let isTagsFolder = parent._id == PlacesUtils.tagsFolderId;
> -      notify(observers, "onItemAdded", [ itemId, parent._id, item.index,
> -                                         item.type, uri, item.title,
> -                                         PlacesUtils.toPRTime(item.dateAdded), item.guid,
> +      let url = "";
> +      if (item.type == Bookmarks.TYPE_BOOKMARK) {
> +        url = (item.url instanceof URL) ? item.url.href : item.url;

we always convert input to a URL object, so this is probably always true

::: toolkit/components/places/nsNavHistoryResult.cpp:4007
(Diff revision 1)
>      }
>    }
>  }
>  
>  void
>  nsNavHistoryResult::AddHistoryObserver(nsNavHistoryQueryResultNode* aNode)

I think there's a problem here, a query can listen to history notifications, or bookmark notifications, or both, depending on what the query is.
The discrimation is done in nsNavHistoryQueryResultNode::FillChildren

so we have AddHistoryObserver and AddAllBookmarksObserver (depending if the query returns only history, or also bookmarks)

Additionally, the result may use AddBookmarkFolderObserver, if it's a folder, because it only cares about bookmark changes happening inside itself.

In practice listening to bookmark events here is wrong, because this way an history-only query would receive bookmark notifications, that are pointless.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8990464 [details]
Bug 1426245 - Replace OnItemAdded with bookmark-item-added

https://reviewboard.mozilla.org/r/255504/#review263394

> How cheap is this enum, is it passing around strings?
> I'd prefer to keep using the old integer types exposed by Bookmarks.jsm
> 
> The problem there is tags, but it's a temporary problem because tags won't keep being bookmarks.
> I think you added tags to the notifications because the old notification has skipTags that this one doesn't have.
> The tags conversion is likely to happen in the next few months, but I don't have a clear ETA.
> I suppose we could introduce temporary types for tags used only in these notifications, and in the future remove them.
> 
> Otherwise the only other plausible solution would be to wait for tags to move :(

It's very cheap. It's just an integer in C++ and I believe it's an atomized string when it goes to JS. That being said I'm fine with changing it if tags are being converted soon. I can also just add an isTag boolean instead of introducing temporary types.

Comment 6

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1426245 - Allow partial removal of places oberver listeners

This was a bug in the original design that just hadn't shown up yet.
If I were to listen to, say "notification-a", and then try to remove
my listener for "notification-a" and "notification-b", we would end
up with a dead listener staying in the list. This fixes that.

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

Comment 7

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8990463 [details]
Bug 1426245 - eslint changes for bookmark-item-added

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

Comment 8

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8990464 [details]
Bug 1426245 - Replace OnItemAdded with bookmark-item-added

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

Comment 9

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8990465 [details]
Bug 1426245 - Test changes

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

Comment 10

[Mark Banner (:standard8)]

By the way, for the ESLint addition, I'm happy with those being rolled into the patch that actually adds the global, rather than having a separate patch.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8990464 [details]
Bug 1426245 - Replace OnItemAdded with bookmark-item-added

https://reviewboard.mozilla.org/r/255504/#review267272

::: browser/base/content/browser-places.js:1701
(Diff revision 2)
> -    if (aURI && aURI.equals(this._uri)) {
> +      if (url && url == this._uri.spec) {
> -      // If a new bookmark has been added to the tracked uri, register it.
> +        // If a new bookmark has been added to the tracked uri, register it.
> -      if (!this._itemGuids.has(aGuid)) {
> -        this._itemGuids.add(aGuid);
> +        if (!this._itemGuids.has(guid)) {
> +          this._itemGuids.add(guid);
> -        // Only need to update the UI if it wasn't marked as starred before:
> +          // Only need to update the UI if it wasn't marked as starred before:
> -        if (this._itemGuids.size == 1) {
> +          if (this._itemGuids.size == 1) {

We can do this check and updateStart our of the loop, I think.

::: dom/chrome-webidl/PlacesEvent.webidl:119
(Diff revision 2)
> +   */
> +  readonly attribute unsigned short source;
> +
> +  /**
> +   * True if the item is a tag or a tag folder
> +   */

please add a note that this will go away with bug 424160

::: toolkit/components/places/nsNavHistoryResult.cpp:4079
(Diff revision 2)
>      nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>      if (!bookmarks) {
>        MOZ_ASSERT_UNREACHABLE("Can't create bookmark service");
>        return;
>      }
>      bookmarks->AddObserver(this, true);

I think something is still missing here, now you properly added an allBookmarksObserver, but folders don't use that, they use addBookmarkFolderObserver, that is pretty much the same, but filters on parentId.
Regardless, it lookos like this addObserver doesn't have a corresponding listener in the new world...

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8992833 [details]
Bug 1426245 - Allow partial removal of places oberver listeners

https://reviewboard.mozilla.org/r/257674/#review267336

Comment 13

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1426245 - Allow partial removal of places oberver listeners r=mak

This was a bug in the original design that just hadn't shown up yet.
If I were to listen to, say "notification-a", and then try to remove
my listener for "notification-a" and "notification-b", we would end
up with a dead listener staying in the list. This fixes that.

MozReview-Commit-ID: KhYQSJaBDF9

Comment 14

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1426245 - Replace OnItemAdded with bookmark-item-added r=mak

See https://docs.google.com/document/d/1G45vfd6RXFXwNz7i4FV40lDCU0ao-JX_bZdgJV4tLjk/edit#
for further info. This essentially follows the same philosophy as
the onVisits migration.

MozReview-Commit-ID: I4bOvFH0ZQR

Depends on D4605

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 9005037 [details]
Bug 1426245 - Allow partial removal of places oberver listeners r=mak

Marco Bonardo [::mak] has approved the revision.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 9005039 [details]
Bug 1426245 - Replace OnItemAdded with bookmark-item-added r=mak

Marco Bonardo [::mak] has approved the revision.

Comment 17

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1426245 - Test changes r=mak

MozReview-Commit-ID: 4fhhzspxLJZ

Depends on D4606

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 9006967 [details]
Bug 1426245 - Test changes r=mak

Marco Bonardo [::mak] has approved the revision.

Comment 19

[Mark Banner (:standard8)]

Doug, is there a reason this hasn't been landed yet?

Comment 20

[Alex Thayer [:alexical] (she/her)]

(In reply to Mark Banner (:standard8) from comment #19)
> Doug, is there a reason this hasn't been landed yet?

It was failing a few tests on try which were a bit trickier to diagnose, and I got bogged down with other things. I'm catching up on it now though and hopefully should have all the kinks worked out shortly.

Comment 21

[Alex Thayer [:alexical] (she/her)]

Ursula, do I need to submit my PlacesFeed.jsm changes[1] as a PR to the Activity Stream GitHub before landing? I'm not sure how your process works with that.

[1] https://phabricator.services.mozilla.com/D4606#change-B1vRyTWxeAkc

Comment 22

[Ursula Sarracini (:ursula)]

Hey Doug, don't worry about making a PR to the Github repo for these changes, once this lands in central we'll do a backport for the changes that were made in PlacesFeed.jsm. Thanks for the heads up! :)

Comment 23

[Alex Thayer [:alexical] (she/her)]

The test failures appear to be cleared up now, so, Marco, are you comfortable with landing this now that it's later in the cycle, or would you prefer I wait until next cycle?

Comment 24

[Marco Bonardo [:mak]]

Considered we don't plan any major work on top of this, it should remain an easy backout in case of a total disaster (that I don't expect!). We are 2 weeks from the next merge, so there should also be enough time to manage eventual problems.
I'd proceed with landing.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01f732762529
Allow partial removal of places oberver listeners r=mak
https://hg.mozilla.org/integration/autoland/rev/50ca67245a71
Replace OnItemAdded with bookmark-item-added r=mak
https://hg.mozilla.org/integration/autoland/rev/85a806b69f15
Test changes r=mak

Comment 26

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/01f732762529
https://hg.mozilla.org/mozilla-central/rev/50ca67245a71
https://hg.mozilla.org/mozilla-central/rev/85a806b69f15

Comment 27

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/5506dbd17ac218a8267db028679a5cd2c49466db
Backport Bug 1426245 - Replace nsINavBookmarksService::onItemAdded with 'page-bookmarked' (#4483)