lightweight-theme-changed notification should pass a stringified theme

RESOLVED WONTFIX

Status

()

Toolkit
Add-ons Manager
--
minor
RESOLVED WONTFIX
9 years ago
2 years ago

People

(Reporter: mkaply, Unassigned)

Tracking

unspecified
Points:
---
Bug Flags:
blocking1.9.2 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: docs: see comment 18 and up)

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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
(Reporter)

Comment 1

9 years ago
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.
Flags: blocking-firefox3.6?
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).
Component: Theme → Add-ons Manager
Flags: blocking-firefox3.6?
Product: Firefox → Toolkit
QA Contact: theme → add-ons.manager
Version: 3.6 Branch → unspecified

Updated

9 years ago
Severity: major → minor
(Reporter)

Comment 3

9 years ago
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?
(Reporter)

Comment 4

9 years ago
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.
(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.
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.
Flags: blocking1.9.2-
(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.)
(Reporter)

Comment 8

9 years ago
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.
(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.
(Reporter)

Comment 10

9 years ago
Created attachment 415410 [details] [diff] [review]
Pass JSON theme on lightweight-theme-changed

Instead of passing null for lightweight-theme-changed, pass JSON theme, consistent with previous defined behavior
Attachment #415410 - Flags: review?
Attachment #415410 - Flags: approval1.9.2?
Comment on attachment 415410 [details] [diff] [review]
Pass JSON theme on lightweight-theme-changed

r+ but please correct the double space
Attachment #415410 - Flags: review? → review+
(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 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.
Attachment #415410 - Flags: review-
(Reporter)

Comment 14

9 years ago
(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?
See comment 12... you should be using currentThemeForDisplay.
(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.
(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.
(Reporter)

Comment 18

9 years ago
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.
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.
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.
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.
Keywords: dev-doc-needed
Whiteboard: docs: see comment 18 and up
(Reporter)

Comment 22

2 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.