Bug 556644 and others implemented omnijar support for Firefox, we should do the same for Thunderbird. Bug 601571 is adding the required comm-central build-config changes, this bug will do the specific app changes.

Comment on attachment 480591 [details] [diff] [review] WIP Pressed enter at the wrong time... This patch adds initial support. It doesn't cover windows installer issues (yet). The main problem I have at the moment is this error: JS Component Loader: ERROR (null):0 uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: resource:///modules/quickFilterManager.js :: <TOP_LEVEL> :: line 62" data: no] JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: chrome://messenger/content/quickFilterBar.js :: <TOP_LEVEL> :: line 42" data: no] That's basically saying that: Cu.import("resource://app/modules/gloda/indexer.js"); is not found, although it exists in the right place in omni.jar. I'm wondering if for some reason omnijar isn't supporting resource://app/modules right. I tried a few combinations including jar:resource://app/modules/gloda/indexer.js but they seemed to turn out as invalid URI. Ideas welcome, otherwise I'll do some more debugging soon.

I thought we had determined resource://app/ wasn't a real thing and removed all such references, replacing them with resource:///? I think none of the app references worked during the startup phase at all, but thanks to either a change in behaviour of the error handling logic or because we had added a chrome manifest entry to shut the error up, it would work in steady state.

If resource://app/ can be replaced with resource:///, it would be best. resource:/// gets pointed to either the app dir or omnijar, depending on whether it exists. See http://hg.mozilla.org/mozilla-central/file/523a116d617f/netwerk/protocol/res/nsResProtocolHandler.cpp#l174 and the function below it for how it's done. There is nothing builtin for resource://app/

This patch now passes all the xpcshell-tests, the MozMill tests are still broken however.

I've split this patch into two parts - the first is for the changes we need to the existing code base so that omnijar builds will run properly. The second will be for the actual omnijar change and for the required packaging updates. Therefore we can get the first part in now, and do the second part once the core patch has landed.

Note: I'm still investigating one or two apparent test failures with omnijar, but currently I believe they are probably not a result of part 1.

This is the patch that'll switch omnijar on, but currently is lacking some packaging details, and confirmation to see if the last test failures are just random or not.

