Closed Bug 1081099 Opened 5 years ago Closed 5 years ago

Implement bookmarks notifications from Bookmarks.jsm

Categories

(Toolkit :: Places, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
mozilla36
Iteration:
36.1

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

We must hook up nsINavBookmarksObserver notifications to Bookmarks.jsm
Flags: in-testsuite?
Flags: firefox-backlog+
Points: --- → 5
Flags: qe-verify-
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 36.1
This is just the first piece of work, exposing methods to send notifications through xpcom.

Still working on the next part.
Attachment #8509830 - Flags: review?(mano)
I forgot to add to NS_IMPL_ISUPPORTS

second part almost done.
Attachment #8509830 - Attachment is obsolete: true
Attachment #8509830 - Flags: review?(mano)
Attachment #8510343 - Flags: review?(mano)
Attached patch part 2 - send notifications (obsolete) — Splinter Review
The final part.

This also fixes a bug in eraseEverything that is erasing too much, with a test for that.
Attachment #8510495 - Flags: review?(mano)
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.
(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 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.
Attachment #8510343 - Flags: review?(mano)
Attachment #8510495 - Flags: review?(mano)
Attached patch patch v3 (obsolete) — Splinter Review
I've not yet re-read this, since it's late. It seems to be working fine.
Attachment #8510343 - Attachment is obsolete: true
Attachment #8510495 - Attachment is obsolete: true
Attachment #8511390 - Flags: review?(mano)
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.
Attachment #8511390 - Flags: review?(mano) → review+
(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.
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #8511390 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/e9795af95ded
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla36
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
Hey Marco, ping me on IRC before re-landing this.
Attached patch patch v5Splinter Review
added the method to the exclusions list for test_null_interfaces and some small cleanup of that test, nothing to see here.
Attachment #8511538 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/eb681a24b755
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.