Closed Bug 1372936 Opened 3 years ago Closed 3 years ago

onManyFrecenciesChanged doesn't get notified for the new Bookmarks eraseEverything and remove APIs

Categories

(Toolkit :: Places, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

Discovered whilst working on another bug. When PlacesUtils.bookmarks.eraseEverything is called or when PlacesUtils.bookmarks.remove is called with a folder, then we don't notify onManyFrecenciesChanged.

The issue is that Bookmarks.jsm is calling notify, which then does:

    try {
      observer[notification](...args);
    } catch (ex) {}

However, in the above cases args is null, so this silently fails.
Comment on attachment 8877629 [details]
Bug 1372936 - onManyFrecenciesChanged doesn't get notified for the new Bookmarks eraseEverything and remove APIs.

https://reviewboard.mozilla.org/r/149076/#review153494

::: toolkit/components/places/Bookmarks.jsm:1059
(Diff revision 1)
>   *        based on the observer's preferences.
>   */
>  function notify(observers, notification, args, information = {}) {
> +  if (!args) {
> +    Cu.reportError("notify expected 'args' to be specified, setting to empty");
> +    args = [];

is there a reason to not just do args = [] and making it [optional]?
It sounds like a pretty valid case, some notifications may not have arguments.

::: toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js:31
(Diff revision 1)
> +      resolve();
> +    };
> +
> +    PlacesUtils.history.addObserver(obs);
> +  });
> +}

nit: you don't have to fix this, and could also be a good-first-bug, but it would probably be nice to expose a .promiseHistoryNotification and .promiseBookmarksNotification to PlacesTestUtils...
Attachment #8877629 - Flags: review?(mak77)
Comment on attachment 8877629 [details]
Bug 1372936 - onManyFrecenciesChanged doesn't get notified for the new Bookmarks eraseEverything and remove APIs.

https://reviewboard.mozilla.org/r/149076/#review153516

LGTM, thank you!

::: toolkit/components/places/Bookmarks.jsm:1050
(Diff revision 2)
>   *
> - * @param observers
> + * @param {Array} observers
>   *        array of nsINavBookmarkObserver objects.
> - * @param notification
> + * @param {String} notification
>   *        the notification name.
> - * @param args
> + * @param {Array} args

nit: there's an [optional] annotation usable for javadoc
Attachment #8877629 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9a955b219d5
onManyFrecenciesChanged doesn't get notified for the new Bookmarks eraseEverything and remove APIs. r=mak
https://hg.mozilla.org/mozilla-central/rev/e9a955b219d5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.