1081099 - Implement bookmarks notifications from Bookmarks.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We must hook up nsINavBookmarksObserver notifications to Bookmarks.jsm

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: part 1 - expose notify methods from the bookmarks service

This is just the first piece of work, exposing methods to send notifications through xpcom.

Still working on the next part.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: 8509830: part 1 - expose notify methods from the bookmarks service (v2)

I forgot to add to NS_IMPL_ISUPPORTS

second part almost done.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: part 2 - send notifications

The final part.

This also fixes a bug in eraseEverything that is erasing too much, with a test for that.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 8510343 [details] [diff] [review]
8509830: part 1 - expose notify methods from the bookmarks service (v2)

I'd really prefer exposing the observers as an array getter in nsINavBookmarks. This is too much for too little.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #4)
> Comment on attachment 8510343 [details] [diff] [review]
> 8509830: part 1 - expose notify methods from the bookmarks service (v2)
> 
> I'd really prefer exposing the observers as an array getter in
> nsINavBookmarks. This is too much for too little.

I'm not sure what you mean?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8510343 [details] [diff] [review]
8509830: part 1 - expose notify methods from the bookmarks service (v2)

I'll refactor this based on IRC discussion.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v3

I've not yet re-read this, since it's late. It seems to be working fine.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 8511390 [details] [diff] [review]
patch v3

Review of attachment 8511390 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/Bookmarks.jsm
@@ +708,5 @@
> +/**
> + * Sends a bookmarks notification through the given observers.
> + *
> + * @param observers
> + *        array of nsINavBookmarkObserver.

nsINavBookmarkObserver objects

@@ +715,5 @@
> + * @param args
> + *        array of arguments to pass to the notification.
> + */
> +function notify(observers, notification, args) {
> +  observers.forEach(obs => {

doesn't for..of work?

@@ +718,5 @@
> +function notify(observers, notification, args) {
> +  observers.forEach(obs => {
> +    try {
> +      obs[notification](...args);
> +    } catch (ex) {}

reportError?

::: toolkit/components/places/nsINavBookmarksService.idl
@@ +544,5 @@
>     */
>    void removeObserver(in nsINavBookmarkObserver observer);
>  
> +  void getObservers([optional] out unsigned long count,
> +                    [retval, array, size_is(count)] out nsINavBookmarkObserver observers);

I'm actually fine with no-documentation here, but wondering if this was done on purpose.

BTW, I see that you didn't take this to a private interface after all. I tend to agree that's an overkill.

::: toolkit/components/places/nsNavBookmarks.cpp
@@ +2676,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(_observers);
> +  *_count = 0;
> +  *_observers = nullptr;
> +  nsCOMArray<nsINavBookmarkObserver> observers;

Move this declaration after the (!mCanNotify) block.

::: toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
@@ +339,5 @@
> +            if (expectedNotifications.length == 0)
> +              throw new Error("Received more notifications than expected");
> +            let expected = expectedNotifications.shift();
> +            Assert.equal(notification.name, expected.name);
> +            Assert.equal(notification.arguments.length, expected.arguments.length)

missing ;

@@ +357,5 @@
> +        }
> +      }
> +
> +      if (name.startsWith("onItem"))
> +        return () => notifications.push({ name: name, arguments: arguments });

Array.from, or just [...arguments], and then you can use for...of above.

Actually, if you use Array.from along with its mapping function (mapping dates and URLs), you may even use deepEqual.

::: toolkit/components/places/tests/unit/test_bookmark_catobs.js
@@ +31,5 @@
> +        Assert.equal(observers.length, initialObservers.length + 1);
> +
> +        // Check the common observer is the last one.
> +        for (let i = 0; i < initialObservers.length; ++i) {
> +          Assert.equal(initialObservers[i], observers[i]);

FWIW, with a little refactoring (make initialObservers "expectedObservers", basically) you could use deepEqual here. not-so-important-at-all.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #8)
> @@ +718,5 @@
> > +function notify(observers, notification, args) {
> > +  observers.forEach(obs => {
> > +    try {
> > +      obs[notification](...args);
> > +    } catch (ex) {}
> 
> reportError?

I'm not going to reportError here cause it may be noisy and not very useful, it would be hard once you get an error to figure which observer is causing it. We can add it later if you care about that.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v4

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/fx-team/rev/e9795af95ded

Comment 12

[Phil Ringnalda (:philor)]

test_null_interfaces.js | test failed (with xpcshell return code: 0), see following log, -1 != -1, false == true

Backed out in https://hg.mozilla.org/integration/fx-team/rev/377279f4179c

Comment 13

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2014-10-25T19:43:05
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=fx-team&job_id=997138
repository: fx-team
who: philringnalda[at]gmail[dot]com
machine: talos-mtnlion-r5-092
buildname: Rev5 MacOSX Mountain Lion 10.8 fx-team opt test xpcshell
revision: e9795af95ded

TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_null_interfaces.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_null_interfaces.js | -1 != -1 - See following stack:
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_null_interfaces.js | false == true - See following stack:
Return code: 1

Comment 14

[Asaf Romano (gone)]

Hey Marco, ping me on IRC before re-landing this.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v5

added the method to the exclusions list for test_null_interfaces and some small cleanup of that test, nothing to see here.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

re-landed https://hg.mozilla.org/integration/fx-team/rev/eb681a24b755

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eb681a24b755