Can you tell me why these paths are bad? resource://gre/modules is used throughout the tree. https://mxr.mozilla.org/mozilla-central/search?string=gre/modules

(In reply to Gregory Szorc [:gps] from comment #2) > Can you tell me why these paths are bad? resource://gre/modules is used > throughout the tree. > https://mxr.mozilla.org/mozilla-central/search?string=gre/modules Services code is installed in the app directory rather than platform. I'm not sure why that is. Cc'ing a few people who in who might know.

(In reply to Jim Mathies [:jimm] from comment #3) > Services code is installed in the app directory rather than platform. I'm > not sure why that is. Cc'ing a few people who in who might know. The answer is likely hysterical raisins. Parts of /services should probably be considered platform (like /services/common). Others (like /services/sync) might be a better fit for app. What's the distinction? Why does it matter? It seems that up until now it really didn't matter and it was just a matter of paths and URIs being in agreement. You are obviously changing things for Metro, so it must matter now. What's going on?

With metro we are adding a second application with it's own app resources to the standard packaging. The work to enable this is taking place in bug 755724, which should be landing on mc here in the next few weeks. So we have two app dirs which get chosen depending on how the exe starts up. For firefox it's ./browser, for metro it's ./metro. (Also we have webaprt, which will be split out well.) When we run xpcshell tests with this new setup by default the application directory is the gre directory. Hence these tests fail because they rely on resources that are stored in app dir modules. Bug 810617 takes care of allowing us to set an app dir ('browser' or 'metro' or ..) per test or for a set of tests through the xpcshell.ini file, so we have that bit of logic fixed up. What's left to do involves going over all the places where test authors have assumed an app resource is available that isn't, and touching those tests up. Hence this bug. Make sense?

That does help. Most of the code in /services isn't application specific. Most of the code in /services is expected to be able to run on multiple apps. Therefore, I'm tempted to say that the code (read: JS modules) in /services should be installed in platform/GRE, not an app dir. Is that reasoning proper?

(In reply to Gregory Szorc [:gps] from comment #6) > Is that reasoning proper? It works for me. Firefox browser-specific services code already lives in browser/ or components/ (mainly the setup UX). services/ should be considered platform, if the alternative is to dupe-deploy it….

(In reply to Jim Mathies [:jimm] from comment #5) > With metro we are adding a second application with it's own app resources to > the standard packaging. The work to enable this is taking place in bug > 755724, which should be landing on mc here in the next few weeks. So we have > two app dirs which get chosen depending on how the exe starts up. For > firefox it's ./browser, for metro it's ./metro. (Also we have webaprt, which > will be split out well.) Even before that, you could build firefox as a xulrunner application, in which case the application path and the gecko path are different. That's how firefox is built on several Linux distros.

(In reply to Gregory Szorc [:gps] from comment #6) > That does help. > > Most of the code in /services isn't application specific. Most of the code > in /services is expected to be able to run on multiple apps. > > Therefore, I'm tempted to say that the code (read: JS modules) in /services > should be installed in platform/GRE, not an app dir. > > Is that reasoning proper? This sounds right to me and solves the problem of having to build services twice to get the app resources in two different app dirs. I'm just curious why we put these resources in the app dir in the first place. Maybe there was good reason. I'll experiment with moving to gre.

As I said, it was likely app for hysterical raisins. See also bug 779370. In short, we know the current state sucks. It just hasn't been a high priority for us to fix.

Comment on attachment 685309 [details] [diff] [review] patch v.1 Ok, so I came at this from the wrong angle. On elm we added an include that imported the DIST_SUBDIR from the browser in services - http://mxr.mozilla.org/projects-central/source/elm/services/defs.mk http://mxr.mozilla.org/projects-central/source/elm/browser/defs.mk Which put everything into dist/bin/browser. I think this was a mistake, since most of the paths in the services code already pointed to gre. So I'm removing this include and will check all the paths again to be sure we don't have any app paths. I'll look at bug 779370 as well although it won't hold up landings on elm.

However, there *are* parts in browser and parts in gre, and paths added by bug 809930 and bug 802914 are wrong.

And bug 804491.

The interesting thing is that services code usually uses resource://services-common/ instead of resource:///modules/services-common/ because it's an alias it sets. So using resource://services-common/ would solve every problem with resource://{gre,}/modules/services-common/, even if services-common is moved to gre in the future

(In reply to Mike Hommey [:glandium] from comment #13) > And bug 804491. And many others :(

This would certainly benefit from aliases like services-common for metrics and healthreport, but i'll leave that for another bug.

I don't think you want the resource://gre/ -> resource:/// conversions in metrics tests, did you migrate those from my original patch?) (which was wrong)

Also in the health report code.

For discussion, I was moving this code in the opposite direction: placing all services code in gre. This also requires a change to xpcshell and xredirprovider so that the services prefs are found.

Every js and jsm file under services/ is installed in browser.

(In reply to Jim Mathies [:jimm] from comment #19) > Created attachment 687057 [details] [diff] [review] > svc to gre v.1 > > For discussion, I was moving this code in the opposite direction: placing > all services code in gre. This also requires a change to xpcshell and > xredirprovider so that the services prefs are found. I'm also interested in fixing the urls in beta and aurora.

after a chat on irc, glandium is going to fix these paths for ff-on-xr on aurora/beta. We'll leave this open for continued work on moving services code to gre.

I think this is the direction we want to go. The one issue I came up on was 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.

This will need to be updated to undo most of the changes in "Fix services import urls" once that lands on mc.

This removes the def.mk import and the bit of build logic that rebuilt svcs code so that it was distributed into /metro/*.

(In reply to Jim Mathies [:jimm] from comment #23) > 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. Why is there any need for changes for prefs? There's everything already to handle prefs in all the possible locations.

(In reply to Mike Hommey [:glandium] from comment #26) > (In reply to Jim Mathies [:jimm] from comment #23) > > 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. > > Why is there any need for changes for prefs? There's everything already to > handle prefs in all the possible locations. gre/defaults/preferences isn't picked up by Preferences.cpp without these changes once we split gre and app locations.

Holy cow a lot happened in just a few hours! First, where did this C++ patch come from? AFAICT it's not related to /services and IMO should be a separate bug. (In reply to Jim Mathies [:jimm] from comment #22) > after a chat on irc, glandium is going to fix these paths for ff-on-xr on > aurora/beta. Why? What harm are they causing that warrants uplift?

See also bug 813287 for changing how the HealthReport prefs work. Not sure if that is related. I was planning on landing that soonish.

(In reply to Gregory Szorc [:gps] from comment #28) > Holy cow a lot happened in just a few hours! > > First, where did this C++ patch come from? AFAICT it's not related to > /services and IMO should be a separate bug. > > (In reply to Jim Mathies [:jimm] from comment #22) > > after a chat on irc, glandium is going to fix these paths for ff-on-xr on > > aurora/beta. > > Why? What harm are they causing that warrants uplift?

(In reply to Gregory Szorc [:gps] from comment #28) > Holy cow a lot happened in just a few hours! > > First, where did this C++ patch come from? AFAICT it's not related to > /services and IMO should be a separate bug. > > (In reply to Jim Mathies [:jimm] from comment #22) > > after a chat on irc, glandium is going to fix these paths for ff-on-xr on > > aurora/beta. > > Why? What harm are they causing that warrants uplift? It breaks firefox when built as a xulrunner application, because the resource://gre/... urls are not found.

(In reply to Jim Mathies [:jimm] from comment #27) > gre/defaults/preferences isn't picked up by Preferences.cpp without these > changes once we split gre and app locations. Why would there be things to pick up from gre/defaults/preferences? There shouldn't be anything in there in the first place.

(In reply to Mike Hommey [:glandium] from comment #31) > It breaks firefox when built as a xulrunner application, because the > resource://gre/... urls are not found. Firefox isn't building any of that code on aurora/beta, right?

(In reply to Mike Hommey [:glandium] from comment #32) > (In reply to Jim Mathies [:jimm] from comment #27) > > gre/defaults/preferences isn't picked up by Preferences.cpp without these > > changes once we split gre and app locations. > > Why would there be things to pick up from gre/defaults/preferences? There > shouldn't be anything in there in the first place. See comments in bug 817076.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #33) > (In reply to Mike Hommey [:glandium] from comment #31) > > It breaks firefox when built as a xulrunner application, because the > > resource://gre/... urls are not found. > > Firefox isn't building any of that code on aurora/beta, right? At least services/common/bagheeraclient.js and services/metrics/collector.jsm are. BTW, there's a mistake in the patch, the manifest file should be left untouched. (bad sed)

(In reply to Mike Hommey [:glandium] from comment #35) > BTW, there's a mistake in the patch, the manifest file should be left > untouched. (bad sed) services/sync/SyncComponents.manifest, that is

(In reply to Mike Hommey [:glandium] from comment #31) > It breaks firefox when built as a xulrunner application, because the > resource://gre/... urls are not found. Is anyone actually complaining about this? (In reply to Mike Hommey [:glandium] from comment #35) > (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment > #33) > > (In reply to Mike Hommey [:glandium] from comment #31) > > > It breaks firefox when built as a xulrunner application, because the > > > resource://gre/... urls are not found. > > > > Firefox isn't building any of that code on aurora/beta, right? > > At least services/common/bagheeraclient.js and > services/metrics/collector.jsm are. Those files shouldn't be referenced by Firefox until 20. I do not consider those files part of the public API. B2G (18) will reference them. But, B2G is not a XULRunner application and things seem to work just fine. So, I ask again: why do we need to uplift?

(In reply to Gregory Szorc [:gps] from comment #37) > (In reply to Mike Hommey [:glandium] from comment #31) > > It breaks firefox when built as a xulrunner application, because the > > resource://gre/... urls are not found. > > Is anyone actually complaining about this? Me, with my Debian hat on, and the Fedora people, when they figure it out. > (In reply to Mike Hommey [:glandium] from comment #35) > > (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment > > #33) > > > (In reply to Mike Hommey [:glandium] from comment #31) > > > > It breaks firefox when built as a xulrunner application, because the > > > > resource://gre/... urls are not found. > > > > > > Firefox isn't building any of that code on aurora/beta, right? > > > > At least services/common/bagheeraclient.js and > > services/metrics/collector.jsm are. > > Those files shouldn't be referenced by Firefox until 20. I do not consider > those files part of the public API. B2G (18) will reference them. But, B2G > is not a XULRunner application and things seem to work just fine. > > So, I ask again: why do we need to uplift? So, why are these files installed? (that's how i figured out, i have a script checking installed files for such wrong imports in the Debian package)

(In reply to Mike Hommey [:glandium] from comment #38) > So, why are these files installed? (that's how i figured out, i have a > script checking installed files for such wrong imports in the Debian package) /services/common is installed because everything in /services/common is installed. /services/metrics is installed because we wanted test coverage on as many branches as possible. The code is not intended to be used on 18 outside of B2G. We figured the desire for more test coverage outweighed shipping files that not aren't yet used. FTR, I just see uplifting as more trouble than it's worth. I'm not going to block the effort - I just think it's a waste of time.

Ok, I'll just add an error filter, then. Since bug 817076 was filed, I guess we can close this one.

Mmmmm although it will still break those tests for me... sigh