529769 - notify observers when lightweight theme is selected

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

For the purposes of extending the core lightweight theme functionality in Firefox, it would be very useful for there to be a notification when a lightweight theme is selected in addition to when it changes, since the "changed" notification happens both when lightweight themes are previewed and when they are selected, but extensions (like Personas) want to distinguish between previews and selections.

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

[Mike Kaply [:mkaply]]

Dao, does this give me what I asked for in that previous bug? The changed notification now is only used to notify outside sources, not internal sources?

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #0)
> For the purposes of extending the core lightweight theme functionality in
> Firefox, it would be very useful for there to be a notification when a
> lightweight theme is selected in addition to when it changes, since the
> "changed" notification happens both when lightweight themes are previewed and
> when they are selected, but extensions (like Personas) want to distinguish
> between previews and selections.

This affects the add-ons manager too. It observes lightweight-theme-changed but doesn't really care about previews there.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 413595 [details] [diff] [review]
patch

Huh, I could've sworn we only sent the notification when actually changing.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 413595 [details] [diff] [review]
patch

Why are we changing the name of the observer? Won't that cause add-on compatibility problems?

Comment 7

[Mike Kaply [:mkaply]]

I doubt there are many extensions using this, since there isn't any documentation on the new observer messages yet (that I could find), although I'm using it for Brand Thunder 3.6 compatibility.

But that's a good question. Why don't we leave the old one in place and create a name for the new one?

Then we're keeping the old name and simply adding a new notification?

I realize that the naming will be wrong/inconsistent, but since we are supposed to be API frozen for extensions at this point, it would be a more correct change.

Comment 8

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

The name change does break the latest Personas development builds, which observe the current notification, but I'm ok with that, since we plan to update Personas to use the new notification when it becomes available anyway (bug 530146), given that it's so much more reliable than how we currently detect persona changes.

I personally think we're better off making the name change now than forever holding our peace, and while it might break some extensions (if there are any besides Personas that are observing the notification), it might just as easily fix others that treat the notification as if it really did what it says it does (i.e. the other way to look at this is that it is a bug fix, not an API change, that makes the API actually do what extension developers expect from it).

Comment 9

[Mike Kaply [:mkaply]]

It should be OK with me to. We have six extensions out so far that use this and I think they will work correctly - my only concern is that preview will work incorrectly now. I won't really know until this fix drops.

Comment 10

[Dave Townsend [:mossop]]

I'd concur that we we should take this change now, but lets get some kind of mention of it out there

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 413595 [details] [diff] [review]
patch

a192=beltzner, against my better judgement.

Please do make sure that you blog the hell out of this change. And Myk, please make sure that by the time we launch Firefox 3.6 there's a compatible version of Personas out there.

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/21e65793a3cc

Comment 13

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

(In reply to comment #11)
> And Myk, please make sure that by the time we launch Firefox 3.6 there's a
> compatible version of Personas out there.

Roger that. Shirley, I'll do so.  To wit, Personas compatibility with Firefox 3.6 is being tracked in bug 521913, whose dependencies are all actively being worked on, and we're planning to push a release candidate for Personas 1.5 (with all known compatibility issues resolved) this week.

Comment 14

[Eric Shepherd [:sheppy]]

Added documentation:

https://developer.mozilla.org/en/Observer_Notifications#Themes
https://developer.mozilla.org/en/Themes/Lightweight_themes#Lightweight_theme_notifications

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/55dc2dfb8b91

Comment 16

[Samuel Sidler (old account; do not CC)]

These bugs landed after b4 was cut. Moving flag out.

Comment 17

[Mike Kaply [:mkaply]]

(In reply to comment #16)
> These bugs landed after b4 was cut. Moving flag out.

That's a real bummer. That means extension developers won't be able to test with this before final. Ouch.

Comment 18

[Mike Kaply [:mkaply]]

This needs one more change. In order to maintain compatibility with previous consumers, the changed message needs to pass in stringified theme data.

Comment 19

[Tony Chung [:tchung]]

anyway this fix can be tested manually?  Is there a way i can distinguish the notification actions are updating in the json data?

Comment 20

[Dão Gottwald [:dao]]

The add-ons manager uses this notification... If you open the add-ons manager and select a theme on getpersonas.com, the add-ons manager should reflect that change.