Closed Bug 335154 Opened 14 years ago Closed 13 years ago

Get SeaMonkey's themes registering and switching with Theme Manager (in suiterunner)

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(11 files, 9 obsolete files)

5.07 KB, image/png
Details
1.64 KB, image/png
Details
7.64 KB, patch
standard8
: review+
Details | Diff | Splinter Review
422 bytes, image/png
Details
2.20 KB, image/png
Details
1.27 KB, patch
kairo
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review
10.46 KB, patch
standard8
: review+
Details | Diff | Splinter Review
10.18 KB, patch
kairo
: review+
Details | Diff | Splinter Review
645 bytes, patch
neil
: review+
Details | Diff | Splinter Review
2.16 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.87 KB, patch
standard8
: review+
Details | Diff | Splinter Review
Now that we have theme manager, we need to:

- Set up the our existing themes for registration with the new theme manager
- Remove the old theme controls
The existing theme controls aren't any use in suiterunner. Therefore we should remove them in preference of the Theme Manager IMO. This patch adds in some temporary ifdefs that we can get rid of once we throw the switch.
Attachment #219520 - Flags: superreview?(neil)
Attachment #219520 - Flags: review?(neil)
This patch does the necessary to make the modern theme show up in extension manager. It actually needs some additional prefs to switch - I'll post those in a followup patch once I've looked at them in more detail.

It also needs two image files to go into suite/themes/modern which I'll attach in a moment.
Attachment #219534 - Flags: superreview?(neil)
Attachment #219534 - Flags: review?(neil)
Attached patch icon for modern theme (obsolete) — Splinter Review
The preview icon for the modern theme - to be added to suite/themes/modern. Based on the reload button and converted to a png.
Preview image for the modern theme - taken from the existing one, just converted to a png and will go into suite/themes/modern.
Comment on attachment 219535 [details] [diff] [review]
icon for modern theme

Try getting the right attachment type ;-)
Attachment #219535 - Attachment is obsolete: true
Correct attachement type.
Comment on attachment 219534 [details] [diff] [review]
Part 2 - make the modern theme compatible with the new themes manager.

Thinking about it, it'd be useful if Kairo checks this patch as well.
Attachment #219534 - Flags: review?(neil) → review?(kairo)
Attached file Proposed addition to browser-prefs.js (obsolete) —
This attachment contains a provisional list of preferences for addition to browser-prefs.js. They are related to updates or extensions. We need the extension ones for certain so that the extension manager will work correctly. The update ones I'd thought I'd include as we are looking at prefs related areas - though will we be able to use the update mechanism? I've set updates off for app ones for the time being, and on for extensions, though obviously it won't really work just yet (unless we have some extensions that are already enabled for suiterunner.

I need to make this into a proper patch, but any comments before I do that would be very useful.
Comment on attachment 219588 [details]
Proposed addition to browser-prefs.js

Looks reasonable. I assume these will be #ifdef MOZ_XUL_APP (and the existing ones will be in the #else)?

>// Interval: When all registered timers should be checked (in milliseconds)
>//           default=5 seconds
>pref("app.update.timer", 600000);
Looks like 10 minutes to me - someone forgot to update the comment ;-)
Comment on attachment 219588 [details]
Proposed addition to browser-prefs.js

>// A default value for the "More information about this update" link
>// supplied in the "An update is available" page of the update wizard. 
>pref("app.update.url.details", "chrome://browser-region/locale/region.properties");

browser-region does not exist in SeaMonkey. Set this to communicator-region and add the correct line in that file along with a patch for that.

>// *** These are up & coming for firefox 2.0

I guess that comment can be removed ;-)

We need to check chrome://mozapps/locale/extensions/extensions.properties if the stuff it sets is OK for us, it's references multiple times in this section.
Comment on attachment 219534 [details] [diff] [review]
Part 2 - make the modern theme compatible with the new themes manager.

>+INSTALL_EXTENSION_ID   = $(XPI_NAME)@mozilla.org

I think I'd prefer to make clear that this is a theme in the ID. Probably $(XPI_NAME)@themes.mozilla.org would be a good idea here.

>+++ suite/themes/modern/install.rdf	2006-04-23 17:37:10.000000000 +0100

Thanks for adding those files in the new location already! :)

