531830 - lightweight-theme-changed notification should pass a stringified theme

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, defect)

Description

[Mike Kaply [:mkaply]]

Before the change in bug 529769, lightweight-theme-changed passed a JSON version of the selected theme.

This was removed in that bug.

It needs to be put back.

Removing that parameter broke compatibility with the old notifications.

I already have six extensions out that this breaks.

patch is 

_observerService.notifyObservers(null, "lightweight-theme-changed", null);

to

_observerService.notifyObservers(null, "lightweight-theme-changed", JSON.stringify(aData));

in LightweightThemeManager.jsm

Comment 1

[Mike Kaply [:mkaply]]

This should block 3.6.

Since the previous fix didn't make it into beta 4, extension developers haven't seen the new code yet, and changing this will have better compatibility with the old way.

Comment 2

[Dão Gottwald [:dao]]

The old lightweight-theme-changed (now lightweight-theme-styling-update) was intended to be used internally to tell each application window to update its styling. Because of that very specific use case, we pass the data that we know will be needed. If you want the same behavior, observe lightweight-theme-styling-update.

The new lightweight-theme-changed is a general-purpose notification. When observing that, use the LightweightThemeManager module to get the data you need (currentTheme, currentThemeForDisplay, usedThemes or whatever).

Comment 3

[Mike Kaply [:mkaply]]

When the change for bug 529769 was put in, I indicated that I already had extensions out in the wild that used the changed behavior. The only way they will work with the new behavior is if "changed" uses a JSON string.

At the very least, "changed" should pass "null" instead of null.

I specifically wrote my code following your directions as to how "changed" would work. I accept that the notifications are different, but there is absolutely no reason not to pass the JSON theme as a parameter to changed.

Why require someone to go to the theme manage when you can tell them exactly what happened?

Comment 4

[Mike Kaply [:mkaply]]

In addition, I'm releasing extensions now. I can't wait for GA. And they have to work on the beta and the GA. You've created a situation that is difficult to detect, where as passing JSON on the "changed" message allows addons that use the old behavior to work unmodified.

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #3)
> At the very least, "changed" should pass "null" instead of null.

That would in fact be wrong, as the intention is to pass no data rather than some JSON object whose value happens to be null.

> I specifically wrote my code following your directions as to how "changed"
> would work.

... where I also said that it's used internally to style the windows. Again, you may use lightweight-theme-styling-update if that's the behavior you want.

> I accept that the notifications are different, but there is
> absolutely no reason not to pass the JSON theme as a parameter to changed.
> 
> Why require someone to go to the theme manage when you can tell them exactly
> what happened?

Because we don't know what the consumer is going to do. We could pass all the theme manager's data in a big stringified object, but letting the consumer ask the manager for the data it wants is obviously simpler.

