Webapp runtime files are mixed with Firefox files in the Firefox package, which means they inherit Firefox default preferences, require platform hacks to override Firefox behavior, and may have hidden bugs associated with this fraternization. Thus those files should be isolated from Firefox files in the package. I have a patch for this, which bsmedberg already reviewed over in bug 725408. It just needs some fixing up per his review comments. But since we landed the main patch in that bug, I'm filing this one to track the followup work.

(In reply to Benjamin Smedberg [:bsmedberg] from bug #725408, comment #53) > Comment on attachment 614594 [details] [diff] [review] > followup v2: updated for patch v4 > > >diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in > > > #ifdef MOZ_WEBAPP_RUNTIME > >-; [Webapp Runtime] > >-@BINPATH@/webapprt@BIN_SUFFIX@ > >-@BINPATH@/chrome/webapprt@JAREXT@ > >-@BINPATH@/chrome/webapprt.manifest > >-@BINPATH@/components/WebappRTComponents.manifest > >-@BINPATH@/components/WebappRTDirectoryProvider.js > >-@BINPATH@/components/WebappRTCommandLineHandler.js > >-@BINPATH@/webapprt.ini > >+[WebappRuntime] > >+@BINPATH@/webapprt/* > > #endif > > I'd prefer that we list each file. It makes it easier to be sure that when > we're adding or removing files that they are listed in the proper > package-manifest or removed-files location. With the wildcard you won't get > a packaging warning if a file is removed or renamed. Makes sense, done. https://github.com/mykmelez/mozilla-central/commit/8efe3e3eae13a1f97e6de108d4c5b952548aefd3 > >diff --git a/webapprt/CommandLineHandler.js b/webapprt/CommandLineHandler.js > >diff --git a/webapprt/DirectoryProvider.js b/webapprt/DirectoryProvider.js > > I have made the assumption that this has not actually changed. I wonder if > in your workflow there is a way to provide the equivalent patch to an `hg > move` so that I can just view the diff? If there are actually substantive > changes let me know. GitHub's representation of the initial (large) commit for this bug does a good job of showing the two file moves (and the trivial change that was made to DirectoryProvider.js in conjunction with these changes): https://github.com/mykmelez/mozilla-central/commit/e9f6bd36c244d30279f5b1c6de6a3c3cff3b4289#diff-13 https://github.com/mykmelez/mozilla-central/commit/e9f6bd36c244d30279f5b1c6de6a3c3cff3b4289#diff-14 > >diff --git a/webapprt/locales/en-US/webapprt/webapp.dtd b/webapprt/locales/en-US/webapprt/webapp.dtd > > Moving the locale files to a new source location requires significant > additional work related to l10n repackaging. I think that we should hold off > this particular part of the patch for another time when we're not under time > pressure. It shouldn't cause other problems. Ok, I have reverted the l10n changes. https://github.com/mykmelez/mozilla-central/commit/385c871593be0ee7d6ac769a29a12a0614dc74cc https://github.com/mykmelez/mozilla-central/commit/5b85c4dcd5832ebc7ba3df135e769073060f6410 For posterity, Axel says: "If you wanted to move the files to webapprt, you'd have to add webapprt to browser/locales/l10n.ini, add webapprt to the directories to repack in libs-%, and have a Makefile.in in webapprt/locales very much like the one in dom/locales." > r=me on the non-l10n bits. Per IRC discussion I believe you were going to > name it webapprt-stub and put it in the root directory, which might require > some undoing of the FINAL_TARGET changes in the two stub directories, but I > don't think that's especially complicated or needs additional review. Yup, here are the FINAL_TARGET changes (plus changes to the stub code to make the stubs look in the root directory for newer versions of themselves). https://github.com/mykmelez/mozilla-central/commit/886788191823a684f0f29173b97e4204cbc3d4c3 However, in the end I had to make an additional change that warrants review, as I noticed in the process of doing additional testing that Firefox default preferences were still being read by WebappRT. That's because Preferences.cpp now reads application default prefs from $gre/omni.ja!/defaults/preferences/ if there is no $app/omni.ja!/defaults/preferences/ to read them from. We need that behavior because Firefox's default prefs are in the $gre omni.ja, since the GRE and Firefox share a directory; but it means that WebappRT gets the Firefox preferences too. The fix is for WebappRT to get its own omni.ja, so Preferences.cpp reads application default prefs from $app/omni.ja!/defaults/preferences/, i.e. WebappRT's omni.ja. This change makes that happen. https://github.com/mykmelez/mozilla-central/commit/329f0ab45eceb0057e42a3a1d443d9ec87b04303 I've tested both with and without omni.ja, on both Mac and Windows, by running both Firefox and WebappRT from dist/bin/ (dist/NightlyDebug.app/ on Mac), dist/firefox/ (after doing |make installer| or |make package| to create the staged files), and an installation. Firefox continues to work correctly, and WebappRT does too, while no longer loading Firefox default preferences. And after seeing the regression caused by bug 725408, I tested with extensions installed in Firefox, and they loaded correctly too. I have also submitted this patch to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=2388935845f6

Comment on attachment 616074 [details] [diff] [review] patch v3: addresses issues in second patch I reviewed the github link, it appears to be correct. I'm a little worried about l10n repacks being busted with this, so you should probably try doing one of those manually, absent any tryserver support. It kinda sucks that we have the three different declarations of PREPARE_PACKAGE, but trying to make it more modular would probably make it less readable, so we'll stick with this version for now anyway.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2) > Comment on attachment 616074 [details] [diff] [review] > patch v3: addresses issues in second patch > > I reviewed the github link, it appears to be correct. I'm a little worried > about l10n repacks being busted with this, so you should probably try doing > one of those manually, absent any tryserver support. Good thinking! Testing l10n repacking (Windows and Mac, mostly fr locale) uncovered a use of UNPACK_OMNIJAR in toolkit/locales/l10n.mk. I had to add UNPACK_OMNIJAR_WEBAPP_RUNTIME at that call site; otherwise, the repack would work the first time around but fail on subsequent runs in the same objdir. https://github.com/mykmelez/mozilla-central/commit/b5e04e52bb829d459774d756f8ce10e5d85ff958 In the process of diagnosing that problem, I also noticed a hardcoded path defaults/pref/firefox-l10n.js in browser/locales/Makefile.in. That needs to be a path to the new location of that file, defaults/preferences, which I did by replacing "defaults/pref" with PREF_DIR rather than hardcoding the new location. https://github.com/mykmelez/mozilla-central/commit/a79a28d1ae0052f0e40cd27eda5fc3eff7bceef3 > It kinda sucks that we have the three different declarations of > PREPARE_PACKAGE, but trying to make it more modular would probably make it > less readable, so we'll stick with this version for now anyway. I stared at it some, and in the end I came to a similar conclusion.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e9e62564ab34 Note: On Sunday, blassey reviewed this patch (along with a set of other WebRT Desktop patches) for Fennec risk, and he concluded that they aren't risky. Then on Monday, via email, akeybl approved them for landing during the APPROVAL REQUIRED period. He said that I didn't need to also nominate them for approval via Bugzilla flags, so I haven't.

