368272 - Notify observers upon generator installation

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Possible patch

In the same spirit of bug 368252, it would be great if notifications of generator installs were broadcast to observers.
This patch adds a "generator-installed" topic with the new generator as the subject and its file name (might be better as a file URI?) as the data.

We might also want a "generator-updated" topic to cover updates and generators installed via reinstallMissingGenerator().

Comment 1

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

Comment on attachment 252876 [details] [diff] [review]
Possible patch

> In the same spirit of bug 368252, it would be great if notifications
> of generator installs were broadcast to observers.

Hmm, indeed, this does seem like it could be useful.  Does this make it easier to code something in the Microsummary Manager extension, or did you have another purpose in mind?


> This patch adds a "generator-installed" topic

I think it would make sense to prefix microsummary-related topics with the string "microsummary" to distinguish them from other notifications, so this should be "microsummary-generator-installed".


> with the new generator as the subject and its file name
> (might be better as a file URI?) as the data.

The generator stores its file's URI in its localURI property.  If that would be useful to observers, then we should just expose that property through the nsIMicrosummaryGenerator interface.


> We might also want a "generator-updated" topic to cover updates and 
> generators installed via reinstallMissingGenerator().

Right, good point.

Comment 2

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

Comment on attachment 252876 [details] [diff] [review]
Possible patch

r- pending resolution of issues raised in previous comment.  Looking forward to the next patch!

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

>Does this make it easier to code something in the Microsummary Manager extension,
>or did you have another purpose in mind?
Yep, this'll allow me to clean up how the listbox is redrawn when a generator is installed from the add-ons manager and allow me to redraw when a generator's installed via the sidebar or otherwise with the manager open.

This patch adds the additional 'updated' notification and prefixes them both with 'microsummary-'. Generators installed from reinstallMissingGenerator() and user reinstalls/updates will also fire the 'installed' and 'updated' topics respectively.

Comment 4

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

Comment on attachment 253851 [details] [diff] [review]
Patch

Looks good, just one more thing: instead of passing the file name to observers via the "data" parameters, simply expose the existing "localURI" property of MicrosummaryGenerator objects as a read-only nsIURI attribute of nsIMicrosummaryObserver (bumping the UUID for that interface in the process).

Then observers will be able to access that value by QIing generator.localURI to nsIFileURL, getting its "file" attribute, and accessing the file's "leafName" attribute.

Comment 5

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v2

Expose the generator's localURI attribute, rev the interface's UUID and pass null via the data param.

Comment 6

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

Comment on attachment 254620 [details] [diff] [review]
Patch v2

Looks great, r=myk.

Ryan, do you have CVS access, or do you need someone to check this in?

Comment 7

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

Attachment: patch for testing functionality

FWIW, here's a patch for testing this functionality.  It just makes the microsummary service observe its own microsummary-generater-installed/updated notifications and log a message to the console when it observes one.

Comment 8

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

Patch checked in to the trunk:

Checking in public/nsIMicrosummaryService.idl;
/cvsroot/mozilla/browser/components/microsummaries/public/nsIMicrosummaryService.idl,v  <--  nsIMicrosummaryService.idl
new revision: 1.6; previous revision: 1.5
done
Checking in src/nsMicrosummaryService.js;
/cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js,v  <--  nsMicrosummaryService.js
new revision: 1.53; previous revision: 1.52
done

Thanks for the fix!

Comment 9

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Don't notify until after _saveGeneratorXML() is called

The previous patch notified observers before the call to _saveGeneratorXML() resulting in a bit of a race condition (doh!). This patch moves the notification to a more appropriate location.

Comment 10

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Real patch

Might help to attach the right file...

Comment 11

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

Comment on attachment 256184 [details] [diff] [review]
Real patch

Yup, this makes sense, and the patch looks good, r=myk

Comment 12

[Phil Ringnalda (:philor)]

browser/components/microsummaries/src/nsMicrosummaryService.js 1.56