817076 - Move services code to gre

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Comment 1

[Jim Mathies [:jimm]]

Attachment: 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. 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.

Comment 2

[Jim Mathies [:jimm]]

Attachment: svc paths v.1

Comment 3

[Jim Mathies [:jimm]]

Attachment: elm fix

Comment 4

[Mike Hommey [:glandium]]

(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 5

[Richard Newman [:rnewman]]

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.)

Comment 6

[Jim Mathies [:jimm]]

(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.

Comment 7

[Richard Newman [:rnewman]]

(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?

Comment 8

[Jim Mathies [:jimm]]

(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 9

[Gregory Szorc [:gps]]

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?

Comment 10

[Mike Hommey [:glandium]]

(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.

Comment 11

[Mike Hommey [:glandium]]

(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.

Comment 12

[Jim Mathies [:jimm]]

(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".

Comment 13

[Jim Mathies [:jimm]]

Attachment: build logic for prefs dir v.1

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.

Comment 14

[Jim Mathies [:jimm]]

(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.

Comment 15

[Jim Mathies [:jimm]]

Attachment: paths v.2

Comment 16

[Mike Hommey [:glandium]]

(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).

Comment 17

[Jim Mathies [:jimm]]

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

Comment 18

[Mike Hommey [:glandium]]

(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.

Comment 19

[Jim Mathies [:jimm]]

Will post the final patches for review once this completes a full try run:

https://tbpl.mozilla.org/?tree=Try&rev=d86ba10443eb

Comment 20

[Jim Mathies [:jimm]]

(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

Comment 21

[Jim Mathies [:jimm]]

Attachment: xpcshell fix v.1

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

Comment 22

[Jim Mathies [:jimm]]

Attachment: build logic for prefs dir v.2

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

Comment 23

[Jim Mathies [:jimm]]

Attachment: service paths v.1

Service path updates so that everything points to the gre.

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 687901 [details] [diff] [review]
service paths v.1

Review of attachment 687901 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 25

[Mike Hommey [:glandium]]

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 26

[Mike Hommey [:glandium]]

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.

Comment 27

[Jim Mathies [:jimm]]

(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

Comment 28

[Jim Mathies [:jimm]]

Attachment: xpcshell fix v.2

Comment 29

[Jim Mathies [:jimm]]

Attachment: build logic for prefs dir v.3

Works both on elm and mc.

Comment 30

[Mike Hommey [:glandium]]

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.

Comment 31

[Jim Mathies [:jimm]]

Attachment: webapprt fix v.1

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

Comment 32

[Jim Mathies [:jimm]]

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

Comment 33

[Jim Mathies [:jimm]]

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

Comment 34

[Ed Morley [:emorley]]

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