>+    <em:id>modern@mozilla.org</em:id>
>+    <em:version>1.0</em:version>

Same with the name here, of course.
I know the way you do it is consistent with current code, but in theory, it would be version 3.x of modern already ;-)


>+    <em:targetApplication>
>+      <Description>
>+        <em:id>{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}</em:id>

Actually, I wonder in how many places (outside our code) this GUID is already used and if it might make sense to move to seamonkey@applications.mozilla.org


With the extension ID change, r=me.
Attachment #219534 - Flags: review?(kairo) → review+
(In reply to comment #11)
> (From update of attachment 219534 [details] [diff] [review] [edit])
> >+    <em:targetApplication>
> >+      <Description>
> >+        <em:id>{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}</em:id>
> Actually, I wonder in how many places (outside our code) this GUID is already
> used and if it might make sense to move to seamonkey@applications.mozilla.org

I'm guessing all the extensions for SeaMonkey and things like update.mozilla.org and addons.mozilla.org as a minimum.
Comment on attachment 219534 [details] [diff] [review]
Part 2 - make the modern theme compatible with the new themes manager.

>+DIST_FILES := \
>+	../../suite/themes/modern/install.rdf \
>+	../../suite/themes/modern/preview.png \
>+	../../suite/themes/modern/icon.png \
>+	$(NULL)
We can't preprocess binary files. You'll have to manually write a libs:: rule for them.
Attachment #219534 - Flags: superreview?(neil) → superreview-
Updated to address KaiRo's and Neil's comments. Carrying forward KaiRo's r+.
Attachment #219610 - Flags: superreview?(neil)
Attachment #219610 - Flags: review+
Attached image preview image for the classic theme (obsolete) —
This is the preview image for the classic theme taken from the existing one and made into a png file. It will go into suite/themes/classic see also patch I'm just about to attach.
Attached image icon for classic theme (obsolete) —
This is the icon for the classic theme taken from the reload button on the preview image. It will go into suite/themes/classic see also patch I'm just about to attach.
Similar to the modern patch, however, to fit into the existing tree structure, we can't easily rename the extension to classic@themes.mozilla.org, therefore I've kept the uuid that Thunderbird & Firefox use, though note the only directories we create with it are on the output.

Also requesting approval of the two image files I've just posted.

Also note we can't use DIST_FILES for the install.rdf as that requires FINAL_TARGET which would affect where the classic.jar would end up.
Attachment #219660 - Flags: superreview?(neil)
Attachment #219660 - Flags: review?(kairo)
Comment on attachment 219610 [details] [diff] [review]
Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)

Bah, this xpi_name stuff really sucks as a way of building chrome. But while you're using it, it looks as if you're supposed to distribute stuff to $(FINAL_TARGET) which rules.mk will then copy to extensions/
Comment on attachment 219660 [details] [diff] [review]
Part 3 - make the classic theme compatible with the new themes manager

>+	$(PERL) $(MOZILLA_DIR)/config/preprocessor.pl $(DEFINES) $(ACDEFINES) $(topsrcdir)/suite/themes/classic/install.rdf.in > install.rdf
I don't really like this, I'd prefer to preprocess suite/themes/classic/install.rdf directly to the target (a bit like EXTRA_PP_COMPONENTS). However I do like the way that the .jar file does not move. Is there any way you can do this for Modern too?
Comment on attachment 219660 [details] [diff] [review]
Part 3 - make the classic theme compatible with the new themes manager

>Index: themes/classic/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/themes/classic/Makefile.in,v
>retrieving revision 1.23
>diff -u -p -r1.23 Makefile.in
>--- themes/classic/Makefile.in	10 Aug 2004 22:59:42 -0000	1.23
>+FILES := \
>+	install.rdf \
>+	$(topsrcdir)/suite/themes/classic/preview.png \
>+	$(topsrcdir)/suite/themes/classic/icon.png \
>+	$(NULL)
>+
>+libs::
>+	$(PERL) $(MOZILLA_DIR)/config/preprocessor.pl $(DEFINES) $(ACDEFINES) $(topsrcdir)/suite/themes/classic/install.rdf.in > install.rdf
>+	$(INSTALL) $(FILES) $(DIST)/bin/extensions/$(CLASSIC_EXTENSION_DIR)

