seen on Thunderbird linux trunk build 2005-04-26-05-trunk -download and install on or two Thunderbird profiles using the Theme manager. notice that on completion of install all theme icons are an open letter. also note the message "708090-lite will be installed the..." for each new theme -select a new theme, then click Use theme. the message changes to "Restart Mozilla Thunderbird to..." -restart Thunderbird. The selected default theme remains in use. The message "708090-lite will be installed the..." for each new theme still exists.

This bug is also happening with the Extensions Manager.

This bug is also afflicts Thunderbird Windows trunk build 2005-04-26-08-trunk

Affects Firefox trunk as well. Seen on Windows Fx build 2005-04-26-07-trunk Themes do the same thing. However, extensions don't appear at all. When trying to restart after installing themes and extensions, I got dialogs about cancelling downloads.

Probably fall out from Bug #286034

Fallout from bug 286034 (and probably a dupe)

This also occurs with Firefox and this is more than likely fixed by the patch for bug 291666

*** Bug 292029 has been marked as a duplicate of this bug. ***

duping against the bug that fixed this. *** This bug has been marked as a duplicate of 291666 ***

The extensions part of this bug is fixed by bug 291666 (except on Mac). However, themes remain broken as originally describe. Seen on Linux Thunderbird trunk build 2005-04-29-04-trunk, Windows Firefox trunk build 2005-04-29-07-trunk and Mac Firefox build 2005-04-29-07-trunk reopened and adjusted summary to cover just theme installation

