368252 - notify observers when a microsummary gets updated

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

In order to better enable extensions (and core code) to extend microsummary functionality, it would be useful to broadcast a notification (via the observer service, most likely) when a live title gets updated so that extensions can do additional work at that point.

For example, an extension could use this to flash the live title or otherwise (f.e. using "growl") bring the update to the attention of the user.

And an extension that allows users to specify refresh intervals for individual live titles might then modify the live title's expiration time from the default assigned by the microsummary service.

One question is whether it would make sense to additionally notify observers before initiating the update request (and giving them the opportunity to cancel or otherwise modify it), i.e. something like a "microsummary-before-update" notification in addition to a "microsummary-after-update" one.

Another issue is what to pass in the aSubject and someData parameters to nsIObserverService::notifyObservers.  Currently, an nsIMicrosummary has no knowledge about the live title it's being used for (if any).  So just passing it as the value of the aSubject parameter doesn't give the observer enough information.

We could additionally pass the bookmark ID as the value of someData, but that seems clunky.  Seems like we're going to have to either make nsIMicrosummary smarter about what it's being used for (bookmark label, page tab label, etc.) or pass some other object referencing both the nsIMicrosummary and the thing using it (nsIMicrosummaryConsumer?).

There may be additional notifications that it would be useful to broadcast, but let's focus this bug on this particular one (or two), which has clear utility, and use separate bugs for other potentially useful notifications.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

Here's a work in progress that makes the microsummary service notify observers when a microsummary gets updated.  This code also makes the service observe its own notifications and log the updates to the console, but that's just for the purpose of testing this functionality, not because the service should really observe its own notifications.

I use the approach of passing the bookmark ID in the someData parameter here, but as noted in the description of this bug, that's probably not the right approach.  I also call the notification "live-title-updated", but live titles are bookmark-specific, and microsummaries won't be in the future, so we'll probably want to use "microsummary" in the name.

A third issue with this patch is that it always notifies observers when a live title gets updated, even if the microsummary didn't change.  It should only notify observers when the microsummary actually changes.  Or, if there's value in knowing the last time we checked, even if the microsummary stayed the same, then we should have a separate notification for that.

(FWIW, the concept of "change" might look very different for rich content microsummaries, and we might want to keep that in mind.)

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: implements notification with nsILiveTitle subject

Here's another approach to implementing this notification.  Instead of passing the microsummary as the subject of the notification and a stringified version of the bookmark ID as the notification data, we create an object that implements a new nsILiveTitle interface and pass that object as the subject of the notification.

This way the microsummary service gives observers the actual bookmark ID (i.e. an RDF resource on the old bookmarks code, a URI in Places) instead of a stringified version of it, and it also gives observers the old live title, which will be useful in some cases (f.e. an observer that notifies users about live title updates might want to highlight what has changed).

Another change from the earlier work-in-progress patch is that the service only kicks off a notification if the live title has actually changed (per a simple string comparison).  I also changed the name of the notification for consistency with another microsummary notification (bug 368272) that starts with "microsummary-".  And I removed the testing code (I'll post it as a separate patch).

One thing that hasn't changed from the earlier patch is that the code remains specific to live title updates.  After thinking about it for a while, I think it makes the most sense to wait on generalizing these notifications until we actually start using microsummaries in some other context.

After all, the update code we're plugging into is all live title-specific (even many of the functions with "microsummary" in their title are really about live titles, not microsummaries generally), and it's hard to predict a priori the notification needs of code that uses microsummaries in some other way.

Seth and Mike: I'm keen to get your feedback on the API design here, particularly the nsILiveTitle interface and whether it's the right way to be passing to observers the information they'll want about live title updates.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: testing code for patch v1

This is code to help test patch v1.  It simply makes the microsummary service observe its own notifications about live titles being updated and dump a message to the console ("observed live title update for...") whenever it receives one.

Comment 4

[(not reading, please use [e-mail] instead)]

myk, sorry for the delay.

overall, I like what you are doing here.  

I do have a couple suggestions for the interface and a few other suggestions.

here are my review comments:

1)

+[scriptable, uuid(f9e577a8-19d9-4ca0-a140-b9e43f014470)]
+interface nsILiveTitle : nsISupports
+{

what do you think of renaming this this interface something like nsILiveTitleSubject or nsILiveTitleNotificationSubject?

It's not really a "live title", is the subect we pass to notifyObservers, right?

2)

+    // The ID of the bookmark displaying this title.
+    // Note: in the old bookmarks code, this is an RDF resource.  In Places
+    // it is currently a URI, but after the fix for bug 360133 lands it will
+    // become an integer.
+    readonly attribute nsISupports bookmarkID;

when you land, could log a spin off bug (that depends on #360133) to come back and fix this comment?

Also, when places is on by default, I think we should update this interface to use a PRInt64 instead of nsISupports. 

Perhaps another spin off bug on that?

3)

+    // Get the old title from the datastore to see if it's actually changed.

this comment seems RDF specific.  Can you make it something like:

+    // Get the old title and see if it's actually changed.

4)

