Mark Banner (:standard8) Assignee
	Description • 19 years ago

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

	Mark Banner (:standard8) Assignee
	Comment 1 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 2 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 3 • 19 years ago

The preview icon for the modern theme - to be added to suite/themes/modern. Based on the reload button and converted to a png.

	Mark Banner (:standard8) Assignee
	Comment 4 • 19 years ago

Preview image for the modern theme - taken from the existing one, just converted to a png and will go into suite/themes/modern.

	Mark Banner (:standard8) Assignee
	Comment 5 • 19 years ago

Comment on attachment 219535 [details] [diff] [review]
icon for modern theme

Try getting the right attachment type ;-)

	Mark Banner (:standard8) Assignee
	Comment 6 • 19 years ago

Correct attachement type.

	Mark Banner (:standard8) Assignee
	Comment 7 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 8 • 19 years ago

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.

	neil@parkwaycc.co.uk
	Comment 9 • 19 years ago

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 ;-)

	Robert Kaiser
	Comment 10 • 19 years ago

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.

	Robert Kaiser
	Comment 11 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 12 • 19 years ago

(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.

	neil@parkwaycc.co.uk
	Comment 13 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 14 • 19 years ago

Updated to address KaiRo's and Neil's comments. Carrying forward KaiRo's r+.

	Mark Banner (:standard8) Assignee
	Comment 15 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 16 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 17 • 19 years ago

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.

	neil@parkwaycc.co.uk
	Comment 18 • 19 years ago

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/

	neil@parkwaycc.co.uk
	Comment 19 • 19 years ago

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?

	Robert Kaiser
	Comment 20 • 19 years ago

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

	Mark Banner (:standard8) Assignee
	Comment 21 • 19 years ago

(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

	Mark Banner (:standard8) Assignee
	Comment 22 • 19 years ago

The correct one this time.

	Mark Banner (:standard8) Assignee
	Comment 23 • 19 years ago

the correct one this time.

	Mark Banner (:standard8) Assignee
	Comment 24 • 19 years ago

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.

	neil@parkwaycc.co.uk
	Comment 25 • 19 years ago

As we're not really building an extension.

	Mark Banner (:standard8) Assignee
	Comment 26 • 19 years ago

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.

	Robert Kaiser
	Comment 27 • 19 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 28 • 19 years ago

Addressed KaiRo's comments. Carrying forward r.

	Mark Banner (:standard8) Assignee
	Comment 29 • 19 years ago

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.

	neil@parkwaycc.co.uk
	Comment 30 • 19 years ago

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.

	neil@parkwaycc.co.uk
	Comment 31 • 19 years ago

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.

	Robert Kaiser
	Comment 32 • 19 years ago

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

	Robert Kaiser
	Comment 33 • 19 years ago

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

	Mark Banner (:standard8) Assignee
	Comment 34 • 19 years ago

(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

	Mark Banner (:standard8) Assignee
	Comment 35 • 19 years ago

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.

	jag (Peter Annema)
	Comment 36 • 19 years ago

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

	neil@parkwaycc.co.uk
	Comment 37 • 19 years ago

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.

	neil@parkwaycc.co.uk
	Comment 38 • 19 years ago

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.

	neil@parkwaycc.co.uk
	Comment 39 • 19 years ago

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?

	neil@parkwaycc.co.uk
	Comment 40 • 19 years ago

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"?

	Robert Kaiser
	Comment 41 • 19 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 42 • 19 years ago

(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

	Mark Banner (:standard8) Assignee
	Comment 43 • 19 years ago

(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.

	Mark Banner (:standard8) Assignee
	Comment 44 • 19 years ago

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 ;-)

	Mark Banner (:standard8) Assignee
	Comment 45 • 19 years ago

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.

	neil@parkwaycc.co.uk
	Comment 46 • 19 years ago

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!

	Robert Kaiser
	Comment 47 • 19 years ago

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...

	Robert Kaiser
	Comment 48 • 19 years ago

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...

	Mark Banner (:standard8) Assignee
	Comment 49 • 19 years ago

(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.

	David Barr
	Comment 50 • 18 years ago

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.)

	Mark Banner (:standard8) Assignee
	Comment 51 • 18 years ago

(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.

	Karsten Düsterloh
	Comment 52 • 18 years ago

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).

	Mark Banner (:standard8) Assignee
	Comment 53 • 18 years ago

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.

	Mark Banner (:standard8) Assignee
	Comment 54 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 55 • 18 years ago

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

	Mark Banner (:standard8) Assignee
	Comment 56 • 18 years ago

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.

	neil@parkwaycc.co.uk
	Comment 57 • 18 years ago

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 ;-)

	Mark Banner (:standard8) Assignee
	Comment 59 • 18 years ago

Comment on attachment 257243 [details] [diff] [review]
Adds dss to xpfe (checked in)

Looks ok to me, r=me.

	Mark Banner (:standard8) Assignee
	Comment 60 • 18 years ago

Comment on attachment 257243 [details] [diff] [review]
Adds dss to xpfe (checked in)

Neil just checked this in.

	Mark Banner (:standard8) Assignee
	Comment 61 • 18 years ago

This is now fixed. The themes pane will be removed from preferences by bug 372856.