We currently enumerate components/*.manifest and chrome/*.manifest. Enumerating directories is a known source of bad I/O during startup, and also means that manifests may be loaded in any order. Instead, we should just read the root chrome.manifest and load sub-manifests as necessary. This patch ends up with five manifests total in a packaged build: chrome.manifest -> components/interfaces.manifest -> components/components.manifest -> chrome/nonlocalized.manifest -> chrome/localized.manifest Unpackaged builds keep lots of manifests around. Oh, and I'm going to need some removed-files for trunk builds... I forgot that, but I'll catch it later.

There are a few tests I may need to clean up as well. We'll see.

Comment on attachment 457679 [details] [diff] [review] Part B - packaging and repackaging changes, rev. 1 Tests need work, they drop stuff in the appdir.

Changes: * package mochitest chrome in distribution/bundles instead of dropping a manifest into chrome/ * ship tp-cmdline in release builds

Comment on attachment 457919 [details] [diff] [review] Part B - packaging and repackaging changes, rev. 2 Looks good except that I don't see any reason to move AB_CD.manifest to localized.manifest. It looks like we can keep it as AB_CD.manifest the whole way through. If there's no compelling reason not to keep it that way, lets do that.

Comment on attachment 457678 [details] [diff] [review] Part A - XPCOM changes to load submodules and stop enumerating directories, rev. 1 >diff --git a/xpcom/components/nsIManifestLoader.idl b/xpcom/components/nsIManifestLoader.idl >deleted file mode 100644 >diff --git a/modules/libjar/nsManifestZIPLoader.h b/modules/libjar/nsManifestZIPLoader.h > #include "nsIManifestLoader.h" nsIManifestLoader.idl was removed, and this file has no references left to anything that was in this .h, but it will fail in a clobber, please remove. >diff --git a/modules/libjar/objs.mk b/modules/libjar/objs.mk > MODULES_LIBJAR_LEXPORTS = \ > zipstruct.h \ > nsZipArchive.h \ >- nsManifestZIPLoader.h \ > $(NULL) >diff --git a/xpcom/components/Makefile.in b/xpcom/components/Makefile.in >+ -I$(topsrcdir)/modules/libjar \ >diff --git a/xpcom/components/nsComponentManager.h b/xpcom/components/nsComponentManager.h >--- a/xpcom/components/nsComponentManager.h >+++ b/xpcom/components/nsComponentManager.h >-#include "nsIManifestLoader.h" >+#include "nsManifestZIPLoader.h" Even by adding libjar to the include path, don't we want to keep LEXPORTS including this if its used outside of libjar? (You did however remove the other use of it in the tree). From my skim of everything else this is a great cleanup!

Comment on attachment 457919 [details] [diff] [review] Part B - packaging and repackaging changes, rev. 2 >diff --git a/config/tests/unit-JarMaker.py b/config/tests/unit-JarMaker.py >- debug = False # set to True to debug failing tests on disk >+ debug = True # set to True to debug failing tests on disk Please remove this hunk for checkin. >diff --git a/config/rules.mk b/config/rules.mk >+EXTRA_MANIFESTS = $(filter %.manifest,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS)) >+ifneq (,$(EXTRA_MANIFESTS)) >+libs:: >+ $(PYTHON) $(MOZILLA_DIR)/config/buildlist.py $(FINAL_TARGET)/chrome.manifest $(patsubst %,"manifest components/%",$(notdir $(EXTRA_MANIFESTS))) > endif Why chrome.manifest here, doesn't it make more sense to use components.manifest instead? [and make sure components.manifest is linked in chrome.manifest as well] >diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in The hunk here looks unrelated to this bug, I always like changes to have their own bug so reasoning/followup Q's etc. can be found easily; That said, I see no reason a separate changeset/bug is needed for that [since it is already here] :-)

(In reply to comment #4) > Created attachment 457919 [details] [diff] [review] > Part B - packaging and repackaging changes, rev. 2 > ... > * ship tp-cmdline in release builds Why is this patch planning on shipping what appears to be test-only code in release builds? Surely we should be correcting the test harness to work properly with the new code...

I tested these patches on Fennec, and they do fix the component registration ordering issues. I do agree with comments about using "chrome.manifest" as the name, since chrome/chrome.manifest exists too. Using "components.manifest" sound good to me.

> Even by adding libjar to the include path, don't we want to keep LEXPORTS > including this if its used outside of libjar? (You did however remove the other > use of it in the tree). No, I don't. > >diff --git a/config/rules.mk b/config/rules.mk > >+EXTRA_MANIFESTS = $(filter %.manifest,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS)) > >+ifneq (,$(EXTRA_MANIFESTS)) > >+libs:: > >+ $(PYTHON) $(MOZILLA_DIR)/config/buildlist.py $(FINAL_TARGET)/chrome.manifest $(patsubst %,"manifest components/%",$(notdir $(EXTRA_MANIFESTS))) > > endif > > Why chrome.manifest here, doesn't it make more sense to use components.manifest > instead? [and make sure components.manifest is linked in chrome.manifest as > well] No, why would I create an extra file if I don't need to? There will just be /chrome.manifest and whatever it loads. > >diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in > The hunk here looks unrelated to this bug, I always like changes to have their > own bug so reasoning/followup Q's etc. can be found easily; That said, I see no > reason a separate changeset/bug is needed for that [since it is already here] > :-) It isn't unrelated at all: because you can no longer drop in manifests or components into the Firefox install directory, Talos will not work unless we either ship the tp-commandline file by default or somehow have special code to allow it to be dropped in. I opted to ship the -tp commandline code by default.

Comment on attachment 457678 [details] [diff] [review] Part A - XPCOM changes to load submodules and stop enumerating directories, rev. 1 >diff --git a/modules/libjar/nsManifestZIPLoader.cpp b/modules/libjar/nsManifestZIPLoader.cpp >-nsManifestZIPLoader::nsManifestZIPLoader() { >+nsManifestZIPLoader::nsManifestZIPLoader() >+ : mZipReader(new nsJAR()) >+{ >+ nsresult rv = reader->Open(mozilla::OmnijarPath()); What is "reader"? Do you mean mZipReader? Is this path tested? I also wonder if there is any gain in using the nsIZipReaderCache for stuff talking to the omnijar? >diff --git a/xpcom/components/ManifestParser.cpp b/xpcom/components/ManifestParser.cpp >--- a/xpcom/components/ManifestParser.cpp >+++ b/xpcom/components/ManifestParser.cpp >@@ -83,16 +83,18 @@ struct ManifestDirective > void (nsChromeRegistry::*regfunc) > (nsChromeRegistry::ManifestProcessingContext& cx, > int lineno, char *const *argv, > bool platform, bool contentaccessible); > > bool isContract; > }; > static const ManifestDirective kParsingTable[] = { >+ { "manifest", 1, false, true, false, Can you line this up like the others. > if (!ok || > stApp == eBad || > stAppVersion == eBad || > stOs == eBad || > stOsVersion == eBad || > stABI == eBad) > continue; > >- if (directive->ischrome) { >+ if (directive->regfunc) { > #ifdef MOZ_IPC > if (GeckoProcessType_Default != XRE_GetProcessType()) > continue; > #endif > I wonder if it would be better to just put something higher up in the loop, before all the flag checks to the effect of: if (!directive->ischrome && aChromeOnly) continue; Then just need an if/else here.

The mochitest-chrome stuff sounds like it overlaps some work Joel was doing.

Thanks ted, it doesn't look like much overlap. Actually when I do get around the landing my mochikit.jar stuff, it will be installed in the profile instead of the application directory (a side effect of android)

Comment on attachment 459065 [details] [diff] [review] When checking for sorted-matching, keep version A as the unified version, not the sorted version >diff --git a/build/Makefile.in b/build/Makefile.in >--- a/build/Makefile.in >+++ b/build/Makefile.in >@@ -181,17 +181,17 @@ check:: > else \ > echo "TEST-PASS | build/ | unify unified a Java class file!"; \ > fi > # try unifying some files that differ only in line ordering > rm -rf unify-sort-test > mkdir unify-sort-test unify-sort-test/a unify-sort-test/b > printf "lmn\nabc\nxyz\n" > unify-sort-test/a/file.foo > printf "xyz\nlmn\nabc" > unify-sort-test/b/file.foo >- printf "abc\nlmn\nxyz\n" > unify-sort-test/expected-result >+ printf "lmn\nabc\nxyz\n" > unify-sort-test/expected-result > @if ! $(srcdir)/macosx/universal/unify --unify-with-sort "\.foo$$" \ > ./unify-sort-test/a ./unify-sort-test/b \ > ./unify-sort-test/c; then \ > echo "TEST-UNEXPECTED-FAIL | build/ | unify failed to unify files with differing line ordering!"; \ > false; \ > fi > @if ! diff -q ./unify-sort-test/expected-result ./unify-sort-test/c/file.foo; then \ You could instead just pass unify-sort-test/a instead of expected-result here. Doesn't matter much to me. All this shell scriptery is gross, I should have written it in Python.

Will this break multilocale Maemo builds? (jars+manifests in chrome/)

I don't know, how are they built?

Putting jars and manifests in chrome/, plus some pref changes and maybe some other magic i don't know of. Pretty sure this'll break jmaher's fennecmark stuff, but that's already currently broken.

I was looking for a pointer to the code: if it is just dropping stuff in, yes, that will no longer work. You probably want to generate a chrome/localized.manifest which contains the directives for all the locales combined.

Aki sent a link to http://hg.mozilla.org/mobile-browser/file/55ecfa303d51/locales/Makefile.in#l117 Where and how is this called?

This is called after building we run |make chrome-AB_CD| for every locale we want added to the multi-locale deb, then make package/make deb.

In that case I suspect this will "just work". We'll find out when it gets checked in... waiting for tryserver results, and I'm on vacation tomorrow, so ugh.

for fennecmark (talos test for fennec), what needs to be done? Should this be installed in the profile instead of the appdir?

I don't know. Why can't it do what desktop fennec does and install into distribution/bundles?

dev-doc-needed: for the new "manifest" command to chrome.manifest (to be documented at https://developer.mozilla.org/en/Chrome_Registration ). Anything else documentation-worthy?

If there are XULRunner docs which say that manifests may be dropped into chrome/ those should be updated as well.

Thanks, I found https://developer.mozilla.org/en/XUL_Tutorial/Manifest_Files#Manifest_Files and possibly https://developer.mozilla.org/en/Creating_XULRunner_Apps_with_the_Mozilla_Build_System#Okay.2c_okay.2c_can_I_build_it_now.3f

The manifest command has been documented for a while. The other articles mentioned in comment 30 still need updating.

Documentation has been updated here: https://developer.mozilla.org/en/XUL_Tutorial/Manifest_Files#Manifest_files https://developer.mozilla.org/en/Chrome_Registration Linked to from: https://developer.mozilla.org/en/Firefox_4_for_developers#Other_changes