Outstanding test issues: - test_viewFolderWrapper.js appears to fail on a 32/64 bit Mac build when run on 32 bit Mac (I don't understand this yet, will try and reproduce locally). - test_handlerService.js fails on Linux (both 32 & 64).

There was an uninitialised rv in CopyMailViewsFileFromOmnijar that was causing the failure in test_viewFolderWrapper.js. Updated the patch to fix that. My guess is that the Linux failures are separate as they are to do with the handler service and not with mail views loading.

Can we simply not package up mailViews.dat in the omnijar? I think there's a way to exclude particular files from being packed, viz. https://bugzilla.mozilla.org/attachment.cgi?id=470525&action=diff#a/toolkit/mozapps/installer/packager.mk_sec1

(The patch looks fine otherwise, except that I think all the ...Native calls should be replaced by their Unicode versions.)

(In reply to comment #11) > Can we simply not package up mailViews.dat in the omnijar? I think there's a > way to exclude particular files from being packed I'm torn both ways, but I think in the spirit of omnijar including the file in the jar is probably slightly more appropriate.

Comment on attachment 489509 [details] [diff] [review] Part 1 v2 >+ifeq ($(MOZ_CHROME_FILE_FORMAT), jar) >+DEFINES += -DJAREXT=.jar >+else >+DEFINES += -DJAREXT= >+endif >+ [snip] >-@BINPATH@/chrome/@AB_CD@.jar >+@BINPATH@/chrome/@AB_CD@@JAREXT@ So package-manifest.in's used to tell which files/directories are going to be packaged up during make package? >+nsresult nsMsgMailViewList::CopyMailViewsFile() >+ >+ rv = defaultMessagesFile->AppendNative(nsDependentCString("mailViews.dat")); Unicode call please. >+ >+ // get the profile directory Since you've capitalized a couple of comments below, you should capitalize this too :) >+ // now copy the file over to the profile directory and this >+ return defaultMessagesFile->CopyToNative(profileDir, EmptyCString()); Unicode call please. Also I'm wondering whether an explicit Exists check here would be worthwhile. >+nsresult nsMsgMailViewList::CopyMailViewsFileFromOmnijar() >+ // First look for the file in the omnijar >+ nsZipArchive* jarReader = mozilla::OmnijarReader(); >+ if (!jarReader) Can this ever reasonably happen? If it can't, then please add an NS_ERROR here. >+ >+ rv = outFile->AppendNative(NS_LITERAL_CSTRING("mailViews.dat")); Unicode call please. >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsILocalFile> localFile(do_QueryInterface(outFile, &rv)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ PRFileDesc *fd; >+ rv = localFile->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE, >+ item->Mode(), &fd); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCAutoString path; >+ rv = outFile->GetNativePath(path); Unicode call please (also see the comment right below this one). While I don't think fixing the other calls is necessary correctness-wise, this one's important because it points to a file inside the user profile and usernames can easily be non-ASCII. >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ return jarReader->ExtractFile(item, path.get(), fd); So. ExtractFile is at <http://mxr.mozilla.org/comm-central/source/mozilla/modules/libjar/nsZipArchive.cpp#397>. Now path is going to be UTF-16 after the change above, so you think you might get away by converting it to UTF-8 before passing it in... however, at least on Windows that PR_Delete call ultimately resolves to ::DeleteFileA, and according to <http://www.eggheadcafe.com/software/aspnet/35950632/deletefile-shows-as-deletefilea.aspx>, ::DeleteFileA has issues with UTF-8. Looks like this needs to somehow be fixed in core, but right now this is going to lead to problems. >+ // If the file doesn't exist, we should try to get it from the defaults >+ // directory and copy it over >+ PRBool exists = PR_FALSE; >+ file->Exists(&exists); rv check please. I'm minusing because of the ExtractFile call.

(And yes, assuming those issues with UTF-8 are true, this basically implies PR_Delete's broken on Windows.) I just realized that if you don't package up mailViews.dat, you'll avoid this problem. :)

(In reply to comment #14) > Comment on attachment 489509 [details] [diff] [review] > Part 1 v2 > > >+ifeq ($(MOZ_CHROME_FILE_FORMAT), jar) > >+DEFINES += -DJAREXT=.jar > >+else > >+DEFINES += -DJAREXT= > >+endif > >+ > [snip] > >-@BINPATH@/chrome/@AB_CD@.jar > >+@BINPATH@/chrome/@AB_CD@@JAREXT@ > > So package-manifest.in's used to tell which files/directories are going to be > packaged up during make package? Yep, exactly right. (In reply to comment #15) > (And yes, assuming those issues with UTF-8 are true, this basically implies > PR_Delete's broken on Windows.) > > I just realized that if you don't package up mailViews.dat, you'll avoid this > problem. :) I'm currently going for that method, which does require a core fix but that seems small and hopefully we can get it in easily.

Much simpler this time - we keep mailViews.dat separate from omni.jar to avoid the unicode issues. This needs the patch from bug 615850 applying to core for it to work. Try server passed on Windows and Mac with this patch, the bug 615850 patch, and the current part 2 patch. Linux still fails in two xpcshell-tests which I think we mentioned before, but we may as well get this bit landed when we can as it'll fix the resource URLs.

Comment on attachment 494374 [details] [diff] [review] [checked in] Part 1 v3 >diff --git a/mail/installer/Makefile.in b/mail/installer/Makefile.in >--- a/mail/installer/Makefile.in >+++ b/mail/installer/Makefile.in >@@ -89,6 +95,8 @@ ifndef MOZ_ENABLE_LIBXUL > $(error you need an "--enable-libxul" build to package a build) > endif > >+NON_OMNIJAR_FILES = defaults/messenger/mailViews.dat ... >diff --git a/mail/locales/Makefile.in b/mail/locales/Makefile.in >--- a/mail/locales/Makefile.in >+++ b/mail/locales/Makefile.in >@@ -85,6 +85,8 @@ UNINSTALLER_PACKAGE_HOOK = $(RM) -r $(ST > $(NULL) > endif > >+NON_OMNIJAR_FILES = defaults/messenger/mailViews.dat Do we really need this twice? Other than that, looks good.

(In reply to comment #18) > >diff --git a/mail/installer/Makefile.in b/mail/installer/Makefile.in > >+NON_OMNIJAR_FILES = defaults/messenger/mailViews.dat > >diff --git a/mail/locales/Makefile.in b/mail/locales/Makefile.in > >+NON_OMNIJAR_FILES = defaults/messenger/mailViews.dat > > Do we really need this twice? Other than that, looks good. Yes, the locale repacks unfortunately need to know not to repack that file as well.

Comment on attachment 494374 [details] [diff] [review] [checked in] Part 1 v3 I've landed part 1 as it fixes the incorrect resource url (not that that is breaking anything at the moment) and just gets this patch landed out the way. The mailViews.dat bit won't work until the core patch gets landed, but this doesn't matter much anyway as we've not enabled omnijar yet. Checked in: http://hg.mozilla.org/comm-central/rev/d79b63175724

This should be all that is required to switch us over to omnijar - We'll need bug 616520 as well (so that MozMill tests still run on Linux), but that should land within a day or so anyway. This version of the patch also fixes the previous issues we were seeing with the linux xpcshell-tests failing - because components.manifest wasn't being packaged, it then couldn't be used to generate the binary components list, and hence the binary components weren't being loaded. So the main things in this patch are to remove the unnecessary packaging of interfaces.manifest, add all the files that omni.jar replaces to removed-files.in and do the actual switch. Note that the switch will also change the default chrome format to "flat", but given the advantages to Linux & Mac developers, I think that's probably a good thing. I've just thrown the whole thing at try server one last time, and I'm going to see if I can whip up some perf estimates.

Comment on attachment 496148 [details] [diff] [review] Part 2 - Main switch v2 This is good to go now. It passed all tests on try server. The removed files list I updated by downloading the current nightly builds on each platform and comparing with the equivalent omnijar try server build. I also cross-referenced with the Firefox removed files list to make sure I've got everything. When this lands I'll also do a before and after on nightly build updates on all platforms just to confirm I got everything. Initial indications on my local machine show a small improvement in warm-startup. I'm hoping to get some more stats before I land this.

Comment on attachment 496148 [details] [diff] [review] Part 2 - Main switch v2 >diff --git a/mail/installer/package-manifest.in b/mail/installer/package-manifest.in >--- a/mail/installer/package-manifest.in >+++ b/mail/installer/package-manifest.in >@@ -112,13 +112,15 @@ > > @BINPATH@/isp/* > >+; Although components.manifest ends up being shipped inside omni.jar >+; it still needs listing here as it is parsed for various binary >+; components before being packaged inside omni.jar. > @BINPATH@/components/components.manifest > @BINPATH@/components/aboutRights.js > @BINPATH@/components/activity.xpt > @BINPATH@/components/activityComponents.manifest > @BINPATH@/components/addrbook.xpt > @BINPATH@/components/fts3tok.xpt >-@BINPATH@/components/interfaces.manifest Per discussion on IRC, please replace the somewhat confusing comment about components.manifest with a comment explaining why interfaces.manifest is not present. Looks fine otherwise.

Checked in: http://hg.mozilla.org/comm-central/rev/2acce7110433

(In reply to comment #22) > When this lands I'll also do a before and after on nightly build updates on all > platforms just to confirm I got everything. I just did that check and found a few things that were missing from removed-files.in, fixed in: http://hg.mozilla.org/comm-central/rev/8bbeb8adbfca