This still broke windows l10n nightlies. The bustage comes from application.ini which is assumed to be only in the dist once by the partial update process. I don't have a good idea for a fix on the build side, I wonder if the webapprt code can go for a different filename?

(In reply to Axel Hecht [:Pike] from comment #6) > I don't have a good idea for a fix on the build side, I wonder if the > webapprt code can go for a different filename? We can, if needed. But let's discuss that in bug 747394 to avoid fragmenting the conversation!

Myk - How would I go about verifying this bug? What am I looking for?

Jason: install the latest nightly build, open the installation directory, and look for a "webapprt" directory inside it. If it exists, this bug has been fixed.

Verified on Win 7 64-bit, Win Vista, Win XP, OS X 10.5, OS X 10.6, OS X 10.7.

Comment on attachment 616482 [details] [diff] [review] patch v4: resolve issues revealed by l10n repack test Review of attachment 616482 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/installer/packager.mk @@ +897,5 @@ > endif > ifdef MOZ_OMNIJAR > cd $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && $(PACK_OMNIJAR) > +ifdef MOZ_WEBAPP_RUNTIME > + cd $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/webapprt && $(PACK_OMNIJAR_WEBAPP_RUNTIME)) Note the extra ) at the end here, which is burning RPM nightlies.

(In reply to :Ms2ger from comment #11) > > + cd $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/webapprt && $(PACK_OMNIJAR_WEBAPP_RUNTIME)) > > Note the extra ) at the end here, which is burning RPM nightlies. Is there a bug on file about the burning RPM nightlies?

This extra ) makes the following error: /bin/sh: -c: line 0: syntax error near unexpected token `)' /bin/sh: -c: line 0: `cd ../../dist/mozilla-nightly/webapprt && rm -f omni.ja; /usr/bin/zip -r9m omni.ja chrome chrome.manifest components/*.js components/*.xpt components/*.manifest modules res defaults greprefs.js jsloader jssubloader hyphenation update.locale -x chrome/icons/\* defaults/preferences/channel-prefs.js defaults/pref/channel-prefs.js res/cursors/\* res/MainMenu.nib/\* && /builds/slave/m-cen-lnx-rpm-ntly/build/obj-firefox/_virtualenv/bin/python /builds/slave/m-cen-lnx-rpm-ntly/build/config/optimizejars.py --optimize /builds/slave/m-cen-lnx-rpm-ntly/build/obj-firefox/browser/installer/../../jarlog//en-US ./ ./)' See RPM Nightly of https://tbpl.mozilla.org/?rev=a7a905fd70d5

(In reply to :Ms2ger from comment #11) File as bug 761975.