(In reply to comment #9) > The extensions part of this bug is fixed by bug 291666 (except on Mac). However, > themes remain broken as originally describe. Seen on Linux Thunderbird trunk > build 2005-04-29-04-trunk, Windows Firefox trunk build 2005-04-29-07-trunk and > Mac Firefox build 2005-04-29-07-trunk > > reopened and adjusted summary to cover just theme installation I have a patch for this that does work as stated in bug 292248 and should have it ready by this evening. Themes currently will install as long as they meet the requirement of having a chrome.manifest as stated in bug 291666 and there are several other issues with theme installation the patch fixes. > taking

I see this bug on the 4/29 trunk Mac build. If it's occurring on both Mac and Windows Firefox trunk builds, shouldn't the "Hardware" field be changed to "All"?

Tracy does bug 289460 fundamentally differ from this one, or can it be duped (or visa versa) ?

bug 289460 should probably have been marked WFM some time ago. In this bug, the theme seems to install initially. Then it doesn't take when selected for use and app restarted. So I'd say the two bugs are different.

Ben, is this fallout from your recent changes? We need to get this cleared before we ship 1.8b2/1.1a

This is the theme portion of one of the patches in progress from bug 292506. It includes the patch from bug 251026 that is already r+=bsmedberg & sr+=darin. There are several other issues fixed in the current patch in bug 292506 and I was able to apply this patch cleanly after applying the one from bug 292506. This will allow the installation of themes with or without chrome.manifest files. One item to note is that it fixes an issue with where Themes with chrome.manifests will break. Since xpinstall uses CreateUnique when saving a theme to the profile's chrome dir if there is already a jar file with that name there the EM has no way of knowing the correct jar file name which is hardcoded in the chrome.manifest.

Benjamin - this is the bug we discussed briefly regarding theme breaking due to xpinstall renaming the jar files in the case of an upgrade or a reinstall due to a copy being left behind in the profile's chrome dir. This bug also fixes the EM code so that it will allow themes that do not have a chrome manifest to install. It works as is but I would like to also remove the calls to ProcessContentsManifest and CheckForNewChrome in nsSoftwareUpdateRun.cpp so there would no longer be a case where a jar file is left behind on theme install. I don't see a use case where ProcessContentsManifest and CheckForNewChrome would ever be used here since the EM handles it and thought you would know of one since you touched that section of code last. http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsSoftwareUpdateRun.cpp#678 Also, I don't know if / when Ben is going to have time to review this and wondered if you would? This bug and bug 292506 take care of a lot of significant issues with the EM which would be good to get into the alpha.

Updated to remove the patch from bug 292362 that snuck in here

Comment on attachment 182576 [details] [diff] [review] patch I never added support for theme chrome.manifests for a reason: the manifests ought to contain relative URLs, but they are extracted from the JAR file which means that the relative URLs are wrong. All this stuff with the leafname is a bad workaround, because you have to assume that the manifest is being extracted to somewhere in particular. I'm not sure what to do about this. And yes, it does make sense to remove the if (!installed) block from the xpinstall code.

(In reply to comment #18) > (From update of attachment 182576 [details] [diff] [review] [edit]) > I never added support for theme chrome.manifests for a reason: the manifests > ought to contain relative URLs, but they are extracted from the JAR file which > means that the relative URLs are wrong. All this stuff with the leafname is a > bad workaround, because you have to assume that the manifest is being extracted > to somewhere in particular. I'm not sure what to do about this. I am unsure how this is a problem with an EM initiated install... can you please explain after taking the following into consideration? The installation including the directory structure of an extension or a theme is dictated by an EM installation prior to the new code as well as today. The directory structure being: Install-location/{GUID}/chrome.manifest Install-location/{GUID}/install.rdf Install-location/{GUID}/chrome/theme.jar where Install-location is either bin/firefox/ or <profile> http://wiki.mozilla.org/Firefox:1.1_Extension_Manager_Upgrades So, the urls are relative and correct in relation to Install-location/GUID/. There isn't an assumption as to where the chrome.manifest will be extracted to since it is always extracted to Install-location/{GUID} which is created by the EM on install. Also, if it is an assumption as to where it is extracted to when we always extract it to Install-location/{GUID}/ how can it not be an assumption to always create it in the Install-location/{GUID}/ directory as the code you wrote to upgrade the chrome does? The only issue with an EM initiated install then becomes the reliance on xpinstall in regards to the file name. I am using the same data and method to get the leafName as xpinstall does in GetDestinationFile::GetDestinationFile... the only difference is that I am passing this value to installItemFromFile. http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsXPInstallManager.cpp#825 A couple of alternatives I considered for dealing with the file being renamed by xpinstall is to always create a chrome.manifest and to either change the jar file name in the one provided or create one if one isn't provided. Both of these seem like a bad workaround once all of the previous points are taken into consideration.

When the chrome.manifest is in the JAR file, the relative URIs would be skin package skinname path/inside/jar/ When the chrome.manifest is extracted, these paths change: skin package skinname jar:chrome/theme.jar!/path/inside/jar/ I do not particularly want skin authors to go writing manifests that are not actually correct until you unpack the JAR in a particular way, so I was not going to support flat manifests in theme JARs for the time being. I could be convinced to change my mind if an elegant solution to the above could be found... perhaps a "baseuri" instruction in the flat manifest.

(In reply to comment #20) > When the chrome.manifest is in the JAR file, the relative URIs would be > > skin package skinname path/inside/jar/ > > When the chrome.manifest is extracted, these paths change: > > skin package skinname jar:chrome/theme.jar!/path/inside/jar/ I still don't see the problem since the paths that are written in the chrome.manifest file do not change just because the file is extracted and there is no code I know of that changes it which has also been confirmed by my testing. Is there any code that reads a chrome.manifest while it is inside of an xpi or a jar file in order to get the relative paths that it contains? Whether the chrome.manifest file is inside of a jar or an xpi file it is always extracted to Install-location/{GUID}/ and it always contains paths like so when it is referencing a jar file: skin package skinname jar:chrome/theme.jar!/path/inside/jar/ In what use case would your first example be applicable to a theme jar file installation? The only possible case I can see is being able to install a jar file unpacked which seems like a fringe case and it should be possible to accomplish this with a theme installation using an xpi. All this is doing is placing the same format requirements for a theme's chrome.manifest as there is for an extension's chrome.manifest which I believe has always been the case. The only significant difference between an extension and a theme in regards to the chrome.manifest is that with an extension the chrome.manifest is inside of an xpi. In the case of a theme the chrome.manifest is inside of the jar file itself. In both cases they are extracted to the exact same location just like the install.rdf is extracted and in both cases the jar file is placed inside of a chrome directory inside of the same directory as the chrome.manifest and the install.rdf. > I do not particularly want skin authors to go writing manifests that are not > actually correct until you unpack the JAR in a particular way, so I was not > going to support flat manifests in theme JARs for the time being. I could be > convinced to change my mind if an elegant solution to the above could be > found... perhaps a "baseuri" instruction in the flat manifest. There is no unpacking of the jar but there is extraction of the chrome.manifest to Install-location/{GUID}/ just as there is extraction of the install.rdf to this same location. This is true for both extensions and themes and In both cases the relative paths are correct when using the format as documented and provided above.

Regarding "baseuri" - inside the extensions-startup.manifest there is already app-global and app-profile providing the info as to where the Install-location is as well as the GUID like so. app-profile {GUID} extensions/{GUID} 1115058404 Inside the Install-location/{GUID}/ there is the chrome.manifest which contains the relative paths from that location to the jar file, etc. like so: skin package skinname jar:chrome/theme.jar!/path/inside/jar/

This patch includes the removal of the no longer needed code from nsSoftwareUpdateRun.cpp to remove another case where a jar file would be left behind in the profile's chrome dir as well as a simple one liner to extensions.js to fix a bug with uninstalling a theme that is in use. In thinking on the comments the only desire I can see for being able to specify relative paths for theme jar files in the format of 'skin package skinname path/inside/jar/' is during theme development or possibly when actually packaging a theme but I don't believe it is any more of a hardship for theme developers as it is for extension developers while keeping the two formats consistent. You just have to specify the name of the jar file in the chrome.manifest that you add to the root of the jar file as you have to specify the name of the jar file in the chrome.manifest that you add to the root of the xpi file.

If the general process used for installing themes prior to 4/26 or in the attached patch is not acceptable then I suggest the following process. Themes with a chrome.manifest: If the theme has a chrome.manifest in the root of the archive during the installation the chrome.manifest would be extracted then parsed and if it has any lines that specify anything other than a skin package an alert would be displayed similar to the malformed rdf alert and the theme would not be installed. The directory structure would be similar to what extensions have. This will allow extracting the jar directly into the extensions directory to install it as can be done with extensions. This would also allow for the contents of the chrome directory to be flat file instead of archive based. It would be simple enough to limit the extracted directory structure for themes to that as outlined below and thereby prevent a components directory or other additional files besides those located in the chrome directory if there are any concerns of this happening. I would also suggest placing the preview and icon images in the root in order to lessen the hassles caused by zip readers holding the jar file open when an upgrade or an uninstall occurs. package.jar chrome.manifest chrome/theme.jar icon.png install.rdf preview.png Themes without a chrome.manifest: The chrome can either be upgraded on install as is done with builds before 4/26 as well as the patch and thereby provide backwards compatibility or the packaging requirements as outlined above could be mandatory. I have already coded the above process and I'm very sure that I can provide a patch to meet the requirements outlined above or a variation thereof in less than a day and if I am not busy in a few hours. I think the above should address Benjamin's concerns about relative paths in the chrome.manifest since it would bring themes up to parity with extensions in regards to chrome.manifests. Regarding the initial concern of the manifest being extracted to somewhere in particular I believe this is a non-issue since the code always extracts it to the same location when using any of the methods proposed in the same manner that it already does this with the install.rdf.

ok, that sounds like a decent proposal. Let me talk with Ben, but you can presume that this is ok unless you hear otherwise. We definitely want to maintain compatibility with the existing structure. It would be good to write up some docs about this for one of the wikis; I'm not sure where, exactly.

Comment on attachment 182605 [details] [diff] [review] updated patch >+ upgradeThemeChrome: function() { >+ var manifestFile = this._installLocation.getItemFile(this._id, FILE_CHROME_MANIFEST); <..> What's all of this? Are you fully up to date?

Currently, if you install a theme that does not have a chrome.manifest file using the theme manager or by dropping a theme jar file in the extensions directory it tries to install the theme but fails with the following assertions and no alert to the user. The theme's display name is listed in the theme manager though the theme cannot be used. *** installTheme: failed to extract install manifest to: <path>/{2A10B180-05EF-11D9-8C50-444553540001} *** safeInstallOperation: install operation (caller-supplied callback) failed, rolling back file moves and aborting inst allation. ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIZipReader.extract]" nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)" location: "JS frame :: file:///C:/moz/mozilla-source/moz illa/ff-debug/dist/bin/components/nsExtensionManager.js :: extractThemeFiles :: line 1267" data: no] ************************************************************ This error is not caused by not being able to extract the install manifest... it is caused by there being no chrome.manifest file. The upgradeThemeChrome function provides a path to upgrade the theme's chrome in this situation. The current patch is not up to date with the process as described. I will submit an updated patch this evening that provides the new structure as outlined and maintains compatibility with the existing structure.

If the theme has a chrome.manifest it requires the package structure as outlined and if it doesn't it attempts to upgrade the contents.rdf to a chrome.manifest. For the new structure it only extracts the install.rdf, chrome.manifest, icon.png, and preview.png if present to the root and it extracts everything under the chrome entry into the chrome dir. It doesn't care if any of these files are present since the check for an install.rdf occurs well beforehand and exits silently if not present. Perhaps a bug should be opened for this? The one thing I am not happy with is the way I shimmed in the error code when a theme has a chrome.manifest that has entries other than skin. Without this check at the start an upgrade isn't rolled back to the previous version in part due to the extensions.rdf already having new data and for new installs some data is left behind in the extensions.rdf as well. I have successfully tested installing by dropping in a file, dropping in an extracted new structure theme dir, and with the themes manager with the new structure and by dropping in a file and with the themes manager with the old structure as well as several failure conditions. So far, I haven't seen any issues within the scope of this bug that the patch doesn't address. It appears that by using the extracted icon.png and preview.png the need to notify chrome-flush-skin-caches has been removed so it isn't included in this patch. If you prefer the method where the icon and preview images are opened from inside the jar I can put it back that way and add the chrome-flush-skin-caches back to the patch... either way works though I am leery of the hold on them that a zip reader can have.

Comment on attachment 182768 [details] [diff] [review] patch This is getting a lot closer, but I'm still concerned about a few things: 1) Why do we need leafname now? Can't we just use whatever filename xpinstall decides, for old-style themes? New-style themes obviously don't care. 2) You don't need isValidChromeManifest... the chrome registry checks for skin packages at runtime. Other than that, this looks good.

Addresses the comments... thanks for the review.

Comment on attachment 182811 [details] [diff] [review] patch Beautiful. Lemme find a driver for quick approval.

Comment on attachment 182811 [details] [diff] [review] patch a=asa

patch checked in - thanks db48x - resolving fixed

Firefox beast-trunk 05/05/07 07:19:00 Theme install and select OK -> install.rdf+contents.rdf Fail -> install.rdf+chrome.manifest Fail -> install.rdf+contents.rdf+chrome.manifest Theme's specification is decided to not using chrome.manifest? http://www.mozilla.org/projects/firefox/extensions/packaging/themes.html

The current theme specification can't have a chrome.manifest in the root of the jar after this patch. This is in part due to that a theme's jar file name can change during install, etc. which would then break the chrome registration of the theme when using a theme supplied chrome.manifest. With the current specification it was decided that the safest method is to dynamically create a chrome.manifest from a theme's contents.rdf and I believe this is why there is no mention of using a chrome.manifest on the link you provided for the current specification. A new package structure for themes is described in comment #24 that is based on the same package structure used for extensions. This new structure is not required as you found with your successful install of a theme that had an install.rdf and a contents.rdf. I will hopefully have docs for this finished by the end of this weekend to further describe the structure but I believe the information in the comment #24 should be sufficient to be able to package a theme using the new structure.

*** Bug 293401 has been marked as a duplicate of this bug. ***

I put together http://exchangecode.com/robert/themes.html which is based on http://www.mozilla.org/projects/firefox/extensions/packaging/themes.html

(In reply to comment #37) > I put together > http://exchangecode.com/robert/themes.html which is based on > http://www.mozilla.org/projects/firefox/extensions/packaging/themes.html Thank you. This is comprehensible. I have already experimentally opened the Theme of this packaging to the public. And, two Theme creators in Japan have released the Themes by this packaging. These will be tested until 1.1 release.

Is it possible the last checkin caused bug 293514?

*** Bug 292148 has been marked as a duplicate of this bug. ***