+    var bookmarkIdentity = 
+#ifdef MOZ_PLACES_BOOKMARKS
+      bookmarkID.spec;
+#else
+      bookmarkID.Value + " (" + microsummary.pageURI.spec + ")";
+#endif

Like you noted in your interface (comment #2 above), this "bookmarkID.spec" also deserves a comment indicating that
when #360133 lands, this will need to be fixed.  please note this in the spin off bug.

5)

+    if (typeof oldValue != "undefined" && oldValue != microsummary.content) {

would this also work?

if (oldValue && oldValue != microsummary.content) {

6)

+      this._setField(bookmarkID, FIELD_GENERATED_TITLE, microsummary.content);
+      var liveTitle = new LiveTitle(bookmarkID, microsummary, oldValue);
+      LOG("updated live title for " + bookmarkIdentity +
+          " from '" + liveTitle.oldValue + "' to '" + microsummary.content + "'");
+      this._obs.notifyObservers(liveTitle, "microsummary-livetitle-updated", null);
+    }
+    else {
+      LOG("didn't update live title for " + bookmarkIdentity + "; it hasn't changed");
+    }

Perhaps we should not have oldValue as part of the nsILiveTitle(NotificationSubject) interface, 
and instead pass that as the "someData" to notifyObservers() as the third param, instead of null.  I think that would be cleaner.  What do you think?

7) 
 
+#ifdef MOZ_PLACES_BOOKMARKS
+            liveTitle.bookmarkID.spec + " " +

same as comment #4 above, please add a comment and note this in the spin off bug that depends on bug #360133

Comment 5

[(not reading, please use [e-mail] instead)]

Comment on attachment 254349 [details] [diff] [review]
patch v1: implements notification with nsILiveTitle subject

clearing the review request

Comment 6

[(not reading, please use [e-mail] instead)]

getting dietrich in the fun, as we refer to his places URI -> ID bug.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)

> 1)
> 
> +[scriptable, uuid(f9e577a8-19d9-4ca0-a140-b9e43f014470)]
> +interface nsILiveTitle : nsISupports
> +{
> 
> what do you think of renaming this this interface something like
> nsILiveTitleSubject or nsILiveTitleNotificationSubject?

This seems reasonable.


> It's not really a "live title", is the subect we pass to notifyObservers,
> right?

That's a good question.  It is the subject we pass to notifyObservers, but it does contain the crucial bits of information that define a live title, namely a microsummary and a reference to the bookmark to which it applies.

It doesn't, however, include stuff like a set of references to UI elements representing the bookmark and displaying its live title, which we might want to include in an interface to generic live title objects (not sure about this).

And the old value of the title seems specific to this notification and not something we would include in an interface to generic objects.

So I think it's fine to name this something notification-specific for the time being and change it later if/when we start needing generic live title objects.


> +    // The ID of the bookmark displaying this title.
> +    // Note: in the old bookmarks code, this is an RDF resource.  In Places
> +    // it is currently a URI, but after the fix for bug 360133 lands it will
> +    // become an integer.
> +    readonly attribute nsISupports bookmarkID;
> 
> when you land, could log a spin off bug (that depends on #360133) to come back
> and fix this comment?

Yup.  I've filed bug 370215 on it already, since there's existing microsummaries code that uses URIs to identify bookmarks.


> Also, when places is on by default, I think we should update this interface to
> use a PRInt64 instead of nsISupports. 
> 
> Perhaps another spin off bug on that?

Yes, good point.  I have filed this one as bug 370216.


> 3)
> 
> +    // Get the old title from the datastore to see if it's actually changed.
> 
> this comment seems RDF specific.  Can you make it something like:
> 
> +    // Get the old title and see if it's actually changed.

I'm actually using the word "datastore" here (and in other microsummaries code) to generically mean both "datasource" in RDF-based bookmarks and "database" in Places-based bookmarks.

But looks like that can be misleading, and it's not critical to specify that we're getting the old title from the datastore (after all, where else would we get it from), so I'll remove that bit.


> 4)
> 
> +    var bookmarkIdentity = 
> +#ifdef MOZ_PLACES_BOOKMARKS
> +      bookmarkID.spec;
> +#else
> +      bookmarkID.Value + " (" + microsummary.pageURI.spec + ")";
> +#endif
> 
> Like you noted in your interface (comment #2 above), this "bookmarkID.spec"
> also deserves a comment indicating that
> when #360133 lands, this will need to be fixed.  please note this in the spin
> off bug.

Yup, I have noted it in the bug, and I also filed bug 370218 about ripping out all the #ifndef MOZ_PLACES_BOOKMARKS (and #ifdef MOZ_PLACES_BOOKMARKS ... #else) from the microsummaries code generally, as we won't need that anymore once Places-based bookmarks are building by default.


> 5)
> 
> +    if (typeof oldValue != "undefined" && oldValue != microsummary.content) {
> 
> would this also work?
> 
> if (oldValue && oldValue != microsummary.content) {

I thought I should use typeof to differentiate between "doesn't have an old value" and "has an old value which is the empty string and thus evaluates to false".

But now that I think about it, if a microsummary doesn't have an old value, and it's just been given one, observers will probably want to see that as a change from the empty string to the new value, so it would make sense to set oldValue to the empty string if we can't pull it from the datastore and then use the simpler conditional construct you suggest.


> 6)
> 
> +      this._setField(bookmarkID, FIELD_GENERATED_TITLE, microsummary.content);
> +      var liveTitle = new LiveTitle(bookmarkID, microsummary, oldValue);
> +      LOG("updated live title for " + bookmarkIdentity +
> +          " from '" + liveTitle.oldValue + "' to '" + microsummary.content +
> "'");
> +      this._obs.notifyObservers(liveTitle, "microsummary-livetitle-updated",
> null);
> +    }
> +    else {
> +      LOG("didn't update live title for " + bookmarkIdentity + "; it hasn't
> changed");
> +    }
> 
> Perhaps we should not have oldValue as part of the
> nsILiveTitle(NotificationSubject) interface, 
> and instead pass that as the "someData" to notifyObservers() as the third
> param, instead of null.  I think that would be cleaner.  What do you think?

My original patch used someData for the bookmark ID, but that seemed wrong, since it required serializing the nsIRDFResource/nsIURI bookmark ID to a string equivalent.  oldValue, however, is indubitably a string, and it doesn't quite fit into the nsILiveTitle(NotificationSubject) interface very well anyway, so putting it in someData, which is a wstring, seems like a great idea.


> 7) 
> 
> +#ifdef MOZ_PLACES_BOOKMARKS
> +            liveTitle.bookmarkID.spec + " " +
> 
> same as comment #4 above, please add a comment and note this in the spin off
> bug that depends on bug #360133

