Closed Bug 1426245 Opened 2 years ago Closed 1 year ago

Replace nsINavBookmarksService::onItemAdded with "page-bookmarked"

Categories

(Firefox :: Migration, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: dthayer, Assigned: dthayer)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fxperf:p1])

Attachments

(7 files)

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.
Depends on: 1340498
Depends on: 1473530
No longer depends on: 1340498
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Summary: Replace nsINavBookmarksService::onItemAdded with nsINavBookmarksService::onItemsAdded → Replace nsINavBookmarksService::onItemAdded with "page-bookmarked"
Whiteboard: [fxperf:p1]
Blocks: 1473530
No longer depends on: 1473530
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.
Attachment #8990464 - Flags: review?(mak77)
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.
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 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...
Attachment #8990464 - Flags: review?(mak77)
Comment on attachment 8992833 [details]
Bug 1426245 - Allow partial removal of places oberver listeners

https://reviewboard.mozilla.org/r/257674/#review267336
Attachment #8992833 - Flags: review?(mak77) → review+
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
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 on attachment 9005037 [details]
Bug 1426245 - Allow partial removal of places oberver listeners r=mak

Marco Bonardo [::mak] has approved the revision.
Attachment #9005037 - Flags: review+
Comment on attachment 9005039 [details]
Bug 1426245 - Replace OnItemAdded with bookmark-item-added r=mak

Marco Bonardo [::mak] has approved the revision.
Attachment #9005039 - Flags: review+
MozReview-Commit-ID: 4fhhzspxLJZ

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

Marco Bonardo [::mak] has approved the revision.
Attachment #9006967 - Flags: review+
Doug, is there a reason this hasn't been landed yet?
Flags: needinfo?(dothayer)
(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.
Flags: needinfo?(dothayer)
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
Flags: needinfo?(usarracini)
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! :)
Flags: needinfo?(usarracini)
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?
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
Pushed by dothayer@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/01f732762529
https://hg.mozilla.org/mozilla-central/rev/50ca67245a71
https://hg.mozilla.org/mozilla-central/rev/85a806b69f15
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
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)
Depends on: 1511062
No longer depends on: 1511062
You need to log in before you can comment on or make changes to this bug.