Closed Bug 217410 Opened 22 years ago Closed 20 years ago

Bump skin version?

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Platform:
x86
Windows XP
Type:
defect

Tracking

()

Status:
RESOLVED INVALID
Milestone:
Firefox0.9

People

(Reporter: bugzilla, Assigned: bugs)

Details

Attachments

(9 obsolete files)

Target Milestone: --- → Firebird0.7
I concur, not updated themes are broken. We do not ship anymore the xpfe/global widgets. They were bound in skin files (arg).
I am thinking of changing both our localeVersions and our skinVersions for everything to fb0.7 so we can get on our own versioning system without having to follow Mozilla. The "fb" prefix is so we don't overlap with Mozilla; we don't want people using Mozilla 1.0 skins and locales when Firebird 1.0 comes out. Objections?
Is "Firebird" going to be a permanent name, or just a code name until all this to-ing and fro-ing is complete? I would tend to suggest "MB", for "Mozilla Browser" since that's what the branding strategy says, and I quote: "3. When referring to Thunderbird or Firebird before or during the 1.4 release cycle, make sure to use the project name with Mozilla pre-pended as "Mozilla Thunderbird" or "Mozilla Firebird" instead of Mozilla alone or Firebird/Thunderbird alone. "After the release of 1.4 we will be doing our primary development on the Firebird and Thunderbird projects. When we do releases of that codebase we should be using self-descriptive brand identities for the public and the press. New rule: "4. Use the names "Mozilla Browser" and "Mozilla Mail" to describe the Firebird and Thunderbird projects after the 1.4 release." http://www.mozilla.org/roadmap/branding.html Another thing I'd like you to consider is appending a letter each time you break theme-related stuff in a nightly, so that mb0.7 can be reserved for the 0.7 release, etc, and mb0.7a can be for the first theme bustage, mb07b for the second, etc, in the pre-0.7 nightlies. This avoids the problem of having allegedly compatible themes attempt to apply themselves to a browser that isn't. It does mean however that the skinVersion will have to be bumped on release day.
IMO, theme versions should only increase when there is a theme-breaking change. If 0.7 themes and 0.9 themes are compatible, the two milestones should have the same themeVersion.
Sorry, I wasn't entirely clear there. What I meant is that if we break stuff in a nightly, we use a letter suffix for the theme version (mb0.7a). Then when we release, we pull the suffix off (mb0.7). But if there are no skin-breaking changes in the pre-0.8 nightlies, we do nothing and leave the skinVersion at 0.7 for the 0.8 release.
Branding is still up in the air. The branding doc is out of date, unfortunately. We're trying to fix this quickly. I agree with comment #4. /be
Fixed. Skin and locale versions are now fb0.7. I like David's idea of fb0.8a, fb0.8b, etc.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
I agree with David, except I don't think 0.8b should be changed to 0.8 right before the release. Just leave it as 0.8b for the 0.8 release (and future releases if there are no more theme-breaking changes).
reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
If the current theme version is e.g. fb0.8c and we're getting closer to a release, the developers should have a pretty good idea about whether or not there will be theme breaking checkins. Most probably, there will be no more theme breaking checkins when we're a week from a release. Therefore, just bump the theme version from fb0.8c to fb0.8 a week before (expected) release. By communicating this in the forums, theme authors should have plenty of time to update their theme to be compatible with the fb0.8 canditate builds and the final fb0.8. Leaving the theme version as fb0.8c just wouldn't look good imo.
I don't think there's a problem calling a skinVer fb0.8c. In terms of Firebird Help, our browser version detector has gone right out the window. I'm sure things will settle down again, but maybe we want to add something in the userAgent string as identification when issuing nightly builds.
Well this is just me, but I would prefer that the a, b, c, etc. be appended to the current version not the next release version. Otherwise the versions will go 0.7, 0.8a, 0.8b, 0.8c 0.8, 0.9a ... Kind of makes it difficult for people to figure out which is newer and which is older don't you think? 0.7, 0.7a, 0.7b, 0.7c, 0.8, 0.8a seems to be a more logical progresssion of version numbering. Might even want to consider .1, .2, .3 instead of a, b, c. makes it easy to apply some formula to convert the dotted decimal version number to an integer so that < and > tests can be used in comparisons. So that 0.7 would maean version 0.7. first post 0.7 bustage version would be 0.7.1 all leading up to the next version which would be 0.8.
QA Contact: asa
OK, let's look at this for .8. I'm too busy chasing other things at the moment to make sure this gets done right by cutoff tomorrow.
Target Milestone: Firebird0.7 → Firebird0.8
I can't believe we're going with 0.7 without this. Oh well, I'll dirty my hands a little and see if I can finish this before 0.7 - what version string do we want? I'm going ahead with 1.5 for right now. Tell me if you want otherwise.
Oops, meant that as 1.6, not 1.5...
Okay, I've gone through and changed skin version in the files changed originally in the "1.5"->"fb0.7" change back on 08-27. New skin version = "1.5". Now - I'm reasonably new to CVS, Linux, and working with Moz source code - could someone tell me how to create a patch? I won't be able to get to this until about 2:30pm EST, so I don't know if it'll make 0.7 or not.
Attached patch Patch to make skin version="1.6" (obsolete) — — Splinter Review
Okay, finally got a patch made. This changes all the files that were changed in the "fb0.7" change made on 08-27 (excluding tabbrowser.xml, which didn't belong as part of the changes), viewable at: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=08%2F25%2F2003+17%3A23%3A00&maxdate=08%2F28%2F2003+17%3A23%3A00&cvsroot=%2Fcvsroot The new skin version as set by this patch is "1.6". If the "fb0.7" patch changed the version, this should too hopefully. Important to note is the fact that the Qute default theme's themeversion has not been changed in this patch; I don't know enough about the source tree to want to try modifying that, so someone else will have to patch that. Also, as this is my first patch ever and I really don't know much about the tree, this needs to be checked over a little bit more than normal before approval.
Why 1.6? Why not tk0.1? The whole point was that this is no longer synced to Seamonkey's skinVer. Although I'd like to see this go in for 0.7, I'm not sure if there's enough debugging time left.
Why 1.6? I really don't know. I just figured that was next and based on comments, I wasn't really sure what should be used. Something with "fb0.x" was possible, but I wasn't certain. If you know more than I do, feel free to update the attachment (if you can, not sure how patch edit permissions are set).
1.5 is because Seamonkey was at 1.5. Toolkit is now out of sync, which is why I think tk0.1 is good.
Okay then, hearing only objections to using "1.6" and hearing only a comment suggesting "tk0.1", here's a new patch that converts skin/locale versions to "tk0.1". Marking old "1.6" attachment obsolete. I think I'll also put this up for review after this post so it'll hopefully get in soon.
Attachment #132084 - Attachment is obsolete: true
Comment on attachment 132173 [details] [diff] [review] Make skin version="tk0.1" as per comments Since Blake Ross did the old skin version change from "1.5"->"fb0.7", I'll give this to him. He'll probably know if anything's changed since then that would make the patch bad.
Attachment #132173 - Flags: review?(blake)
Okay, looking through Bugzilla and saw bug 215328, which did some localeVersion modifying the old patch for this bug had done. I got rid of the files that patch modified for me (and all files that had a localeVersion=1.5b already). There are still a few files with only localeVersion being modified, but I'm 100% positive the changes should go. I'll post on that bug for the author to look at this patch and recommend any changes he thinks are necessary. Setting review? in a second...
Attachment #132173 - Attachment is obsolete: true
Comment on attachment 133376 [details] [diff] [review] Newer patch that gets rid of bug 215328 conflicts Giving to Blake...please look for inconsistencies you see with the patch in bug 215328.
Attachment #133376 - Flags: review?(blake)
Attachment #132173 - Flags: review?(blake)
There's not much point to checking this in until something else gets broken. pch likes to do that. I recommend that this bug depends on 213228.
I believe you should use the preprocessor for setting that version in the files. We'll probably move to a scheme like that in Seamonkey as well in the future (at least for locale, but I believe it would be the best way for themes as well). This way, you have to flip only one setting in one files and get all the relevant contents.rdf changed to fit it.
Is there an open bug for using the preprocessor?
If there isn't a bug for preprocessor, let's make it valid for all *Version strings, like localeVersion (or are these the only two?). A possible summary which probably needs a little refinement: Set all (skin|locale|*)Version strings for Firebird via preprocessor
Jeff: If I was a fb developer, I'd go for that solution ;-) And yes, skinVersion and localeVersion are the only two such strings.
FYI: mscott now resolved bug 222783 for TB with the preprocessor-based approach, and filed a patch in bug 224514 to get toolkit/ use this as well. You might want to make your changes based on that work :)
That bug/patch does not provide a single location to change skinVersion, just localeVersion. A new patch should probably be created after it lands anyways.
Comment on attachment 133376 [details] [diff] [review] Newer patch that gets rid of bug 215328 conflicts Yup, attachment 133376 [details] [diff] [review] is obsolete now. I have to confess that as I don't know much about C++ or makefiles in general I can't figure out how this whole preprocessor thingy works just by looking at the checkin, so I probably won't be making a fix for this. I'll keep on it though in case I do get something going.
Attachment #133376 - Attachment is obsolete: true
Attachment #133376 - Flags: review?(blake)
alanjstr: True, it doesn't include skinVersion, but it shows how the preprocessor approach works. Jeff: It shouldn't be too hard to do, someone just has to look into it...
oops, I just realized that mscott's approach is exactly my approach of using .in files, and not the XUL preprocessor approach. Sorry for the confusion...
Scott uses .in files and allmakefiles.sh and calls it "leveraging the pre-processor": http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=11%2F02%2F2003+20%3A58&maxdate=11%2F02%2F2003+21%3A01 http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=11%2F03%2F2003+19%3A51&maxdate=11%2F03%2F2003+19%3A51 What's the connection between allmakefiles.sh and the preprocessor? Is there any at all?
There shouldn't be any connection of those, no... Files using the preprocessor only get marked with a * mark in the jar.mn file.
Following in the steps of the method used to automate localeVersion, I have edited my local CVS repository so that all skinVersion info in /toolkit and /browser comes from one source (the implement is a little kludgy, but it's probably simple to fix), just as localeVersions come from milestone.txt. I'm trying to make a patch right now. Since we want to de-sync Firebird's skinVersion from Mozilla's skinVersion, I added a few new files to access/store skinVersion. Also, in order to get some of the other files preprocessed, I had to dele the original *.rdf and add in a *.rdf.in (and along with that add in some Makefile.in's where necessary). The problem now is that I can't get diff reports for files that aren't part of both repositories (aka the files I added/deled). What syntax would I use to make a patch that would cover files in one repository or the other (and would work for a read-only CVS login - the real problem behind this)? I tried the diff -uN syntax, but that didn't work, and cvs -n diff... to (by my understanding) emulate a forbidden command was similarly fruitless. If creating such a patch with my permissions is impossible, I'll probably end up posting a zip file of all the edited files with instructions as to where they go and what to dele - but that's hardly ideal. A little help with this is appreciated.
Jeff: This way of doing it works, but it's not the one that should really be used if someone figures out how to use the XUL preprocessor to do it correctly. I'd be more than happy to see how it can be done, as I'd change Seamonkey to this way of doing things as well (as the XUL preprocessor is now available in Seamonkey as well)... Anyone here who knows about that XUL preprocessor and how to use it?
Ok, how about the stuff that just went in for bug 226377? Is that this?
alanjstr: It's not this, but in that bug bsmedberg and I discussed that one of us (we'll see who has the time earlier) will do it the XUL preprocessor way for Seamonkey soon. Bug 226377 should be some help for FB/TB already, but it's not where we want to go in the end, yet.
Flags: blocking0.8?
Attached patch XUL preprocessor-based approach (obsolete) — — Splinter Review
I've been digging into the code, looking through make-jars.pl and preprocessor.pl to figure this all out. I'm absolutely sure the syntax for parsing variables is okay within the RDF files. That's relatively easy to determine as you look through the preprocessor code. As for actually parsing the variables and getting the variables to parse in preprocessor.pl, that's a little more uncertain, although not a whole lot. preprocessor.pl is referenced in only two relevant lines in mozilla/config/rules.mk. It's called when running make-jars.pl, which is called in only two relevant spots, both on the same lines in rules.mk. Passing through those lines, the only differences are irrelevant, so we can look at either set of options. The parsable variables list must be specified somehow to preprocessor.pl, and looking at the line it's clear "$(DEFINES) $(ACDEFINES)" is what does it. A few LXR searches show ACDEFINES is set to @MOZ_DEFS@. This is parsed by the C preprocessor into $DEFS, an automatically-set C preprocessor variable. $DEFS is populated, from what I can tell from autoconf docs, by AC_SUBST(). By adding in the Firebird skinversion variable to the same place as some of the Mozilla chrome variables are, it will get processed. Adding a line to configure.in to get the Firebird skinversion variable parsed gets it to preprocessor.pl's line of sight. Adding in the relevant *s in jar.mn's gets the preprocessor.pl to work on them. I might be wrong on all this, but I doubt I am. Could someone test this because none of us knows exactly how the preprocessor is set up? Then perhaps we can get it in before 0.8.
bsmedberg, could you look if this patch here doesn it the way the XUL preprocessor should be used? If yes, you could even give him a r= - and Seamonkey should do it that way as well ;-)
Comment on attachment 136906 [details] [diff] [review] XUL preprocessor-based approach Does this patch do what you expect? Looking at it I would expect you want an AC_DEFINE in the configure script, not an AC_SUBST
A look back at the autoconf documentation shows that AC_DEFINE or AC_DEFINE_UNQUOTED is what we want. It puzzles me how I came up with AC_SUBST instead of AC_DEFINE for populating DEFS. Because we probably want to keep all these chrome variables stored together instead of randomly interspersed throughout the code, we will want to set skinVersion in chrome-versions.sh. This would mean we'd need the _UNQUOTED version of AC_DEFINE(), because the regular one only accepts unparsed strings and not shell variables. If C preprocessor variables and shell variables share the same namespace (probably don't, but I don't know), then we'd have to make two changes - one to chrome-versions.sh and one to configure.in. For clarity's sake it probably makes sense not to reuse the same name, anyways. The changes would be something along the lines of: In configure.in in place of the discussed line: +AC_DEFINE_UNQUOTED(FIREBIRD_SKIN_VERSION,$FB_SKINVER) In chrome-versions.sh in place of the suggested change: +FB_SKINVER=tk0.1 Anyways, here's a new patch that includes these changes. It's also updated for a conflict found during a merge of the Inspector jar manifest with an updated tree. Since someone else has now looked at it, I'll put the revised version up for review - maybe it'll (finally) get in before 0.8.
Attachment #136906 - Attachment is obsolete: true
Comment on attachment 137030 [details] [diff] [review] XUL preprocessor-based approach v2.0 The issue of AC_SUBST instead of AC_DEFINE mentioned two comments ago has been fixed in configure.in. I don't know if there's a better (looking) way to set the skinVersion variable in the same place as the other Mozilla chrome variables, but this way should work.
Attachment #137030 - Flags: review?(blake)
-> 0.9
Target Milestone: Firebird0.8 → Firebird0.9
Comment on attachment 137030 [details] [diff] [review] XUL preprocessor-based approach v2.0 I finally got Firebird building on my system, but __FIREBIRD_SKIN_VERSION__ is replaced with an empty string. I need to look some more to figure out how to get the variable into the preprocessor's view.
Attachment #137030 - Attachment is obsolete: true
Attachment #137030 - Flags: review?(blake)
moved to 0.9 by ben, minusing
Flags: blocking0.8? → blocking0.8-
After backtracking from DEFINES and ACDEFINES, changing the location of the code to let the preprocessor see FIREBIRD_SKIN_VERSION, and building a couple times at about 3.5 hours each (overnight, so not a time problem), I finally got substitution to work. This patch still keeps the value in chrome-versions.sh (good for keeping all the version stuff for Mozilla/FB together), but the variable has to be referenced in the configure script and added to confdefs.h. I checked every file with the skinVersion expansion after building within the correct jar file for each, and all had correctly expanded. The patch also provides a clear model for doing the same thing for localeVersion in Firebird as well. If there's a relevant bug, I'd be happy to save some people some time and make a patch for that as well (in a little bit, tho - I'm going on vacation until after the new year). Once again: this patch has been tested and works (substitution, at least - see http://forums.mozillazine.org/viewtopic.php?t=42069 for why I can't test a working build). Will set a review flag in a minute...
Comment on attachment 137851 [details] [diff] [review] Preprocessor solution that really works! Everything substituted correctly for one build I made, but I didn't actually test it. A later build I made failed for reasons mentioned above, so I can't test how this works; I just know the substitution is correct. The skinVersion set by this patch is "tk0.1". It's easy enough to change in the patch, tho, if another string is desired.
Attachment #137851 - Flags: review?(bugs)
An updated patch for the trunk that changes the skinVersion for the recent trunk addition of Firebird Help (http://firebirdhelp.mozdev.org/), too. (I made sure it changed the jar.mn file that makes the edited Help files be preprocessed.) If by any obscure, minuscule chance this patch suddenly becomes needed for the 0.8 branch, I can make one given half a day's notice (I'm on EST - if it's overnight I probably can't tho) if the previous patch doesn't work.
Attachment #137851 - Attachment is obsolete: true
Attachment #137851 - Flags: review?(bugs)
Comment on attachment 138835 [details] [diff] [review] Updated patch, addresses new Firebird Help chrome Requesting review from Ben...
Attachment #138835 - Flags: review?(bugs)
The previous patch had the name FIREBIRD_SKIN_VERSION throughout. Firebird is no more. I also realized that the mozilla/toolkit changes would affect Thunderbird too: I know the toolkit/components/help code at the very least is common to both Firefox and Thunderbird, so this would most likely at least partially break Thunderbird's frontend (in other places if not in the non-existent help). The variable name has been changed from FIREBIRD_SKIN_VERSION to TK_SKIN_VERSION to reflect its commonality to the whole toolkit. I'm going to create a second patch (my primary tree doesn't include mozilla/mail) for the few Thunderbird files that use this (I'm only seeing something like 6 TB-specific and not part of mozilla/toolkit). I believe all such files are in mozilla/mail, but I'm not certain. I'll probably ask for a review of that patch by mscott and ask if he sees anything that's likely to break or not with this patch and the following Thunderbird patch. Perhaps with a concerted effot in the toolkit as a whole we can finally get this fixed.
Attachment #138835 - Attachment is obsolete: true
Attachment #138835 - Flags: review?(bugs)
I think these are all the Thunderbird changes (excluding the ones to mozilla/toolkit, covered in attachment 142411 [details] [diff] [review]). They're all the files in mozilla/mail that contain the string skinVersion. I'm pretty certain mozilla/mailnews is only Mozilla Mail in terms of FE, so I think I got everything. Will ask mscott for review in a minute...
Comment on attachment 142411 [details] [diff] [review] Updated, changed preprocessor variable name Here's a new skinVersion patch, and this time it (along with the second patch which I'm having mscott review) takes into account the whole of Thunderbird as well as Firefox. For this reason the new preprocessor variable is named TK_SKIN_VERSION. Just a note - this will need to be checked in before attachment 142412 [details] [diff] [review] (the Thunderbird patch for this), as this patch is the one that sets TK_SKIN_VERSION to be initialized.
Attachment #142411 - Flags: review?(bugs)
Comment on attachment 142412 [details] [diff] [review] Thunderbird changes in mozilla/mail only This patch changes skinVersion in Thunderbird to use a preprocessor variable, which should hopefully ease the process of changing skinVersions when necessary, making the themers happy. The skinVersion is set in one location, and this patch references that location (which is only updated when attachment 142411 [details] [diff] [review] is applied to the tree). I hit everything in mozilla/mail, but as I'm not at all familiar with the exact layout of code for Thunderbird in source, I might have missed something outside that directory. However, I did get everything in mozilla/mail that contained "skinVersion" (along with the corresponding jar.mn's).
Attachment #142412 - Flags: review?(mscott)
Jeff: You shouldn't patch configure, you should do this in configure.in - configure will be updated automatically with automake. See my patches in bug 232011 and bug 234014 as a reference. As Firefox and Thunderbird are still using a bunch of files from Seamonkey IIRC, it might be better to reuse MOZILLA_SKIN_VERSION, I believe. But of course, that's completely up to you Firefox/Thunderbird people. What you should do as well (and it might be good to consider doing that at once, in one patch) is to use the XUL preprocessor for localeVersion as well. This would help us to resolve the minor but global issue of bug 235325.
I agree that it needs to be a variable, but does it have to be defined as only "toolkit skin version"? In other words, why not use MOZILLA_SKIN_VERSION and just set a different value? I'm seeing a lot more +'s and -'s due to whitespace differences.
Please note: the skinversion of the toolkit and the skinversion of the apps should be different variables... once we get the toolkit fully-separated from the apps, we're going to want to bump the toolkit skinversion much less often than the app. This probably won't work yet, because everything is too intertwined.
Attachment #142411 - Flags: review?(bugs)
Comment on attachment 142412 [details] [diff] [review] Thunderbird changes in mozilla/mail only Given that this is how I was doing it in the last couple patches, it's nice to finally know I was doing it wrong. The issues brought up are a little more than I understand (although it sounds like only a few are actually real issues), so I'm just removing the review requests and but not obsoleting the attachments. They'll be here for reference, but I'm not likely to try another time, because I obviously don't know enough about what I'm trying to do to actually do it correctly.
Attachment #142412 - Flags: review?(mscott)
Ok, so there is a need for a separate name, in which case MOZILLA_SKIN_VERSION would still be defined. TK_SKIN_VERSION should be defined in the same way. So how is MOZILLA_SKIN_VERSION done?
Attachment #142411 - Attachment is obsolete: true
Comment on attachment 142412 [details] [diff] [review] Thunderbird changes in mozilla/mail only Oops. Mid-air collision.
Attachment #142412 - Attachment is obsolete: true
alanjstr: I just checked in MOZILLA_SKIN_VERSION a couple of hours ago, so to see how it is implented, just look into bug 234014. bsmedberg: OK, that means we should add a TOOLKIT_SKIN_VERSION (or similar name), what about localeVersion in this regard? Should there be an additional one as well for toolkit? If we end up that only two (or a small bunch of) contents.rdf in browser/ will use the same version, everything else a different one, then it should probably be hardcoded into those files. Replacing at build time is only suitable if we have a lot of files across the tree that use the same version (as currently in Seamonkey).
Assignee: firefox → bugs
Status: REOPENED → NEW
Priority: -- → P1
Can we go forward with TOOLKIT_SKIN_VERSION?
Since we need new themes for 0.9's new theme/extension manager, we should really consider to bump the skin version.
Flags: blocking0.9?
The new extension mechanism (by which skins and locales are installed) doesn't need or use skinversion or localeversion any more. We are going to be removing skinversion and localeversion checks completely.
Then we can remove skinVersion and localeVersion from the contents.rdf files in browser and toolkit as well?
Flags: blocking0.9?
> Then we can remove skinVersion and localeVersion from the contents.rdf files in > browser and toolkit as well? I haven't actually disabled skinVersion and localeVersion yet... when I do, then we can clean up the RDF.
Since they're pointless, shouldn't this be WONTFIX and a new bug for cleanup be created?
Change target to FF 1.1 or mark as wontfix (per comment #70) ?
This is now INVALID, contents.rdf is no more!
Status: NEW → RESOLVED
Closed: 22 years ago → 20 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: