1399474 - bookmarks.onChanged ChangeInfo only includes either the url or title, but not both

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[sblask]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170816210634

Steps to reproduce:

Add a listener: browser.bookmarks.onChanged.addListener((_id, changeInfo) => console.log(changeInfo)); and then change a bookmark title.


Actual results:

ChangeInfo contains only title, not url.


Expected results:

title AND url should be present. This is how it is in Chrome and this is what the documentation says: https://developer.mozilla.org/th/Add-ons/WebExtensions/API/bookmarks/onChanged

Comment 1

[Bob Silverberg [:bsilverberg]]

I suspect that the reason this was implemented the way it was is because the notification that we receive when a bookmark is changed only contains the property that was changed, so if only the title is changed, we only get notified of the title. If both the title and URL are changed we receive two events, and we fire two events, one for the title and one for the URL.

We could potentially fix this by querying for the title or URL whenever we receive the notification for the other, but that would be a performance hit, having to do that query each and every time a title or a URL is changed. It would also mean we'd be firing two events, each of which contain the same information, if both title and URL were updated.

A more performant solution would be to always return the URL and title from the Places bookmarks onItemChanged notification, by making changes to Bookmarks.jsm. It does appear that the title and URL are available when issuing the notification, but there are a number of existing consumers of that notification which would need to be considered.

Marco, what do you suggest?

Comment 2

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #1)
> I suspect that the reason this was implemented the way it was is because the
> notification that we receive when a bookmark is changed only contains the
> property that was changed

That's sort-of correct. onItemChanged contains a lot of info, but not the url.

> We could potentially fix this by querying for the title or URL whenever we
> receive the notification for the other, but that would be a performance hit,

Indeed, it's not a good idea because it may roundtrip to the disk.

> A more performant solution would be to always return the URL and title from
> the Places bookmarks onItemChanged notification, by making changes to
> Bookmarks.jsm. It does appear that the title and URL are available when
> issuing the notification, but there are a number of existing consumers of
> that notification which would need to be considered.

You can add a further optional argument at the end of onItemChanged, and then existing js consumers won't care. cpp consumers  will need the new method signature.
The only thing to verify is if all the call points notifying onItemChanged have the uri at hand, since if we have to fetch it, it would be a problem. Unfortunately this is also valid for the old API (nsNavBookmarks.cpp) since we didn't complete the work on bug 979280 in time for 57. Once bug 979280 will be done, likely in the 58 cycle, then Bookmarks.jsm will be the only thing that matters and most of nsNavBookmarks.cpp will go away.
If the timing here is not a problem, I'd probably wait for the old nsNavBookmarks.cpp API to be removed, so you don't have to update both APIs. Up to you.

Also, for the future we are already considering a rewrite of the notifications in a better way that will always send out the whole bookmark information, rather than just a part of it (bug 1340498).

Comment 3

[Bob Silverberg [:bsilverberg]]

Thanks Marco. It will be fine to wait for bug 979280 to land before this, which, as you say, will reduce the work required for this fix. I'll mark this as being blocked by  bug 979280.

Will, do you think you could update the docs for bookmarks.onChanged to describe how it currently differs from Chrome's implementation?

Comment 4

[Will Bamberg [:wbamberg]]

Sure thing, as long as it is dev-doc-needed it will end up on my plate.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> Thanks Marco. It will be fine to wait for bug 979280 to land before this,
> which, as you say, will reduce the work required for this fix. I'll mark
> this as being blocked by  bug 979280.

A small heads-up: to reduce risks we just decided the code removal will begin only in the 60 Nightly cycle. As such, if this has an higher priority, I fear it will have to touch both the old and new bookmarking APIs sooner. On the positive side, we care less about the legacy synchronous API and we could skip the info in those few cases where the uri may not be readily available.

Comment 6

[Bob Silverberg [:bsilverberg]]

Thanks for the heads-up, Marco. I think it's still fine to wait for the 60 cycle for this, but let's see what Mike thinks.

Comment 7

[Mike Conca [:mconca]]

I agree with Bob, we can wait for the 60 cycle to get to this.

Comment 8

[Bob Silverberg [:bsilverberg]]

Marco, it looks like bug 979280 is now marked as fixed. Looking back at your comment #2, does this mean that we can now make the changes suggested just to Bookmarks.jsm without worrying about any other APIs? Also, does that make sense to do now, or do you think we should wait on bug 1340498 for a more long-term fix?

Comment 9

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> Marco, it looks like bug 979280 is now marked as fixed. Looking back at your
> comment #2, does this mean that we can now make the changes suggested just
> to Bookmarks.jsm without worrying about any other APIs?

Unfortunately not, we won't be able to remove the old APIs until the Nightly 60 cycle for safety reasons (in case we need to revert something), as I said in comment 5.

Comment 10

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #9)
> (In reply to Bob Silverberg [:bsilverberg] from comment #8)
> > Marco, it looks like bug 979280 is now marked as fixed. Looking back at your
> > comment #2, does this mean that we can now make the changes suggested just
> > to Bookmarks.jsm without worrying about any other APIs?
> 
> Unfortunately not, we won't be able to remove the old APIs until the Nightly
> 60 cycle for safety reasons (in case we need to revert something), as I said
> in comment 5.

Ok, sorry, I assumed that that bug being fixed signaled this was ready. Is there a different bug I can block on for the removal of the old APIs?

Comment 11

[Marco Bonardo [:mak]]

Bug 1083465 is the closest thing.

Comment 12

[Bob Silverberg [:bsilverberg]]

Thanks Marco. :)

Comment 13

[Irene Smith]

Can someone tell me the status of this issue? I'm not sure exactly what information needs to be provided to readers for this.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Nothing has changed on the status here.

If it isn't already documented as such, a caveat could be added to the doc on bookmarks.onChanged that multiple events may occur for bookmark changes, and that events may only contain the data that has changed, rather than all the data for the bookmark.

Comment 15

[Irene Smith]

Added this to the onChange documentation:

Multiple events may occur when a bookmark changes, and that changeInfo object may contain only the data that has changed, rather than all the data for the bookmark. In other words, if the url for a bookmark changes, the changeInfo may only contain the new url information.