(In reply to comment #4)
> In addition, I'm releasing extensions now. I can't wait for GA. And they have
> to work on the beta and the GA.

Using the module will work in the beta as well as in the RC.

Comment 6

[Dave Townsend [:mossop]]

I agree that we should pass the theme with the notification both for simplicity and consistency. It is extremely unlikely that someone will be observing this and not want to know details of the theme when it changes.

However since there is an easy workaround we can't justify blocking on it. I'd like to get a patch on trunk but we are probably too late to fix this on 1.9.2 now.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> I agree that we should pass the theme with the notification both for simplicity
> and consistency. It is extremely unlikely that someone will be observing this
> and not want to know details of the theme when it changes.

Well, some details. It's not clear whether the URLs of the original images or the cached ones would be preferred, for instance. (Michael would prefer the latter, according to the case he told me about.)

Comment 8

[Mike Kaply [:mkaply]]

API changes are not supposed to be made once the beta started, especially this
late in the cycle.

Bug 529769 broke those rules and I didn't fight it because it was going to be
done in a way that didn't break existing users of the API.

This aspect of that change was missed and existing users of the API are broke.

This new API has not been released at all. It should be fixed before we release
it.

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #7)
> (In reply to comment #6)
> > I agree that we should pass the theme with the notification both for simplicity
> > and consistency. It is extremely unlikely that someone will be observing this
> > and not want to know details of the theme when it changes.
> 
> Well, some details. It's not clear whether the URLs of the original images or
> the cached ones would be preferred, for instance. (Michael would prefer the
> latter, according to the case he told me about.)

Well neither passing the theme with the notification, nor retrieving it from the LightweightThemeManager while in the notification will give you the cached images since they haven't finished downloading at that point. If we needed a notification for when the theme had actually been fully downloaded then we'd need to add it as something new I think.

(In reply to comment #8)
> API changes are not supposed to be made once the beta started, especially this
> late in the cycle.
> 
> Bug 529769 broke those rules and I didn't fight it because it was going to be
> done in a way that didn't break existing users of the API.
> 
> This aspect of that change was missed and existing users of the API are broke.
> 
> This new API has not been released at all. It should be fixed before we release
> it.

We break APIs sparingly in the beta cycle when there is an obvious problem that needs to be fixed. The previous change was taken under protest by the drivers, if you want to provide a patch then I'll review it but I fully expect the drivers would not approve it for a branch landing.

Comment 10

[Mike Kaply [:mkaply]]

Attachment: Pass JSON theme on lightweight-theme-changed

Instead of passing null for lightweight-theme-changed, pass JSON theme, consistent with previous defined behavior

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 415410 [details] [diff] [review]
Pass JSON theme on lightweight-theme-changed

r+ but please correct the double space

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I agree that we should pass the theme with the notification both for simplicity
> > > and consistency. It is extremely unlikely that someone will be observing this
> > > and not want to know details of the theme when it changes.
> > 
> > Well, some details. It's not clear whether the URLs of the original images or
> > the cached ones would be preferred, for instance. (Michael would prefer the
> > latter, according to the case he told me about.)
> 
> Well neither passing the theme with the notification, nor retrieving it from
> the LightweightThemeManager while in the notification will give you the cached
> images since they haven't finished downloading at that point.

That's only the case because I chose to only cache one theme at a time. This could be changed at any time.

> If we needed a
> notification for when the theme had actually been fully downloaded then we'd
> need to add it as something new I think.

No, Michael's case was about what's actually used for the styling.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 415410 [details] [diff] [review]
Pass JSON theme on lightweight-theme-changed

This doesn't improve the API but will potentially cause confusion in the long run.

Comment 14

[Mike Kaply [:mkaply]]

(In reply to comment #13)
> This doesn't improve the API but will potentially cause confusion in the long
> run.

Can you explain this please? what's the different between querying currentTheme and using the data that is available in the JSON directly?

Comment 15

[Dão Gottwald [:dao]]

See comment 12... you should be using currentThemeForDisplay.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> See comment 12... you should be using currentThemeForDisplay.

But using currentThemeForDisplay during this notification will give exactly the same data right now yes? And should we choose to start caching multiple themes then we can still pass the cached version as a part of this notification.

It is pretty standard practice to pass information about the thing that has changed as a part of notification that something has changed.

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> (In reply to comment #15)
> > See comment 12... you should be using currentThemeForDisplay.
> 
> But using currentThemeForDisplay during this notification will give exactly the
> same data right now yes?

Right now yes. But that API is forward compatible whereas passing some data isn't.

> And should we choose to start caching multiple themes
> then we can still pass the cached version as a part of this notification.

This assumes that every consumer would want the cached version, but my point is that there are legitimate uses for either version, and thus the explicit properties should be used.

Comment 18

[Mike Kaply [:mkaply]]

Where is all this documented?

I understand that you have some vision of how all this stuff works, but extension developers are using this now and the only thing we have any documentation on is the notifications.

So of course we're expecting the notifications to have useful data in them because they are the only things we know about.

Comment 19

[Dão Gottwald [:dao]]

Please read the LightweightThemeManager declaration in <resource://gre/modules/LightweightThemeManager.jsm>. This should be fairly straightforward. If there are questions, please ask. There's no explicit API and no documentation currently.

Comment 20

[Eric Shepherd [:sheppy]]

https://developer.mozilla.org/en/Themes/Lightweight_themes has been around for some time. It largely defers to the existing documentation on http://www.getpersonas.com but does link to it.

Comment 21

[Eric Shepherd [:sheppy]]

OK, clearly there's a lot more going on here than what I thought. I'm more than a little concerned by the total lack of comments in LightweightThemeManager.jsm though. I'll try to find time to write this up in more detail.

Some notes for myself from mkaply:

Limit of 8 lightweight themes installed. An extension can change themes by setting currentTheme on the LightweightThemeManager. Themes are stored as JSON.

Comment 22

[Mike Kaply [:mkaply]]

I'm WONTFIXing this. This ship has sailed, and I don't think anyone uses the lightweight theme manager API beside me and Personas Plus.