Closed
Bug 1221764
Opened 9 years ago
Closed 8 years ago
Implement simple chrome.bookmarks events
Categories
(WebExtensions :: General, defect, P3)
WebExtensions
General
Tracking
(firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: evilpie, Assigned: bsilverberg)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete, Whiteboard: [bookmarks]triaged)
Attachments
(1 file, 6 obsolete files)
I implemented onCreated, onRemoved, onChanged and onMoved still missing are onChildrenReordered, onImportBegan and onImportEnded.
I am going to look at adding some tests.
Reporter | ||
Comment 1•9 years ago
|
||
Assignee: nobody → evilpies
Attachment #8683337 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8683339 -
Flags: review?(wmccloskey)
Attachment #8683339 -
Flags: review?(mak77)
Reporter | ||
Updated•9 years ago
|
Whiteboard: [bookmarks]
Comment on attachment 8683339 [details] [diff] [review]
v1 - chrome.bookmarks events
Review of attachment 8683339 [details] [diff] [review]:
-----------------------------------------------------------------
This really needs Marco's review. I've made some comments, but he's really the only person who needs to look at this.
::: browser/components/extensions/ext-bookmarks.js
@@ +79,5 @@
> return node;
> }
>
> +let lastTagsFolderGuid;
> +function ifNotTag(guid, parentGuid, callback) {
This thing seems really sketchy, especially the lastTagsFolderGuid thing. Hopefully Marco can suggest something better.
@@ +114,5 @@
> +
> + onBeginUpdateBatch() {},
> + onEndUpdateBatch() {},
> +
> + onItemAdded(id, parentId, index, itemType, uri, title, dateAdded, guid, parentGuid) {
How about calling it dateAddedMicros.
@@ +126,5 @@
> + id: guid,
> + parentId: parentGuid,
> + index,
> + title,
> + dateAdded: dateAdded / 1000
Do we want to make children be []?
@@ +147,5 @@
> + itemType == PlacesUtils.bookmarks.TYPE_SEPARATOR) {
> + return;
> + }
> +
> + // Note: Tags aren't moved
Period at the end.
@@ +157,5 @@
> + };
> + this.fire(guid, info);
> + },
> +
> + onItemRemoved(id, parentId, index, itemType, uri, guid, parentGuid) {
It looks like onItemRemoved is called for every item in a removed folder, while Chrome only calls it for the folder. Not sure if there's anything we can do for that.
Attachment #8683339 -
Flags: review?(wmccloskey)
Comment 3•9 years ago
|
||
Comment on attachment 8683339 [details] [diff] [review]
v1 - chrome.bookmarks events
Review of attachment 8683339 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/extensions/ext-bookmarks.js
@@ +88,5 @@
> + return;
> + }
> +
> + // When removing a tag bookmark the parent tags folder is delete before
> + // the tags inside it. So we need to save that guid.
Really? I would expect the opposite since tagging service first removes the tagged urls and then calls _removeTagIfEmpty
How did you remove the tag? By chance does this happen if you remove a tag folder in the Library?
@@ +93,5 @@
> + if (parentGuid == lastTagsFolderGuid) {
> + return;
> + }
> +
> + Bookmarks.fetch({guid: parentGuid}).then(result => {
Yeah this is unfortunate.
Fwiw, the tagging service is so horrible that I wouldn't mind making it worse.
So if you think you'd benefit from that, you could expose a getTagForItemId from nsITaggingService, or use *cough* wrappedJSObject *cough*.
Regardless we will throw away that code for Tagging.jsm, as soon as we find any resources to work on that, then there will be no problem cause tags won't be bookmarks anymore (and we can party).
@@ +143,5 @@
> + onItemVisited() {},
> +
> + onItemMoved(id, oldParentId, oldIndex, newParentId, newIndex, itemType, guid, oldParentGuid, newParentGuid) {
> + if (this.type != "bookmarks.onMoved" ||
> + itemType == PlacesUtils.bookmarks.TYPE_SEPARATOR) {
consistency-nit: sometimes you use Bookmarks and other times PlacesUtils.bookmarks.
@@ +157,5 @@
> + };
> + this.fire(guid, info);
> + },
> +
> + onItemRemoved(id, parentId, index, itemType, uri, guid, parentGuid) {
Unfortunately as Bill pointed out, we notify every removal, mostly cause most add-ons track urls rather than folders, that is a mere hierarchical annotation on a bookmark. We go recursively so children are notified before folders.
the only workaround I could suggest would be to store the deleted folder guid, and don't issue any url removal notification until an onItemRemoved for the given folder guid is hit. Then you would only notify folders. The problem is that if in the meanwhile something else removes a bookmark, you may wrongly filter out its notifications...
@@ +179,5 @@
> + ifNotTag(guid, parentGuid, () => {
> + let info = {};
> + if (prop == "title") {
> + info.title = val;
> + } else if (prop == "uri") {
The cheapest filter should always go first. filtering on prop is cheaper than filtering out tags, so it should happen before.
@@ +181,5 @@
> + if (prop == "title") {
> + info.title = val;
> + } else if (prop == "uri") {
> + info.url = val;
> + } else {
The cheapest filter should always go first. filtering on prop is cheaper than filtering out tags, so it should happen before.
@@ +197,5 @@
> + EventManager.call(this, context, name, fire => {
> + let observer = new BookmarkObserver(name, fire);
> + PlacesUtils.bookmarks.addObserver(observer, false);
> + return () => {
> + PlacesUtils.bookmarks.removeObserver(observer);
could you please explain me how this addition/removal works? It's a little bit cryptic.
how many observers are we adding and when are we removing them?
Attachment #8683339 -
Flags: review?(mak77) → feedback+
Reporter | ||
Comment 4•9 years ago
|
||
Could we maybe just change Bookmarks.jsm to give us the events that we need? So no tags and no notifications for items inside folders that are deleted.
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 5•9 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #4)
> Could we maybe just change Bookmarks.jsm to give us the events that we need?
> So no tags and no notifications for items inside folders that are deleted.
the no tags problem is an architectural one that will disappear naturally once we move them elsewhere, that I hope will happen next year. So I'd not care about it for now.
The problem with not sending notifications for items is that other listeners ARE expecting those notifications, so not sending them would broke them.
What we could do is to modify nsINavBookmarkObserver, add a readonly skipRemoveChildrenNotifications attribute and set it to true on the particular nsINavBookmarkObserver implementation you have. Then Bookmarks.jsm could check that flag on the observer when sending a remove notification of a bookmark and decide what to do.
It should be feasible, provided we assume the new attribute may not exist in the implementation so we keep compatibility with existing implementations, it will just need some additional xpcshell-tests in toolkit/places.
Even better, I think we may be able to reuse this in our treeviews to speed up folders removals.
Reporter | ||
Comment 6•9 years ago
|
||
So I started working on this and noticed something strange: When bookmarking with the star we don't seem to go through Bookmarks.jsm:insert at all?
Comment 7•9 years ago
|
||
heh, we never finished the porting from the old bookmarks API to Bookmarks.jsm, so some parts of the UI still use the old API...
That work is planned since the old API should be deprecated, but since all resources have been pulled I can't give you an ETA.
I don't see an easy way out, some removals will behave properly, others may still notify every child, until the switch to Bookmarks.jsm is complete.
Comment 8•9 years ago
|
||
fwiw, we could implement skipping also in the old cpp implementation and it should work
Reporter | ||
Comment 9•9 years ago
|
||
I now started checking the webextension flag in nsNavBookmarks.jsm. However there are still tons of macro invocations that need to be verified.
Assignee | ||
Comment 10•9 years ago
|
||
evilpie: Can you please comment on the status of this bug? I am taking responsibility for bug 1213674 which depends on this. Thanks!
Flags: needinfo?(evilpies)
Reporter | ||
Comment 11•9 years ago
|
||
At the moment I am a bit busy with exam work, so I can't work on this for the next two weeks. It would be interesting to know if the changes to nsNavBookmarks.cpp make sense. If somebody would actual just do the C++ part we could get this landed after adding tests.
Flags: needinfo?(evilpies)
Assignee | ||
Comment 12•9 years ago
|
||
Thanks evilpie. mak, do you mind taking a look at this and commenting on what still needs to be done to move forward with this?
Flags: needinfo?(mak77)
Comment 13•9 years ago
|
||
As I said, we need to add a settable attribute to nsINavBookmarkObserver (something like "skipChildrenOnFolderDelete"), and when something notifies onItemRemove, it should check if the observer supports that attribute (old observers won't have it) and check its value, then notify everything or only the folder.
The patch looks on the right direction, I'd probably change the attribute name, and try to not hide all the logic in the macro (that is, try to retain the existing macros and add logic code around their invokation) since in the end it's only onItemRemoved changing.
The tags filtering problem, imo, should be left alone for now and properly fixed separately.
Flags: needinfo?(mak77)
Reporter | ||
Comment 14•9 years ago
|
||
I audited all the places in nsNavBookmarks.cpp that add or remove items. This is usually seems to be enough to prevent tags from showing up in the event listener.
We need to change onItemAdded as well, because we don't get a grandparent id for these either. I think changing the macro is a good idea, because otherwise we will have to add this ugly code for checking the listener everywhere.
Now we need to tests for both the old and the new API.
Attachment #8730126 -
Flags: feedback?(mak77)
Reporter | ||
Updated•9 years ago
|
Attachment #8683339 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8709128 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8730126 -
Attachment is patch: true
Comment 15•9 years ago
|
||
Comment on attachment 8730126 [details] [diff] [review]
v2 Bookmarks
Review of attachment 8730126 [details] [diff] [review]:
-----------------------------------------------------------------
I'm sorry, but I'd really like the 2 attributes split into skipDescendantsOnItemRemoval and skipTags. Their meaning is pretty clear I think.
The reason is the former will be re-used in Firefox (I already know where we could benefit from it), while the latter will be removed and ignored as soon as we convert tags. It will be useful during the conversion probably.
I'll accept the macros changes, though I wonder if alternatively we could add a "skipIf" part to the existing macros and put the check in the calling point? I want things to be more explicit.
Same for the notify js methods, we could pass a boolean filtering function, this is also easier to do than the cpp part and will work better if we add more options (also for history).
::: toolkit/components/places/Bookmarks.jsm
@@ +1604,5 @@
> notify(observers, "onItemChanged", [ entry._id, "tags", false, "",
> toPRTime(entry.lastModified),
> entry.type, entry._parentId,
> entry.guid, entry.parentGuid,
> + "" ], true);
pay attention to not confuse "changes to a tag", with "changing tags of an item"
I think the latter should be notified, and filtered later if webExt doesn't care about the change (it will just have to check the changing property is not "tags")
::: toolkit/components/places/PlacesUtils.jsm
@@ +87,5 @@
> * the notification name.
> * @param args
> * array of arguments to pass to the notification.
> */
> +function notify(observers, notification, args, ignoreWebExtensions) {
this is not implemented, but I guess you already know.
::: toolkit/components/places/nsPlacesMacros.h
@@ +38,5 @@
> + \
> + entries[idx]->method; \
> + } \
> + \
> + for (uint32_t idx = 0; idx < array.Length(); ++idx) { \
please add a // Copied from ENUMERATE_WEAKARRAY.
Attachment #8730126 -
Flags: feedback?(mak77)
Updated•9 years ago
|
Priority: -- → P1
Whiteboard: [bookmarks] → [bookmarks]triaged
Comment 17•9 years ago
|
||
Tracking for 49 - Andy will this need a release note to be added?
Flags: needinfo?(amckay)
Updated•9 years ago
|
Reporter | ||
Comment 18•9 years ago
|
||
I am currently rebasing my patch for this and applying Marco's 2 month old feedback. Sorry for not getting back to this sooner.
Reporter | ||
Comment 19•9 years ago
|
||
There we go. Still missing tests. We probably need two independent tests, one for Bookmarks.jsm and one for the C++ implementation.
Attachment #8753857 -
Flags: feedback?(mak77)
Comment 20•9 years ago
|
||
Comment on attachment 8753857 [details] [diff] [review]
v3 Bookmarks
Review of attachment 8753857 [details] [diff] [review]:
-----------------------------------------------------------------
Yep, this needs a test for each of the 2 properties and each of the 2 bookmarking APIs. Then you'll likely need tests for webext...
Note, the comments about the points where you missed to skip in Bookmarks.jsm are likely also valid for nsNavBookmarks.cpp
::: browser/components/extensions/ext-bookmarks.js
@@ +106,5 @@
> + id: guid,
> + parentId: parentGuid,
> + index,
> + title,
> + dateAdded: dateAdded / 1000
we should soon have a toDate in PlacesUtils (bug 1265836)
@@ +317,5 @@
> },
> +
> + onCreated: new BookmarkEventManager(context, "bookmarks.onCreated").api(),
> +
> + onRemoved: new BookmarkEventManager(context, "bookmarks.onRemoved").api(),
I don't like the fact you are adding a new observer per each notification, it is not efficient, cause the bookmarks service notifies all topics to all observers, and you are adding 4 new ones that will have to be notified every time.
I'd prefer if the bookmarks observer would be unique, even if that complicates the code a little bit, it will not be a perf hit.
::: toolkit/components/places/Bookmarks.jsm
@@ +180,5 @@
> let itemId = yield PlacesUtils.promiseItemId(item.guid);
> +
> + // Ignore tagging related bookmarks for WebExtension observers.
> + let isTagging = parent._parentId == PlacesUtils.tagsFolderId;
> + let isTagsFolder = parent._id == PlacesUtils.tagsFolderId;
isTagFolder (no plural "Tags")
@@ +412,5 @@
>
> // Notify onItemRemoved to listeners.
> let observers = PlacesUtils.bookmarks.getObservers();
> let uri = item.hasOwnProperty("url") ? toURI(item.url) : null;
> + let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId;
why is this not also handling the case where a tag folder is removed? That can happen when there are no more entries for a given tag.
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsTaggingService.js#205
@@ +417,4 @@
> notify(observers, "onItemRemoved", [ item._id, item._parentId, item.index,
> item.type, uri, item.guid,
> + item.parentGuid ],
> + { isTagging: isUntagging });
there is an onItemChanged with property "title" that can likely happen for tag folders and should probably be skipped as well
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#334
::: toolkit/components/places/nsINavBookmarksService.idl
@@ +12,5 @@
>
> /**
> * Observer for bookmarks changes.
> */
> +[scriptable, uuid(c06b4e7d-15b1-4d4f-bdf7-147d2be9084a)]
fwiw, it is no more needed to bump the uuids... Doing it won't hurt, but it's not necessary.
::: toolkit/components/places/nsNavBookmarks.cpp
@@ +472,5 @@
> +bool SkipDescendants(nsCOMPtr<nsINavBookmarkObserver> obs) {
> + bool skipDescendantsOnItemRemoval = false;
> + (void) obs->GetSkipTags(&skipDescendantsOnItemRemoval);
> + return skipDescendantsOnItemRemoval;
> +}
well ok, you convinced me.
Let's just pass isTagging and isDescendantRemoval to NOTIFY_BOOKMARKS_OBSERVERS as booleans, and avoid all these calls.
Attachment #8753857 -
Flags: feedback?(mak77)
Comment 21•9 years ago
|
||
This isn't going to make 49 and no, it doesn't need release notes.
tracking-firefox49:
+ → ---
Flags: needinfo?(amckay)
Reporter | ||
Updated•8 years ago
|
Assignee: evilpies → nobody
Status: ASSIGNED → NEW
Comment 22•8 years ago
|
||
Bob, as our resident bookmarks expert, would you mind picking this up?
Flags: needinfo?(bob.silverberg)
Assignee | ||
Comment 23•8 years ago
|
||
Sure, I'll add it to my list.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Flags: needinfo?(bob.silverberg)
Priority: P1 → P3
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8730126 [details] [diff] [review]
v2 Bookmarks
I believe that v3 obsoletes this patch.
Attachment #8730126 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8753857 [details] [diff] [review]
v3 Bookmarks
Marking v3 as obsolete as I have replaced it with a MozReview request.
Attachment #8753857 -
Attachment is obsolete: true
Assignee | ||
Comment 27•8 years ago
|
||
I have rebased Tom's latest patch against m-c and submitted it as a MozReview request. There are still some outstanding issues with it as raised against the earlier patches in Bugzilla comments. Also, a significant part of the patch involves native changes that I am not in a position to work on. I gather from earlier comments that these changes are for the most part r+'d, but I'm not 100% sure about that. I would need someone else to work on those parts, perhaps Marco, Kris or Andrew. I think it might make the most sense to split those into a separate bug which could block then JavaScript changes and could potentially land ahead of the JS changes. I see that Marco is away until Aug 20 and is not accepting needinfo requests. Are any of the above-mentioned people willing to take the native changes in in a separate bug?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
Comment 28•8 years ago
|
||
I don't think I'm the right person to handle the c++ changes. It looks like the c++ part is probably close, maybe if you split it as you suggested, :evilpie could get the c++ landed and you can do the rest of the js work?
Flags: needinfo?(aswan)
Assignee | ||
Comment 29•8 years ago
|
||
What do you think, Tom? If I split out just the c++ changes into a separate bug, do you think you can find the time to see those through to landing?
Flags: needinfo?(evilpies)
Reporter | ||
Comment 30•8 years ago
|
||
Okay, I have a faster computer now so compiling would be quicker.
Flags: needinfo?(evilpies)
Assignee | ||
Comment 31•8 years ago
|
||
Great, thanks Tom. I will spin off another bug and attach a patch to it with just those changes.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Comment 34•8 years ago
|
||
hrm. That last attachment has the bug # here, but it was meant for bug 1296401.
Updated•8 years ago
|
Attachment #8797323 -
Attachment is obsolete: true
Attachment #8797323 -
Flags: review?(markh)
Updated•8 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: General
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8782013 -
Flags: review?(mak77)
Assignee | ||
Comment 37•8 years ago
|
||
This is ready for an initial review. There are still a couple of decisions to make wrt titles and the onChanged and onRemoved events, but other than that this is good to go.
Comment 38•8 years ago
|
||
mozreview-review |
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
https://reviewboard.mozilla.org/r/72304/#review89916
A few nits, I haven't had a chance to look at the test yet but r? me again when this is ready for landing
::: browser/components/extensions/ext-bookmarks.js:8
(Diff revision 4)
> "use strict";
>
> const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>
> Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +var {
var -> const
::: browser/components/extensions/ext-bookmarks.js:85
(Diff revision 4)
> }
>
> return node;
> }
>
> +var observer;
var -> let
::: browser/components/extensions/ext-bookmarks.js:93
(Diff revision 4)
> + if (!observer) {
> + observer = {
> + skipTags: true,
> + skipDescendantsOnItemRemoval: true,
> +
> + onBeginUpdateBatch: function() {},
You can just use the `onBeginUpdateBatch() { ... }` syntax throughtout the observer
::: browser/components/extensions/ext-bookmarks.js:115
(Diff revision 4)
> + bookmark.url = uri.spec;
> + } else {
> + bookmark.dateGroupModified = bookmark.dateAdded;
> + }
> +
> + this.emit("created", {guid, bookmark});
`guid` is also available from `bookmark.id`, why not just pass `bookmark` (and let the EventManager pass the redundant args to any event listeners)
::: browser/components/extensions/ext-bookmarks.js:172
(Diff revision 4)
> +
> + this.emit("changed", {guid, info});
> + }
> + };
> + EventEmitter.decorate(observer);
> + PlacesUtils.bookmarks.addObserver(observer, false);
Can you remove the observer if nothing is actually listening to events?
Attachment #8782013 -
Flags: review?(aswan)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
https://reviewboard.mozilla.org/r/72304/#review89916
> Can you remove the observer if nothing is actually listening to events?
Where would I do this? I am creating the observer in `getObserver` if it doesn't exist, but I cannot check in getObserver to see if there are any listeners because I may be just about to add a listener. Would I need to check in each API event in the `return () => {` function after toggling the listener off? If so, wouldn't that also mean that if an extension is adding and removing listeners that it might potentially cause the observer to be created and destroyed over and over again? Is the overhead of that better than just keeping one single observer around?
Assignee | ||
Comment 41•8 years ago
|
||
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
I think when I pushed an update to MozReview it cleared your r?, Marco. I had flagged you to review the changes to Bookmarks.jsm. These are changes I inherited with this bug, so please let me know if they look okay.
Attachment #8782013 -
Flags: review?(aswan) → review?(mak77)
Comment 42•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
https://reviewboard.mozilla.org/r/72304/#review89916
> Where would I do this? I am creating the observer in `getObserver` if it doesn't exist, but I cannot check in getObserver to see if there are any listeners because I may be just about to add a listener. Would I need to check in each API event in the `return () => {` function after toggling the listener off? If so, wouldn't that also mean that if an extension is adding and removing listeners that it might potentially cause the observer to be created and destroyed over and over again? Is the overhead of that better than just keeping one single observer around?
Yes that is how it would get implemented. Nothing in getObserver looks very expensive, but you could create the observer global one time and just add/remove it if you're concerned. This happening over and over seems unlikely, an extension would either have to repeatedly attach and detach listeners or repeatedly get disabled/enabled or uninstalled/installed. If any of those things are happening, whatever small cost there is to rebuild/reattach the observer will be quite small.
Assignee | ||
Comment 43•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
https://reviewboard.mozilla.org/r/72304/#review89916
> Yes that is how it would get implemented. Nothing in getObserver looks very expensive, but you could create the observer global one time and just add/remove it if you're concerned. This happening over and over seems unlikely, an extension would either have to repeatedly attach and detach listeners or repeatedly get disabled/enabled or uninstalled/installed. If any of those things are happening, whatever small cost there is to rebuild/reattach the observer will be quite small.
I've been trying to figure out _how_ to determine id the observer has no more listeners attached to it. From what I can tell, both `EventEmitter` and `SingletonEventManager` have internal state which could tell us this, but neither expose that information. Do you think it's a good idea to change one of them to expose the information, or would it be better to keep track in the bookmarks API code of how many listeners are attached and use that information to remove the observer if the listener count goes down to 0? Or maybe you had an idea different than either of those options?
Comment 44•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
https://reviewboard.mozilla.org/r/72304/#review89916
> I've been trying to figure out _how_ to determine id the observer has no more listeners attached to it. From what I can tell, both `EventEmitter` and `SingletonEventManager` have internal state which could tell us this, but neither expose that information. Do you think it's a good idea to change one of them to expose the information, or would it be better to keep track in the bookmarks API code of how many listeners are attached and use that information to remove the observer if the listener count goes down to 0? Or maybe you had an idea different than either of those options?
I don't think SingletonEventManager is the right place, since there may be many instances of SingletonEventManager all listening.
EventEmitter is a quasi-standard interface so I wouldn't favor adding new methods to it unless it was really compelling. In this case, all you need to do is keep a count of listeners somewhere in ext-bookmarks.js, it seems straightforward enough.
Comment 45•8 years ago
|
||
mozreview-review |
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
https://reviewboard.mozilla.org/r/72304/#review90836
::: toolkit/components/places/Bookmarks.jsm:193
(Diff revision 5)
> // We need the itemId to notify, though once the switch to guids is
> // complete we may stop using it.
> let uri = item.hasOwnProperty("url") ? PlacesUtils.toURI(item.url) : null;
> let itemId = yield PlacesUtils.promiseItemId(item.guid);
> +
> + // Ignore tagging related bookmarks for WebExtension observers.
In general I think we should not talk about webExtensions in Bookmarks.jsm. Even if they were added for web-ext, the nsIBookmarkObserver options can be used by everyone and we'll likely use them in frontend code.
I'd not put any comment here, at a maximum a "// Ensure to pass tagging information for the observers to skip over these notifications when needed."
::: toolkit/components/places/Bookmarks.jsm:1509
(Diff revision 5)
> let { source = Ci.nsINavBookmarksService.SOURCE_DEFAULT } = options;
> let observers = PlacesUtils.bookmarks.getObservers();
> for (let item of itemsRemoved.reverse()) {
> let uri = item.hasOwnProperty("url") ? PlacesUtils.toURI(item.url) : null;
> + // WebExtensions only want one notification for the folder that is removed,
> + // so pass true as the last parameter.
ditto
::: toolkit/components/places/Bookmarks.jsm:1514
(Diff revision 5)
> + // so pass true as the last parameter.
> notify(observers, "onItemRemoved", [ item._id, item._parentId,
> item.index, item.type, uri,
> item.guid, item.parentGuid,
> - source ]);
> + source ],
> + { isDescendantRemoval: true });
the only problem I see here, is that in the eraseEverything case the consumer won't get any notification at all.
It's not a very often used API, but it may be worth a follow-up bug.
Attachment #8782013 -
Flags: review?(mak77) → review+
Comment hidden (mozreview-request) |
Comment 47•8 years ago
|
||
mozreview-review |
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
https://reviewboard.mozilla.org/r/72304/#review91118
I assume mak reviewed the contents of Bookmarks.jsm which I'm not familiar with. r=ms on the rest.
::: browser/components/extensions/ext-bookmarks.js:373
(Diff revision 6)
> + // TODO:
> + // onChildrenReordered:
> + // onImportBegan:
> + // onImportEnded:
do we need this here? we have them in the schema and presumably also in other bugs?
::: browser/components/extensions/test/xpcshell/test_ext_bookmarks.js:47
(Diff revision 6)
>
> function expectedError() {
> browser.test.fail("Did not get expected error");
> }
>
> + function checkOnCreated(id, parentId, index, title, url, dateAdded, dataToCheck) {
[this is a style nit but feel free to take it or leave it]
I'm finding this a little clunky, that you build these arrays that end up holding every event that fires in the whole test. Could you instead, after each step in the test, verify that you received the set of events you were expecting, then clear out the arrays before the next step?
(in the event that something breaks here, I think that would actually improve usability too -- if you get an extra event at some point, the current test will fail at the end when the total array length doesn't match the expected count, but if you change it as suggested, it would fail much closer to the actual point where the bug manifests)
::: browser/components/extensions/test/xpcshell/test_ext_bookmarks.js:239
(Diff revision 6)
> browser.bookmarks.create({title: "EFF", url: "http://eff.org"}),
> browser.bookmarks.create({title: "Menu Item", url: "http://menu.org", parentId: bookmarkGuids.menuGuid}),
> browser.bookmarks.create({title: "Toolbar Item", url: "http://toolbar.org", parentId: bookmarkGuids.toolbarGuid}),
> ]);
> }).then(results => {
> + eventData.onCreated.expectedCallCount += 6;
You're not actually verifying the contents of the onCreated events here?
Attachment #8782013 -
Flags: review?(aswan) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
https://reviewboard.mozilla.org/r/72304/#review91118
Yes, he did.
> [this is a style nit but feel free to take it or leave it]
> I'm finding this a little clunky, that you build these arrays that end up holding every event that fires in the whole test. Could you instead, after each step in the test, verify that you received the set of events you were expecting, then clear out the arrays before the next step?
> (in the event that something breaks here, I think that would actually improve usability too -- if you get an extra event at some point, the current test will fail at the end when the total array length doesn't match the expected count, but if you change it as suggested, it would fail much closer to the actual point where the bug manifests)
I made a few changes to implement this more like you suggested and it definitley feels better. Thanks!
Comment 51•8 years ago
|
||
Please mark the pending issue in MozReview as resolved so this can autoland.
Keywords: checkin-needed
Assignee | ||
Comment 52•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782013 [details]
Bug 1221764 - Implement simple chrome.bookmarks events
https://reviewboard.mozilla.org/r/72304/#review90836
> the only problem I see here, is that in the eraseEverything case the consumer won't get any notification at all.
> It's not a very often used API, but it may be worth a follow-up bug.
This will only be an issue if an observer is paying attention to that `isDescendantRemoval` flag, correct? Currently it's only our ext-bookmarks.js API code that uses that, and it doesn't call `eraseEverything`, nor do we foresee any reason to add an API that would call `eraseEverything`, so I think this would not affect anything. Does that sound accurate?
Assignee | ||
Comment 53•8 years ago
|
||
Oops, I see that I never published my response to Marco's review comment, but it's at most a follow up anyway, so I've dropped it and this should be good to check in now.
Keywords: checkin-needed
Comment 54•8 years ago
|
||
(In reply to Bob Silverberg [:bsilverberg] from comment #52)
> This will only be an issue if an observer is paying attention to that
> `isDescendantRemoval` flag, correct? Currently it's only our
> ext-bookmarks.js API code that uses that, and it doesn't call
> `eraseEverything`, nor do we foresee any reason to add an API that would
> call `eraseEverything`, so I think this would not affect anything. Does that
> sound accurate?
Yes it can be a follow-up.
The fact is that even if you don't call eraseEverything, something else may do. Bookmarks restore and Sync are the only 2 things that come to my mind atm.
And when that happen, consumers of the skipDescendants option, that means also your consumers, won't get any notification about all of the folders going away. So they may keep tracking something about a folder that has been removed.
Comment 55•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b86a96cb2b6
Implement simple chrome.bookmarks events, r=aswan,mak
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 56•8 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #54)
> (In reply to Bob Silverberg [:bsilverberg] from comment #52)
> > This will only be an issue if an observer is paying attention to that
> > `isDescendantRemoval` flag, correct? Currently it's only our
> > ext-bookmarks.js API code that uses that, and it doesn't call
> > `eraseEverything`, nor do we foresee any reason to add an API that would
> > call `eraseEverything`, so I think this would not affect anything. Does that
> > sound accurate?
>
> Yes it can be a follow-up.
> The fact is that even if you don't call eraseEverything, something else may
> do. Bookmarks restore and Sync are the only 2 things that come to my mind
> atm.
> And when that happen, consumers of the skipDescendants option, that means
> also your consumers, won't get any notification about all of the folders
> going away. So they may keep tracking something about a folder that has been
> removed.
Thanks for the clarification, Marco. What exactly should the follow up bug be? I'm not sure how to describe a change to make to mitigate the problem you describe.
Comment 57•8 years ago
|
||
we need to make eraseEverything notify at least the top-level folders, that is any folder that is a direct ancestor of menu/toolbar/unfiled
Comment 58•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 59•8 years ago
|
||
I've updated the compat data for these:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks#Browser_compatibility
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/onCreated#Browser_compatibility
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/onRemoved#Browser_compatibility
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/onChanged#Browser_compatibility
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/onMoved#Browser_compatibility
Let me know if you need anything else though!
Flags: needinfo?(bob.silverberg)
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•