Yup, duly noted.

Thanks for the thorough review!  I'll have a new patch up shortly.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses review comments

Here's a new version of the patch with the following changes:

1. Changed the name of the interface of the subject we pass to observers from nsILiveTitle to nsILiveTitleNotificationSubject.  My only concern is the length of this name, but it's accurate, which is the most important thing.

2. Removed "from the datastore" from the comment about getting the old title.

3. Made the old value be the empty string when there's no current live title.  Since that means oldValue is always defined, the conditional can be simple:

    if (oldValue != microsummary.content) {

Part of me thinks we should actually let the old value be null when there's no current live title and let the observer make this conversion.  Then observers that care can differentiate between "didn't have an old title" and "had an old title that happened to be the empty string," while observers that don't care can just convert a null value for the someData parameter to the empty string themselves.

If we did this, we'd just have to make the conditional be:

    if (!oldValue || oldValue != microsummary.content) {

(i.e. either there was no old title or the old title doesn't match the new one)

4. Removed oldValue from the nsILiveTitleNotificationSubject interface in favor of passing it as the value of the someData parameter.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: testing code for patch v2

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 254878 [details] [diff] [review]
patch v2: addresses review comments

r=sspitzer, thanks for making all those changes.

I agree nsILiveTitleNotificationSubject, but it is accurate (and the code "comments itself" as dietrich likes to say)

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Seth, thanks for the review!

One question: what are your thoughts on converting a null oldValue to the empty string vs. leaving it null?  I converted it in the latest patch, but I'm starting to think it would make more sense to leave it null and let the observers do the conversion, even if it means a bit more work for observers that don't care about the difference between "didn't have a live title" and "had a blank live title".

What do you think?

Comment 12

[(not reading, please use [e-mail] instead)]

myk, I think you are right, we should leave it null and push the work into the observer.  if you want to fix this and fix the test code, I'll glady (and promptly) re-review.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: leaves oldValue null

(In reply to comment #12)
> myk, I think you are right, we should leave it null and push the work into the
> observer.  if you want to fix this and fix the test code, I'll glady (and
> promptly) re-review.

Here's an updated patch.  I ended up making the conditional be:

    if (oldValue == null || oldValue != microsummary.content) {

instead of:

    if (!oldValue || oldValue != microsummary.content) {

because the latter triggers an update if oldValue is the empty string (which evaluates to false in boolean context), even if the new title is also the empty string and thus the title hasn't changed.

The former conditional, on the other hand, only triggers an update if oldValue == null (i.e. there was no live title to begin with, so any live title, even the empty string, is a change) or if the old title doesn't match the new one.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: testing code for patch v3

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 254898 [details] [diff] [review]
patch v3: leaves oldValue null

r=sspitzer, thanks myk!

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 254898 [details] [diff] [review]
patch v3: leaves oldValue null

sr=mconnor

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Patch checked in to trunk:

Checking in browser/components/microsummaries/public/nsIMicrosummaryService.idl;
/cvsroot/mozilla/browser/components/microsummaries/public/nsIMicrosummaryService.idl,v  <--  nsIMicrosummaryService.idl
new revision: 1.8; previous revision: 1.7
done
Checking in browser/components/microsummaries/src/nsMicrosummaryService.js;
/cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js,v  <--  nsMicrosummaryService.js
new revision: 1.61; previous revision: 1.60
done