Like Neil said, please make the preprocessor write the file directly to the target location instead of using this intermediate location.

>+* skin/classic/global/contents.rdf                              (global/mac/contents.rdf)
>+* skin/classic/global/contents.rdf                            (global/win/contents.rdf)

This doesn't work like you want, it unconditionally overwrites the mac version with the win version - on all platforms.

>+  skin/classic/global/preview.gif                             (global/win/preview.gif)
>+  skin/classic/global/preview.gif                               (global/mac/preview.gif)

same here, but the other way round.

>+    <em:type>4</em:type>

Hmm, can you point me where I can find waht "4" is supposed to mean here? I'm just curious ;-)

>+    <em:description>This theme simulates the familiar appearance of previous Netscape versions</em:description>

We really want to change the icons of this theme at some point and that will break your description. Please change it to something that tells that we are trying to use colors and styling from the system to fit in as good as we can with other applications on the system.


Additionally:
Please attach an icon and preview that really fits Classic - you mistakenly attached the Modern ones again.

r- because of the skin/classic/global/ problems
Attachment #219660 - Flags: review?(kairo) → review-
(In reply to comment #19)
> (From update of attachment 219660 [details] [diff] [review] [edit])
> However I do like the way that the .jar file does not move.
> Is there any way you can do this for Modern too?

Unfortunately no. The way extension manager does it only allows for one default theme - for all other themes you have to construct it as per a proper theme structure - there is no redirect. As far as I can tell, having classic stay where it is is a bit of a hack anyway: http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#1652

(In reply to comment #20)
> (From update of attachment 219660 [details] [diff] [review] [edit])
> >+    <em:type>4</em:type>
> Hmm, can you point me where I can find waht "4" is supposed to mean here? I'm
> just curious ;-)

http://developer.mozilla.org/en/docs/install.rdf#type
The correct one this time.
Attachment #219658 - Attachment is obsolete: true
the correct one this time.
Attachment #219657 - Attachment is obsolete: true
Comment on attachment 219610 [details] [diff] [review]
Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)

The next patch I add will include both the theme patches in the same patch.
Attachment #219610 - Flags: superreview?(neil)
Attachment #219660 - Flags: superreview?(neil)
As we're not really building an extension.
This patch adds the prefs we need to use extension manager/update properly (note updates are disabled for now). It also incorporates the changes for the recent Extension Manager change from Firefox.
Attachment #219588 - Attachment is obsolete: true
Attachment #220040 - Flags: review?(kairo)
Comment on attachment 220040 [details] [diff] [review]
Prefs update and EM/TM -> Addons update

>+// Whether or not we show a dialog box informing the user that the update was
>+// successfully applied. This is off in Firefox by default since we show a 
>+// upgrade start page instead! Other apps may wish to show this UI, and supply
>+// a whatsNewURL field in their brand.properties that contains a link to a page
>+// which tells users what's new in this new update.
>+pref("app.update.showInstalledUI", false);

The comment tells about what Firefox does, but what do we do (once we have turned updates on at all)?


>+pref("extensions.update.interval", 86400);  // Check for updates to Extensions and 
>+                                            // Themes every week

Nit: 86400 isn't a week, right?


>+# This is the fallback URL for release notes. Do not change this
>+# unless you are providing localized release notes!
>+app.update.url.details=http://www.mozilla.org/products/seamonkey/releases/

