This addresses issues with service prefs, which land in the newer 'defaults/preferences' location. From my understanding 'gre/defaults/pref' is depreciated; we keep it around due to channel-prefs.js, which we can't move easily. NS_APP_PREF_DEFAULTS_50_DIR is the constant for this older path which is recognized and returned by nsXREDirProvider, nsAppFileLocationProvider, and by xpcshell. AFAICT this constant is only used by Preferences.cpp. So with the move of services to gre, services prefs need to get picked up by Preferences.cpp without breaking the older channel-prefs location. So I've added the new gre default prefs path to NS_APP_PREFS_DEFAULTS_DIR_LIST which is returned by nsXREDirProvider and by xpcshell, and marked NS_APP_PREF_DEFAULTS_50_DIR as depreciated.

(In reply to Jim Mathies [:jimm] from comment #1) > Created attachment 687203 [details] [diff] [review] > dir svc patch v.1 > > This addresses issues with service prefs, which land in the newer > 'defaults/preferences' location. From my understanding 'gre/defaults/pref' > is depreciated; we keep it around due to channel-prefs.js, which we can't > move easily. It's not deprecated. In fact, it's still there, hidden in omni.ja. The pref directory in gre is defaults/pref ; the pref directory in app is defaults/preferences.

Comment on attachment 687203 [details] [diff] [review] dir svc patch v.1 Review of attachment 687203 [details] [diff] [review]: ----------------------------------------------------------------- Grammar driveby! ::: js/xpconnect/shell/xpcshell.cpp @@ +2044,5 @@ > return mAppFile->Clone(result); > } else if (mGREDir && !strcmp(prop, NS_APP_PREF_DEFAULTS_50_DIR)) { > nsCOMPtr<nsIFile> file; > *persistent = true; > + // Depreciated - Preferences.cpp still uses this to pick up channel-pref-js That'll be "Deprecated", though it's also lost value over time :) ::: modules/libpref/src/Preferences.cpp @@ +1072,5 @@ > > // Load $gre/defaults/pref/*.js > nsCOMPtr<nsIFile> defaultPrefDir; > > + // Depreciated - contains channel-pref-js Likewise. (Also, I have a strong preference for closing periods. Comments are English sentences.)

(In reply to Mike Hommey [:glandium] from comment #4) > (In reply to Jim Mathies [:jimm] from comment #1) > > Created attachment 687203 [details] [diff] [review] > > dir svc patch v.1 > > > > This addresses issues with service prefs, which land in the newer > > 'defaults/preferences' location. From my understanding 'gre/defaults/pref' > > is depreciated; we keep it around due to channel-prefs.js, which we can't > > move easily. > > It's not deprecated. In fact, it's still there, hidden in omni.ja. The pref > directory in gre is defaults/pref ; the pref directory in app is > defaults/preferences. Hmm, seems rather disconnected but ok. The build system was placing the pref files here via PREF_JS_EXPORTS, for example this file - http://mxr.mozilla.org/mozilla-central/source/services/sync/Makefile.in#83 would end up in dist/bin/defaults/preferences rather than prefs. I seem to remember we were trying to standardize on "preferences" but if "prefs" is still valid I can look at fixing whatever PREF_JS_EXPORTS is doing such that the files go to the "prefs" directory.

(In reply to Jim Mathies [:jimm] from comment #6) > Hmm, seems rather disconnected but ok. The build system was placing the pref > files here via PREF_JS_EXPORTS, for example this file - > > http://mxr.mozilla.org/mozilla-central/source/services/sync/Makefile.in#83 > > would end up in dist/bin/defaults/preferences rather than prefs. I wouldn't use the Sync makefiles as an example of correct practices -- after all, this bug is about code being in the wrong place, right?

(In reply to Richard Newman [:rnewman] from comment #7) > (In reply to Jim Mathies [:jimm] from comment #6) > > > Hmm, seems rather disconnected but ok. The build system was placing the pref > > files here via PREF_JS_EXPORTS, for example this file - > > > > http://mxr.mozilla.org/mozilla-central/source/services/sync/Makefile.in#83 > > > > would end up in dist/bin/defaults/preferences rather than prefs. > > I wouldn't use the Sync makefiles as an example of correct practices -- > after all, this bug is about code being in the wrong place, right? True, but the mixed use we have with "prefs" for gre and "preferences" for app seems to be broken too. :)

Comment on attachment 687204 [details] [diff] [review] svc paths v.1 Review of attachment 687204 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/marionette/marionette-actors.js @@ +36,5 @@ > Services.prefs.setBoolPref("marionette.contentListener", false); > let appName = Services.appinfo.name; > > // import logger > +Cu.import("resource://services-common/log4moz.js"); Want to place bets on this breaking tests due to missing resource:// registration?

(In reply to Gregory Szorc [:gps] from comment #9) > Comment on attachment 687204 [details] [diff] [review] > svc paths v.1 > > Review of attachment 687204 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: testing/marionette/marionette-actors.js > @@ +36,5 @@ > > Services.prefs.setBoolPref("marionette.contentListener", false); > > let appName = Services.appinfo.name; > > > > // import logger > > +Cu.import("resource://services-common/log4moz.js"); > > Want to place bets on this breaking tests due to missing resource:// > registration? That registration is in a manifest loaded by the GRE.

(In reply to Jim Mathies [:jimm] from comment #6) > (In reply to Mike Hommey [:glandium] from comment #4) > > (In reply to Jim Mathies [:jimm] from comment #1) > > > Created attachment 687203 [details] [diff] [review] > > > dir svc patch v.1 > > > > > > This addresses issues with service prefs, which land in the newer > > > 'defaults/preferences' location. From my understanding 'gre/defaults/pref' > > > is depreciated; we keep it around due to channel-prefs.js, which we can't > > > move easily. > > > > It's not deprecated. In fact, it's still there, hidden in omni.ja. The pref > > directory in gre is defaults/pref ; the pref directory in app is > > defaults/preferences. > > Hmm, seems rather disconnected but ok. The build system was placing the pref > files here via PREF_JS_EXPORTS, for example this file - > > http://mxr.mozilla.org/mozilla-central/source/services/sync/Makefile.in#83 > > would end up in dist/bin/defaults/preferences rather than prefs. There's probably something defining XPI_NAME, which triggers the change between defaults/pref and defaults/preferences. > I seem to remember we were trying to standardize on "preferences" but if > "prefs" is still valid I can look at fixing whatever PREF_JS_EXPORTS is > doing such that the files go to the "prefs" directory. I don't remember any such standardization.

(In reply to Mike Hommey [:glandium] from comment #11) > > would end up in dist/bin/defaults/preferences rather than prefs. > > There's probably something defining XPI_NAME, which triggers the change > between defaults/pref and defaults/preferences. > > > I seem to remember we were trying to standardize on "preferences" but if > > "prefs" is still valid I can look at fixing whatever PREF_JS_EXPORTS is > > doing such that the files go to the "prefs" directory. > > I don't remember any such standardization. Ok np, I'll rework to get everything in "prefs".

A couple notes on this - the current treatment of PREF_DIR seems pretty broken. For fx it will always point to the app dir unless GRE_MODULE is defined, in which case it points to the greprefs sub dir, which we don't use. Also it will always be equal to the app dir when processing the package manifest, even if we've added prefs to the gre location somewhere in the build using GRE_MODULE. I reformulated this a bit such that PREF_DIR eith points to defaults/pref for gre or defaults/preferences for app. Also in the packager, I'm hard coding gre prefs paths to avoid PREF_DIR's default value, which points to the wrong location. (We are already doing this for channel-prefs.js.) I need to chat with rstrong today about this since I'm moving pref files around. We will need to remove the old ones in the app dir on the update.

(In reply to Jim Mathies [:jimm] from comment #13) > Created attachment 687725 [details] [diff] [review] > build logic for prefs dir v.1 Ignore that AitcComponents.manifest entry, that should be in the next patch I post.

(In reply to Jim Mathies [:jimm] from comment #13) > I need to chat with rstrong today about this since I'm moving pref files > around. We will need to remove the old ones in the app dir on the update. All the files are in omni.ja anyways, although in theory, removed-files.in needs to be updated for flat and jar builds (builds without omni.ja).

Thanks for the qucik feedback. One other curious thing I came across - in fx's package manifest we have - #ifndef LIBXUL_SDK ; Warning: changing the path to channel-prefs.js can cause bugs (Bug 756325) @BINPATH@/defaults/pref/channel-prefs.js #else @BINPATH@/@PREF_DIR@/channel-prefs.js #endif But since MOZ_PHEONIX is defined here, these two paths will be identical. Seems like this logic could be simplified with just: @BINPATH@/defaults/pref/channel-prefs.js

(In reply to Jim Mathies [:jimm] from comment #17) > But since MOZ_PHEONIX is defined here, these two paths will be identical. > Seems like this logic could be simplified with just: > > @BINPATH@/defaults/pref/channel-prefs.js @PREF_DIR@ may expand to defaults/preferences when building FF-on-XR.

Will post the final patches for review once this completes a full try run: https://tbpl.mozilla.org/?tree=Try&rev=d86ba10443eb

(In reply to Jim Mathies [:jimm] from comment #19) > Will post the final patches for review once this completes a full try run: > https://tbpl.mozilla.org/?tree=Try&rev=d86ba10443eb Had to push a new rev with a fix for a marionette path: https://tbpl.mozilla.org/?tree=Try&rev=29339a97eac2

This fix allows xpcshell to find app prefs once gre and app are split up, without breaking current behavior.

Same as original patch w/updates to removed-files.

Service path updates so that everything points to the gre.

Comment on attachment 687901 [details] [diff] [review] service paths v.1 Review of attachment 687901 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 687896 [details] [diff] [review] xpcshell fix v.1 Review of attachment 687896 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/shell/xpcshell.cpp @@ +2118,5 @@ > + nsCOMPtr<nsIFile> exeDir, appDir; > + bool exists; > + if (NS_SUCCEEDED(NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR, getter_AddRefs(exeDir))) && > + NS_SUCCEEDED(exeDir->AppendNative(NS_LITERAL_CSTRING("defaults"))) && > + NS_SUCCEEDED(exeDir->AppendNative(NS_LITERAL_CSTRING("preferences"))) && This part isn't needed. You only really need appdir/defaults/preferences. gredir/defaults/pref is already in returned for NS_APP_PREF_DEFAULTS_50_DIR.

Comment on attachment 687899 [details] [diff] [review] build logic for prefs dir v.2 Review of attachment 687899 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/installer/removed-files.in @@ +97,5 @@ > defaults/pref/reporter.js > defaults/pref/security-prefs.js > defaults/pref/winpref.js > defaults/pref/xpinstall.js > +#ifdef MOZ_SERVICES_AITC You don't want ifdefs in removed-files.in files. ::: config/rules.mk @@ +1169,5 @@ > > +# the default location for gre prefs > +PREF_DIR = defaults/pref > + > +ifndef GRE_MODULE After giving this more thought, I think that better than GRE_MODULE, what would fit the bill is DIST_SUBDIR: if DIST_SUBDIR is not set, then the app and gre directories are mixed, and PREF_DIR = default/pref, which is the expected behaviour (see comment in pref_InitInitialObjects() in modules/libpref/src/Preferences.cpp). If DIST_SUBDIR is set, then the app and gre directories are different, and PREF = default/preferences, which is the expected behaviour as well. The exceptions for XPI_NAME, LIBXUL_SDK should be kept, but the exception for MOZ_PHOENIX can go away. This will move the firefox prefs within omni.ja on non-metro builds, but that doesn't really matter.

(In reply to Mike Hommey [:glandium] from comment #25) > Comment on attachment 687896 [details] [diff] [review] > xpcshell fix v.1 > > Review of attachment 687896 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/xpconnect/shell/xpcshell.cpp > @@ +2118,5 @@ > > + nsCOMPtr<nsIFile> exeDir, appDir; > > + bool exists; > > + if (NS_SUCCEEDED(NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR, getter_AddRefs(exeDir))) && > > + NS_SUCCEEDED(exeDir->AppendNative(NS_LITERAL_CSTRING("defaults"))) && > > + NS_SUCCEEDED(exeDir->AppendNative(NS_LITERAL_CSTRING("preferences"))) && > > This part isn't needed. You only really need appdir/defaults/preferences. > > gredir/defaults/pref is already in returned for NS_APP_PREF_DEFAULTS_50_DIR. Pushed this change individually to try, looks ok - https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=ff2430a56984

Works both on elm and mc.

Comment on attachment 688771 [details] [diff] [review] xpcshell fix v.2 Review of attachment 688771 [details] [diff] [review]: ----------------------------------------------------------------- Note that this likely breaks running xpcshell tests locally with an unpackaged build (i.e. off dist/bin), but the second patch should fix that. I'd thus advise to land the second patch first.

Here's a fix for webapprt which was using FINAL_TARGET vs DIST_SUBDIR. The pref file ended up in 'pref' so packaging broke on try - https://tbpl.mozilla.org/?tree=Try&rev=53132060911c Repushed to try again to be sure. A diff of the webapprt app folder shows everything to be in the proper place with this fix. https://tbpl.mozilla.org/?tree=Try&rev=5b52c9d6f09a

Oops, I'll touch up that 'webaprt' typeo in the comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa74a58cc45e https://hg.mozilla.org/integration/mozilla-inbound/rev/0008f31d7748 https://hg.mozilla.org/integration/mozilla-inbound/rev/69193d62054b https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf5d89f54f3

https://hg.mozilla.org/mozilla-central/rev/aa74a58cc45e https://hg.mozilla.org/mozilla-central/rev/0008f31d7748 https://hg.mozilla.org/mozilla-central/rev/69193d62054b https://hg.mozilla.org/mozilla-central/rev/4bf5d89f54f3