Closed Bug 1221764 Opened 9 years ago Closed 8 years ago

Implement simple chrome.bookmarks events

Categories

(WebExtensions :: General, defect, P3)

defect

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)

Attached patch v1 - chrome.bookmark events (obsolete) — Splinter Review
I implemented onCreated, onRemoved, onChanged and onMoved still missing are onChildrenReordered, onImportBegan and onImportEnded.

I am going to look at adding some tests.
Attached patch v1 - chrome.bookmarks events (obsolete) — Splinter Review
Assignee: nobody → evilpies
Attachment #8683337 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8683339 - Flags: review?(wmccloskey)
Attachment #8683339 - Flags: review?(mak77)
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 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+
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.
(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.
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?
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.
fwiw, we could implement skipping also in the old cpp implementation and it should work
Attached patch WIP chrome.bookmarks (obsolete) — Splinter Review
I now started checking the webextension flag in nsNavBookmarks.jsm. However there are still tons of macro invocations that need to be verified.
Blocks: 1251269
No longer blocks: 1251269
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)
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)
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)
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)
Attached patch v2 Bookmarks (obsolete) — Splinter Review
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)
Attachment #8683339 - Attachment is obsolete: true
Attachment #8709128 - Attachment is obsolete: true
Attachment #8730126 - Attachment is patch: true
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)
Priority: -- → P1
Whiteboard: [bookmarks] → [bookmarks]triaged
[Tracking Requested - why for this release]:
Tracking for 49 - Andy will this need a release note to be added?
Flags: needinfo?(amckay)
I am currently rebasing my patch for this and applying Marco's 2 month old feedback. Sorry for not getting back to this sooner.
Attached patch v3 Bookmarks (obsolete) — Splinter Review
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 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)
This isn't going to make 49 and no, it doesn't need release notes.
Flags: needinfo?(amckay)
Assignee: evilpies → nobody
Status: ASSIGNED → NEW
Bob, as our resident bookmarks expert, would you mind picking this up?
Flags: needinfo?(bob.silverberg)
Sure, I'll add it to my list.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Flags: needinfo?(bob.silverberg)
Priority: P1 → P3
Comment on attachment 8730126 [details] [diff] [review]
v2 Bookmarks

I believe that v3 obsoletes this patch.
Attachment #8730126 - Attachment is obsolete: true
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
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)
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)
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)
Okay, I have a faster computer now so compiling would be quicker.
Flags: needinfo?(evilpies)
Great, thanks Tom. I will spin off another bug and attach a patch to it with just those changes.
Depends on: 1296401
Flags: needinfo?(kmaglione+bmo)
hrm.  That last attachment has the bug # here, but it was meant for bug 1296401.
Attachment #8797323 - Attachment is obsolete: true
Attachment #8797323 - Flags: review?(markh)
Component: WebExtensions: Untriaged → WebExtensions: General
Attachment #8782013 - Flags: review?(mak77)
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 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 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?
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)
Depends on: 1315008
Depends on: 1315009
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.
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 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 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 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 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!
Try looks good. Let's try landing this.
Keywords: checkin-needed
Please mark the pending issue in MozReview as resolved so this can autoland.
Keywords: checkin-needed
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?
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
(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.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b86a96cb2b6
Implement simple chrome.bookmarks events, r=aswan,mak
Keywords: checkin-needed
No longer depends on: 1315008, 1315009
Blocks: 1316297
(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.
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
https://hg.mozilla.org/mozilla-central/rev/8b86a96cb2b6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
No longer blocks: 1316297
Looks good, thanks Will!
Flags: needinfo?(bob.silverberg)
Depends on: 1362863
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.