"More information about this update" in the "An update is available" page of the update wizard is not necessarily release notes. Just tell in the comment what this really is (i.e. first part of my sentence before, shortenend from the blurb you're adding to prefs).


>-    function toEM(aType)
>+    function toAM()

Nah, just leave toEM() as the function name, it's still called "extension manager" or "EM" everywhere internally.


r=me with those nits addressed.
Attachment #220040 - Flags: review?(kairo) → review+
Addressed KaiRo's comments. Carrying forward r.
Attachment #220040 - Attachment is obsolete: true
Attachment #220059 - Flags: superreview?(neil)
Attachment #220059 - Flags: review+
Revised patch for classic addressing previously received comments. Also included a comment removal for something I had previously thought was necessary, but no longer.

Note: the images attached for classic also apply as part of this patch.
Attachment #219534 - Attachment is obsolete: true
Attachment #219660 - Attachment is obsolete: true
Attachment #220060 - Flags: review?(kairo)
Comment on attachment 220060 [details] [diff] [review]
Part 3 v2 - make the classic theme compatible with the new themes manager (checked in)

>+FILES = \
>+	$(topsrcdir)/suite/themes/classic/preview.png \
>+	$(topsrcdir)/suite/themes/classic/icon.png \
>+	$(NULL)
We have separate Mac previews, no? So, I guess this should be ifdef, although I don't know where the Mac files would go.

>+	$(SYSINSTALL) $(IFLAGS1) $(FILES) $(DESTDIR)$(mozappdir)/extensions/$(CLASSIC_EXTENSION_DIR)
>+	$(PERL) $(MOZILLA_DIR)/config/preprocessor.pl $(DEFINES) $(ACDEFINES) $(topsrcdir)/suite/themes/classic/install.rdf > $(DIST)/bin/extensions/$(CLASSIC_EXTENSION_DIR)/install.rdf
Surely this should be > $(DESTDIR)$(mozappdir)/extensions/$(CLASSIC_EXTENSION_DIR)/install.rdf

>+* skin/classic/editor/contents.rdf                                      (editor/contents.rdf)
>+* skin/classic/messenger/contents.rdf                                   (messenger/contents.rdf)
>+* skin/classic/navigator/contents.rdf                                   (navigator/contents.rdf)
>+#ifdef XP_MACOSX
>+* skin/classic/global/contents.rdf                              (global/mac/contents.rdf)
>+  skin/classic/global/preview.gif                               (global/mac/preview.gif)
>+#else
>+* skin/classic/global/contents.rdf                            (global/win/contents.rdf)
>+  skin/classic/global/preview.gif                             (global/win/preview.gif)
>+#endif
>+#endif
It would be nice if all the (s were aligned.

>+  </Description>      
I happened to spot some trailing whitespace here, please recheck your patch.
Attachment #219817 - Flags: superreview?(jag)
Attachment #219817 - Flags: review?(kairo)
Comment on attachment 220059 [details] [diff] [review]
Prefs update and EM/TM -> Addons update v2 (checked in)

> // defaultChromeURI is only needed until our default command line handler knows what to launch
> pref("toolkit.defaultChromeURI","chrome://navigator/content/navigator.xul");
Since this pref is hopefully going away real soon now (I guess I ought to ask for reviews!) I'd prefer you added your new xpinstall prefs near the old xpinstall prefs so that the old xpinstalled prefs only get ifdefed.
Comment on attachment 220060 [details] [diff] [review]
Part 3 v2 - make the classic theme compatible with the new themes manager (checked in)

Looks good to me now. r=me
Attachment #220060 - Flags: review?(kairo) → review+
Comment on attachment 219817 [details] [diff] [review]
Bypass xpi-stage (for themes/modern/Makefile.in - checked in)

This version looks even better then Mark's original one ;-) r=me
Attachment #219817 - Flags: review?(kairo) → review+
Attachment #220060 - Flags: superreview?(neil)
(In reply to comment #30)
> (From update of attachment 220060 [details] [diff] [review] [edit])

Oh didn't realise you'd already looked at this ;-)

> >+FILES = \
> >+	$(topsrcdir)/suite/themes/classic/preview.png \
> >+	$(topsrcdir)/suite/themes/classic/icon.png \
> >+	$(NULL)
> We have separate Mac previews, no? So, I guess this should be ifdef, although I
> don't know where the Mac files would go.

KaiRo and I have basically agreed there is no point keeping the Mac previews - they are not that different. Its the difference between http://lxr.mozilla.org/seamonkey/source/themes/classic/global/win/preview.gif and http://lxr.mozilla.org/seamonkey/source/themes/classic/global/mac/preview.gif
Comment on attachment 219610 [details] [diff] [review]
Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)

We're going to need the non Makefile.in parts of this patch to go with the bypass xpi-stage patch. KaiRo already gave it r, so I'm requesting sr for the non Makefile.in parts.
Attachment #219610 - Flags: superreview?(neil)
Comment on attachment 219817 [details] [diff] [review]
Bypass xpi-stage (for themes/modern/Makefile.in - checked in)

config.mk also pulls in autoconf.mk, so remove |include $(DEPTH)/config/autoconf.mk| and sr=jag
Attachment #219817 - Flags: superreview?(jag) → superreview+
Comment on attachment 220060 [details] [diff] [review]
Part 3 v2 - make the classic theme compatible with the new themes manager (checked in)

sr=me with the whitespace nits fixed.
Attachment #220060 - Flags: superreview?(neil) → superreview+
Comment on attachment 220059 [details] [diff] [review]
Prefs update and EM/TM -> Addons update v2 (checked in)

sr=me with the (ifdefed) prefs moved back to line 194.
Attachment #220059 - Flags: superreview?(neil) → superreview+
Comment on attachment 219610 [details] [diff] [review]
Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)

sr=me with install.rdf's trailing whitespace removed. I assume you'll check in my patch at the same time?
Attachment #219610 - Flags: superreview?(neil) → superreview+
Comment on attachment 220059 [details] [diff] [review]
Prefs update and EM/TM -> Addons update v2 (checked in)

>+<!ENTITY addonsManagerCmd.label "Addons">
>+<!ENTITY addonsManagerCmd.accesskey "A">
Sorry for the late notice, but can we call this "Add-ons Manager"?
(In reply to comment #40)
> Sorry for the late notice, but can we call this "Add-ons Manager"?

"Manager" sounds good, but according to the patches in bug 329045 it's called "Addons" (no hyphen) everywhere else, introducing the hyphen would make us inconsistent.
Attachment #219536 - Attachment description: preview image for modern theme → preview image for modern theme (checked in)
Attachment #219537 - Attachment description: icon for modern theme → icon for modern theme (checked in)
Attachment #219763 - Attachment description: icon for classic theme → icon for classic theme (checked in)
Attachment #219764 - Attachment description: preview image for the classic theme → preview image for the classic theme (checked in)
Attachment #219817 - Attachment description: Bypass xpi-stage → Bypass xpi-stage (for themes/modern/Makefile.in - checked in)
Attachment #219610 - Attachment description: Part 2 v2 - make the modern theme compatible with the new themes manager. → Part 2 v2 - make the modern theme compatible with the new themes manager. (checked in excluding Makefile.in)
Attachment #220060 - Attachment description: Part 3 v2 - make the classic theme compatible with the new themes manager → Part 3 v2 - make the classic theme compatible with the new themes manager (checked in)
(In reply to comment #36)
> (From update of attachment 219817 [details] [diff] [review] [edit])
> config.mk also pulls in autoconf.mk, so remove |include
> $(DEPTH)/config/autoconf.mk| and sr=jag
> 

We can't remove autoconf.mk because it will actually break the build:

NeilAway: you must include autoconf.mk first to get MOZ_XUL_APP
...
NeilAway: we can't set USE_EXTENSION_MANIFEST in an XPFE build
NeilAway: so we need to set it after autoconf.mk to get MOZ_XUL_APP but before config.mk so that it actually does something
NeilAway: then after including config.mk we can reset FINAL_TARGET
Attachment #220059 - Attachment description: Prefs update and EM/TM -> Addons update v2 → Prefs update and EM/TM -> Addons update v2 (checked in)
(In reply to comment #42)
> (In reply to comment #36)
> > (From update of attachment 219817 [details] [diff] [review] [edit] [edit])
> > config.mk also pulls in autoconf.mk, so remove |include
> > $(DEPTH)/config/autoconf.mk| and sr=jag
> > 
> 
> We can't remove autoconf.mk because it will actually break the build:
> 
> NeilAway: you must include autoconf.mk first to get MOZ_XUL_APP
> ...
> NeilAway: we can't set USE_EXTENSION_MANIFEST in an XPFE build
> NeilAway: so we need to set it after autoconf.mk to get MOZ_XUL_APP but before
> config.mk so that it actually does something
> NeilAway: then after including config.mk we can reset FINAL_TARGET
> 

I've checked in the original version of this patch as a fixup, but I've also added a comment to remove "include $(DEPTH)/config/autoconf.mk" when we throw the MOZ_XUL_APP switch as we won't need it then.
Whiteboard: Add-on Manager should work. Closure waiting on removal of existing controls patch.
Follow-up patch, installed-chrome.txt shouldn't be needed by suiterunner. I've been using this patch for quite a while without problems - just forgot to submit it earlier ;-)
Attachment #220313 - Flags: superreview?(neil)
Attachment #220313 - Flags: review?(neil)
Comment on attachment 220313 [details] [diff] [review]
installed-chrome.txt isn't needed by suiterunner (checked in).

KaiRo convinced me that we should just get rid of this after the switch to save adding more ifdefs. I'd missed adding the xxx comment anyway.
Attachment #220313 - Flags: superreview?(neil)
Attachment #220313 - Flags: review?(neil)
Comment on attachment 220313 [details] [diff] [review]
installed-chrome.txt isn't needed by suiterunner (checked in).

This sucked anyway as I now have a zillion select lines in my installed-chrome.txt!
Unfortunately this causs problem with some build configurations, it seems :(

My builds now stop with the following error:

/mnt/mozilla/build/seamonkey-dbg/config/nsinstall -R /mnt/mozilla/src/mozilla/suite/themes/modern/preview.png /mnt/mozilla/src/mozilla/suite/themes/modern/icon.png  ../../dist/bin/extensions/modern@themes.mozilla.org
+++ making chrome /mnt/mozilla/build/seamonkey-dbg/themes/modern  => ../../dist/bin/extensions/modern@themes.mozilla.org/chrome/modern.jar
copy(/mnt/mozilla/src/mozilla/themes/modern/communicator/brand.css, ../../dist/bin/extensions/modern@themes.mozilla.org/chrome/modern/skin/modern/communicator/brand.css) failed: No such file or directory at /mnt/mozilla/src/mozilla/config/make-jars.pl line 503, <STDIN> line 475.
make: *** [libs] Error 2

Obviously, this happens in the /mnt/mozilla/build/seamonkey-dbg/themes/modern dir on my machine (Linux), I think the problem is me building with --enable-chrome-format=both and the flat version of the chrome somehow can't be written correctly...
It's definately the chrome-format option. I tested with manually setting MOZ_CHROME_FILE_FORMAT in autoconf.mk to jar, flat and symlink (as both is just jar+flat), and out of those three only jar works, the two others fail. I guess the @ in the dir name might have something to do with it...
(In reply to comment #48)
> It's definately the chrome-format option. I tested with manually setting
> MOZ_CHROME_FILE_FORMAT in autoconf.mk to jar, flat and symlink (as both is just
> jar+flat), and out of those three only jar works, the two others fail. I guess
> the @ in the dir name might have something to do with it...
> 

I'd be surprised if its the @ - take DOMI for example, I'm fairly sure that gets built before themes (with suiterunner) and that uses inspector@mozilla.org as the directory name.
Is theme manager set up in 2006101906 for Mac? 10.4.9. If I switch to the classic theme in Preferences and restart as instructed, I do not get any icon changes. Preferences shows the Classic theme as selected, but the Modern theme is displayed.

I've seen several bugs about building the Modern theme, but this looks closest to where I might share concern for theme switching not working, short of writing a new bug. (Which I'll do if that's a better solution.)
(In reply to comment #50)
> Is theme manager set up in 2006101906 for Mac? 10.4.9.

No, the theme manager (actually now called Add-on Manager) won't be enabled until bug 328887 depends - when we throw the switch on trunk, unless you're using one of the specific suiterunner builds.

There are several known theme bugs in the suiterunner builds other bugs with non-suiterunner builds should be filed in other bugs if there are no duplicates.
Summary: Get SeaMonkey's themes registering and switching with Theme Manager → Get SeaMonkey's themes registering and switching with Theme Manager (in suiterunner)
Comment on attachment 219520 [details] [diff] [review]
Part 1 - Remove existing theme controls from suiterunner.

>+#ifndef MOZ_XUL_APP
>+// XXX Remove this section when Suite becomes a full XUL app
> function applyTheme(themeName)
...
>+#ifndef MOZ_XUL_APP
>+<!-- XXX Remove this section when Suite becomes a full XUL app -->
>         <menuseparator />
>         <menu label="&applyTheme.label;" accesskey="&applyTheme.accesskey;">
>           <menupopup id="theme" datasources="rdf:chrome" 
>                      ref="urn:mozilla:skin:root" 
>                      oncommand="applyTheme(event.target)" 
>                      onpopupshowing="checkTheme()"

While keeping the pref panel makes no sense with themes being part of the add-on manager, I think we should try to keep the "View->Apply Theme" menu (if possible at all).
Blocks: 366318
As discussed on the previous comment and on irc, this patch just removes the old theme preferences pane and adds an option to the debug pane for allowing dynamic switching of themes.

I think the View -> Apply Theme menu options should be covered by a difference bug.
Attachment #219520 - Attachment is obsolete: true
Attachment #252158 - Flags: superreview?(neil)
Attachment #252158 - Flags: review?(neil)
Attachment #219520 - Flags: superreview?(neil)
Attachment #219520 - Flags: review?(neil)
Comment on attachment 220313 [details] [diff] [review]
installed-chrome.txt isn't needed by suiterunner (checked in).

Previously KaiRo & I said we'd leave this, however we have now decided it would be a good thing to do as it'll make it clearer that its not necessary (we now use the general.skins.selectedSkin pref), and help towards getting rid of installed-chrome.txt for suiterunner.
Attachment #220313 - Flags: superreview?(neil)
Attachment #220313 - Flags: review?(neil)
Comment on attachment 220313 [details] [diff] [review]
installed-chrome.txt isn't needed by suiterunner (checked in).

I never did like these lines, because they accumulate in depend builds. (I had prefixed grep -q skin,install,select <path to>installed-chrome.txt || to them in one of my builds to cut down on the wastage!)

> # select classic as the default skin
>+ifndef MOZ_XUL_APP
> libs::
> 	echo skin,install,select,classic/1.0 >> $(DIST)/bin/chrome/installed-chrome.txt
> 
> install::
> 	echo skin,install,select,classic/1.0 >> $(DESTDIR)$(mozappdir)/chrome/installed-chrome.txt
>+endif
Nit: might as well ifdef out the comment too :-P
Attachment #220313 - Flags: superreview?(neil)
Attachment #220313 - Flags: superreview+
Attachment #220313 - Flags: review?(neil)
Attachment #220313 - Flags: review+
Attachment #220313 - Attachment description: installed-chrome.txt isn't needed by suiterunner. → installed-chrome.txt isn't needed by suiterunner (checked in).
Blocks: 370910
This patch does just the adding of the dynamic skin switching option to the debug page (as Neil mentioned to me a while ago on irc).

The removal of the theme preferences pane in suiterunner will be done at/after the switchover on trunk (no schedule for that yet), so I'll raise a separate bug for that which will allow this one to be closed down once this patch goes in.
Attachment #252158 - Attachment is obsolete: true
Attachment #257163 - Flags: superreview?(neil)
Attachment #257163 - Flags: review?(neil)
Attachment #252158 - Flags: superreview?(neil)
Attachment #252158 - Flags: review?(neil)
Comment on attachment 257163 [details] [diff] [review]
Add dynamic skin switching option to debug page (checked in)

>+<!ENTITY dynamicSkinSwitching.label      "Enable Dynamic Skin Switching (Add-on Manager enabled versions only.">
Nit: missing ). Or rewrite the text as "Allow Add-on Manager to dynamically switch skins". Or I could patch xpfe theme switcher to watch the pref ;-)
Attachment #257163 - Flags: superreview?(neil)
Attachment #257163 - Flags: superreview+
Attachment #257163 - Flags: review?(neil)
Attachment #257163 - Flags: review+
Attachment #257163 - Attachment description: Add dynamic skin switching option to debug page → Add dynamic skin switching option to debug page (checked in)
Comment on attachment 257243 [details] [diff] [review]
Adds dss to xpfe (checked in)

Looks ok to me, r=me.
Attachment #257243 - Flags: review+
Comment on attachment 257243 [details] [diff] [review]
Adds dss to xpfe (checked in)

Neil just checked this in.
Attachment #257243 - Attachment description: Adds dss to xpfe → Adds dss to xpfe (checked in)
This is now fixed. The themes pane will be removed from preferences by bug 372856.
Blocks: 372856
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: Add-on Manager should work. Closure waiting on removal of existing controls patch.
You need to log in before you can comment on or make changes to this bug.