Closed Bug 1203159 Opened 4 years ago Closed 4 years ago

Migrate DevTools resources to resource://devtools/ URLs

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: addon-compat)

Attachments

(9 files)

40 bytes, text/x-review-board-request
jryans
: review+
Details
40 bytes, text/x-review-board-request
ochameau
: review+
glandium
: review+
Details
40 bytes, text/x-review-board-request
glandium
: review+
Details
40 bytes, text/x-review-board-request
ochameau
: review+
Details
40 bytes, text/x-review-board-request
glandium
: review+
ochameau
: review+
Details
40 bytes, text/x-review-board-request
ochameau
: review+
Details
40 bytes, text/x-review-board-request
ochameau
: review+
Details
40 bytes, text/x-review-board-request
jryans
: review+
Details
40 bytes, text/x-review-board-request
gfritzsche
: review+
Details
In bug 912121 comment 24, Alex mentioned we could move to using a new resource://devtools/:

(In reply to Alexandre Poirot [:ochameau] from bug 912121 comment #24)
> About resource://gre/ versus resource://modules/
> Today, it looks like we don't do anything to map resource:// URL to local
> clone. (i.e. we always map to firefox-builtin files).
> I was wondering, if, while we are at changing this... if it would make sense
> to also introduce a resource://devtools path, mapping to the root devtools
> folder. 
> With its associated own jar file. We would not add any devtools specifics to
> any browser/toolkit omni.jar. And it would then be easy to map this resource
> URI to a local checkout via:
>   Services.io.getProtocolHandler("resource")
>              .QueryInterface(Ci.nsIResProtocolHandler)
>              .setSubstitution("devtools", uri);
> (It also open ways to shrink devtools out of firefox ;))
> Also, it would allow to load client files via non-chrome URLs without having
> to do yet another refactoring/file move.

:glandium has expressed some concerns about this, because as I understand it, implementing this would move all DevTools client files up to the top level omni.ja, increasing file size for other XUL app users.  In our own tree, we can avoid this by not building in the client directory, but someone using "firefox -app foo.ini" could be impacted.

Let's discuss the idea further here.
I think this dicussion is also related to having a easier mapping, easier than the various chrome:// conventions that doesn't allow us to have a 1-1 mapping to files.
I wish we could end up with just one URL kind. Today we have resource://gre, resource:///modules and chrome://devtools/content, chrome://devtools/skin.

I understand the issues with moving everything to toolkit's omni.ja.
I was wondering if we could move all devtools stuff to its own omni.ja?
I'm not sure it will help if omni.ja are loaded by default on process startup.
But it could if that can be done on demand.

It would still load all devtools resources, but only if you start using devtools.
Otherwise the only alternative I can think of (which is not incompatible with the devtools.ja proposal), would be to come up with two new resource:// mapping, specifics to devtools.
One mapping to a devtools-browser .jar file, like resource://devtools-client/
and another to a devtools-toolkit .jar file, like resource://devtools-toolkit/

My issue with using resource://gre and resource:///modules is that we can't map them to local files and that forces us to have two kind of URLS (resource: and chrome:).
Make devtools an addon, and you can have everything in one place.
(In reply to Mike Hommey [:glandium] from comment #2)
> Make devtools an addon, and you can have everything in one place.

That brings a whole new set of challenges... because DevTools are part of the Firefox codebase every time we make changes to browser code we run a test suite including devtools tests. Also, it is part of our mission to keep things open and teach people about the web so shipping the tools with the browser aligns perfectly with our mission.
Making it an addon doesn't mean it has to be out of Firefox.
(In reply to Mike Hommey [:glandium] from comment #4)
> Making it an addon doesn't mean it has to be out of Firefox.
*And* make it easier for SeaMonkey to ship the complete devtools!
I strongly object against using "resource://" as that will prevent full themes, such as Nautipolis, Walnut, etc to style the devtools.
(In reply to Mike Hommey [:glandium] from comment #2)
> Make devtools an addon, and you can have everything in one place.

:glandium, when you mention an add-on here, are you thinking of a "flattened" add-on like pdf.js and Shumway (which I believe directly integrates the add-on files into omni.ja, just with their own chrome.manifests), or an explicit add-on in a XPI file, but still built in-tree that is installed in the "dist/bin/browser/extensions" directory?

It seems like the "flattened" add-on approach would have the same omni.ja troubles, since files would still likely get spread across browser and toolkit by the build system.
Flags: needinfo?(mh+mozilla)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #2)
> > Make devtools an addon, and you can have everything in one place.
> 
> :glandium, when you mention an add-on here, are you thinking of a
> "flattened" add-on like pdf.js and Shumway (which I believe directly
> integrates the add-on files into omni.ja, just with their own
> chrome.manifests), or an explicit add-on in a XPI file, but still built
> in-tree that is installed in the "dist/bin/browser/extensions" directory?
> 
> It seems like the "flattened" add-on approach would have the same omni.ja
> troubles, since files would still likely get spread across browser and
> toolkit by the build system.

Mossop and I have been working on making it possible to ship add-ons ("system add-ons") with Firefox in Q3, and we are going to try it with Hello in Q4.

Support for system add-ons has landed in Nightly already (bug 1183866). Currently, Firefox looks in a particular directory in the app directory on startup and activates the add-ons found there. The code (in the prototype we've done for Hello in bug 1186172) continues to live in mozilla-central and the proposed build+release process (bug 1184524 comment 2) is very similar to today (we still need build command(s) for packing the XPI + copying to the right location, that's another Q4 task.)

We're also looking to enable updates to these add-ons outside of the normal release process (these would be controlled by the update server and be installed into the profile), but that particular bit is out of scope for Q4.

We are working on this as part of the "Go Faster" project, there's a mailing list and #gofaster IRC channel if you'd like to discuss.
(In reply to Robert Helmer [:rhelmer] from comment #8)
> Support for system add-ons has landed in Nightly already (bug 1183866).
> Currently, Firefox looks in a particular directory in the app directory on
> startup and activates the add-ons found there. The code (in the prototype
> we've done for Hello in bug 1186172) continues to live in mozilla-central
> and the proposed build+release process (bug 1184524 comment 2) is very
> similar to today (we still need build command(s) for packing the XPI +
> copying to the right location, that's another Q4 task.)

Please loop me in on the build system parts of that.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> :glandium, when you mention an add-on here, are you thinking of a
> "flattened" add-on like pdf.js and Shumway (which I believe directly
> integrates the add-on files into omni.ja, just with their own
> chrome.manifests), or an explicit add-on in a XPI file, but still built
> in-tree that is installed in the "dist/bin/browser/extensions" directory?

Whichever would work better.
Flags: needinfo?(mh+mozilla)
(In reply to Robert Helmer [:rhelmer] from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> > (In reply to Mike Hommey [:glandium] from comment #2)
> > > Make devtools an addon, and you can have everything in one place.
> > 
> > :glandium, when you mention an add-on here, are you thinking of a
> > "flattened" add-on like pdf.js and Shumway (which I believe directly
> > integrates the add-on files into omni.ja, just with their own
> > chrome.manifests), or an explicit add-on in a XPI file, but still built
> > in-tree that is installed in the "dist/bin/browser/extensions" directory?
> > 
> > It seems like the "flattened" add-on approach would have the same omni.ja
> > troubles, since files would still likely get spread across browser and
> > toolkit by the build system.
> 
> Mossop and I have been working on making it possible to ship add-ons
> ("system add-ons") with Firefox in Q3, and we are going to try it with Hello
> in Q4.
> 
> Support for system add-ons has landed in Nightly already (bug 1183866).
> Currently, Firefox looks in a particular directory in the app directory on
> startup and activates the add-ons found there. The code (in the prototype
> we've done for Hello in bug 1186172) continues to live in mozilla-central
> and the proposed build+release process (bug 1184524 comment 2) is very
> similar to today (we still need build command(s) for packing the XPI +
> copying to the right location, that's another Q4 task.)
> 
> We're also looking to enable updates to these add-ons outside of the normal
> release process (these would be controlled by the update server and be
> installed into the profile), but that particular bit is out of scope for Q4.
> 
> We are working on this as part of the "Go Faster" project, there's a mailing
> list and #gofaster IRC channel if you'd like to discuss.

Thanks for the note about Go Faster.  I've been following the Go Faster and system add-ons work at a high level.  AFAIK, the main benefit is to be able to ship updated add-on / feature code to the release channel between major Firefox releases.  DevTools is not really in a good position to take advantage of that: our code is usually deeply connected to the platform, so shipping a DevTools add-on update only without the newer platform sounds likely to break at the moment.

Having said that, it seems like we could structure DevTools as a system add-on code wise, but just never ship any updates.  I think that's something we could consider relatively soon, but I'd rather let the Go Faster group shake things out with Hello and other test add-ons first.  DevTools is a large and complex beast (not that the others are simple or something, but anyway), and we should do something like that when there is plenty of time to test safely.

For this bug, I want try to change to resource://devtools if feasible within the 44 cycle on Nightly.  This will avoid forcing DevTools add-on authors to maintain yet a 3rd branch of paths for these files (we've already changed them once in 44).

If we later want to package as a system add-on, that's fine.  My understanding is that it should not cause another change to the resource URIs if we are already using resource://devtools.
For the moment, I am prototyping using a jar.mn to specify a resource URI for DevTools and then installing the files in the directory to match that.  This is in the same style of current "flattened" add-ons like pdf.js and Shumway.  However, there is one obstacle with WebappRT.

To have a single resource://devtools/ URI (instead of split by client and server), I must install all of the files (client and server files) to the same directory structure.  In more detail:

* All DevTools files would be installed per-app
* Each non-browser app (phones, etc.) now individually includes the DevTools server, it is no longer part of toolkit (you have to "opt-in" to DevTools server)
* For Firefox, it means they go in dist/bin/browser/chrome/devtools/modules/*, and they would be part of browser/omni.ja
* WebappRT now has no way to access the DevTools server it used to reference via resource://gre/modules
  * We would either need to duplicate DevTools server files for both the browser and webapprt in Firefox (increasing download size), or create some custom solution

So, it almost works fine, except for the WebappRT case.

Any advice about this so far?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(myk)
Flags: needinfo?(mh+mozilla)
I think you should break the runtime's use of the DevTools server, disabling any automated unit tests for it in the process (although I don't think there are any unit tests for DevTools integration that currently run in automation), then file a bug on the runtime to fix their use of the DevTools server.  That way the moribund runtime doesn't block your active work nor increase the download size of the browser.
Flags: needinfo?(myk)
Why make it go in dist/bin/browser?
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #13)
> Why make it go in dist/bin/browser?

Well, I meant in FINAL_TARGET, which is dist/bin/browser when coming from a /browser moz.build because of DIST_SUBDIR = 'browser'.  For another app like b2g, it would just be dist/bin, I guess.  Now that I search the tree, I guess 'browser' and 'webapprt' are the only values used for DIST_SUBDIR...

Mainly, I am trying to keep it out of the toolkit omni.ja that gets shipped to all products because we've discussed it would be better to keep that smaller if possible for external apps.

This is an area I don't know very well... is there a good place to put it that is (a) in regular dist/bin, (b) easy to describe from jar.mn so it is loaded by the auto-created devtools.manifest, and (c) outside the toolkit omni.ja?
Flags: needinfo?(mh+mozilla)
As I see it, you can't have it both ways. Your choices are:

- have everything under the same resource://devtools thing in the same location and cope with the consequences, being that either you put it in browser/ and lose the server parts in the webapprt, or you put it in the root and ship the client part to things using firefox -app, or to webapprt for that matter

- the current status quo.

There is a third option, being that you make it an addon of some sort (system addon?), but you don't seem to be considering this option. Rhelmer was saying earlier today that all the infrastructure for system addons is in nightly already, so it's only a matter of putting the stuff in the right place, although I don't know what system addons mean for firefox -app or webapprt. Now, the problem is that we don't have (yet) a way to put the stuff in the right place for system addons, but I'm on it.
Flags: needinfo?(mh+mozilla)
Please don't create another to add things.
The system addon is defined and created for this purpose.
Also please don't put in resource://, at least not the skin part, as that will certainly block full themes in theming the devtools.
Actually, I think we should try to switch to system addons and I already started discussing this with :jryans and :mossop briefly.
But we have a timeframe issue here. The big devtools file move landed for Fx44 and we would like to ensure we don't shuffle the URL again in following releases.
That's why :jryans is pushing foward the better defined and easier "pdf.js like packaging".

But even while using system addons, we hit the same issue around client versus server.
Addons are disabled on b2g! Then, how are we gonna ship devtools server to b2g??
(b2g seems totally out of scope for system addons so far, it seems to be devoted to Desktop for now)
It would be sad if we end up with two distinct patterns between client and server.
Given all that, we may still want to keep a certain level of separation between client and server, at least during packaging.

To get back to original devtools inpulse that lead to this resource://devtools proposal:
 * complex mapping between runtime URI and source file
That forces us to document this complex mapping for each kind of resource:
  https://wiki.mozilla.org/DevTools/Hacking#Chrome_Content
2 ways to locale JSMs, one way for chrome (xul/html/js), one way for css.
Which makes me really sad!
 * hard to map the runtime to a local version of the source file
We are trying to ease contributing and we are now able to hot-reload devtools and maps to local files instead of runtime jar files, but that is very hacky as we look for chrome.manifest, compute a new one made of tons of override to override each devtools chrome file to a local one!? (Given that override rule support only files and not folders, this is really weak)

Regarding resource:// URI, we want to get away from chrome:// URI anyway.
We would like devtools to use an unprivileged scope in order to stop using xpcom and all privileged moz specifics. No matter what we choose: resource:// or a new specific scheme, it will break themes.
Moving to unprivileged scope isn't an immediate goal, but while we are at changing the URLs, I think this is a great time to do it.
Having said that, I would like to see if we can continue theming devtools with some tweaks made the the skin itself.
Flags: needinfo?(poirot.alex)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Bug 1203159 - Remove DevTools support from webapprt. r=myk

To enable resource://devtools, all DevTools files are consolidated at the app
level.  This means they are no longer available for sharing between multiple XUL
apps, such as browser and webapprt.  We'll need to devise a new method to bring
this support back to webapprt if desired.
Attachment #8673954 - Flags: review?(myk)
Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Add resource mapping jar.mn in all /devtools directories, to ensure it
activates.  (Only one such definition needs to be encountered.)  The
DevToolsModules template method is modified to install the files in their new
flattened add-on location.
Attachment #8673955 - Flags: review?(poirot.alex)
Attachment #8673955 - Flags: review?(mh+mozilla)
Bug 1203159 - Update each product's DevTools inclusion. r=glandium,ochameau

Applications that want any part of DevTools must now include it explicitly, so
that DIST_SUBDIR is in effect for all DevTools files if it is used by the app.
Attachment #8673956 - Flags: review?(poirot.alex)
Attachment #8673956 - Flags: review?(mh+mozilla)
Bug 1203159 - Update shim install locations. r=glandium,ochameau

Add more shim paths for JSM locations used during the 44 cycle for add-ons that
already migrated.  Also, clear any DIST_SUBDIR value for the shim directory only
to ensure the old paths are reachable.
Attachment #8673958 - Flags: review?(poirot.alex)
Attachment #8673958 - Flags: review?(mh+mozilla)
Bug 1203159 - Update install location for external libs. r=ochameau

External libs that do not yet use DevToolsModules need manual updating to match
the new system.
Attachment #8673959 - Flags: review?(poirot.alex)
Bug 1203159 - Correct module path in toolbox.reload. r=ochameau
Attachment #8673960 - Flags: review?(poirot.alex)
Comment on attachment 8673954 [details]
MozReview Request: Bug 1203159 - Remove DevTools support from webapprt. r=myk

Bug 1203159 - Remove DevTools support from webapprt. r=myk

To enable resource://devtools, all DevTools files are consolidated at the app
level.  This means they are no longer available for sharing between multiple XUL
apps, such as browser and webapprt.  We'll need to devise a new method to bring
this support back to webapprt if desired.
Comment on attachment 8673955 [details]
MozReview Request: Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Add resource mapping jar.mn in all /devtools directories, to ensure it
activates.  (Only one such definition needs to be encountered.)  The
DevToolsModules template method is modified to install the files in their new
flattened add-on location.
Comment on attachment 8673956 [details]
MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium

Bug 1203159 - Update each product's DevTools inclusion. r=glandium,ochameau

Applications that want any part of DevTools must now include it explicitly, so
that DIST_SUBDIR is in effect for all DevTools files if it is used by the app.
Comment on attachment 8673957 [details]
MozReview Request: Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau

Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau
Comment on attachment 8673958 [details]
MozReview Request: Bug 1203159 - Update shim install locations. r=glandium,ochameau

Bug 1203159 - Update shim install locations. r=glandium,ochameau

Add more shim paths for JSM locations used during the 44 cycle for add-ons that
already migrated.  Also, clear any DIST_SUBDIR value for the shim directory only
to ensure the old paths are reachable.
Comment on attachment 8673959 [details]
MozReview Request: Bug 1203159 - Update install location for external libs. r=ochameau

Bug 1203159 - Update install location for external libs. r=ochameau

External libs that do not yet use DevToolsModules need manual updating to match
the new system.
Comment on attachment 8673960 [details]
MozReview Request: Bug 1203159 - Clean up various incorrect paths. r=ochameau

Bug 1203159 - Correct module path in toolbox.reload. r=ochameau
I had forgotten the package manifest, hopefully good this time.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=605717ad7ebe
Comment on attachment 8673955 [details]
MozReview Request: Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

https://reviewboard.mozilla.org/r/22151/#review19767

::: devtools/server/jar.mn:6
(Diff revision 2)
> +%   resource devtools %modules/devtools/

As you say yourself in the commit message, there only needs to be one of those definitions. Why not just put it in e.g. devtools/shared/?

::: devtools/templates.mozbuild:28
(Diff revision 2)
> -    base = EXTRA_JS_MODULES
> +    base = FINAL_TARGET_FILES.chrome.devtools.modules

We tend to install all files under chrome/ through jar.mn. But adding 87 jar.mn files is not really an attractive alternative, and maintaining just one or two can be a hassle. So I'd tend to say "meh", but if you feel like it, feel free to move all the DevToolsModules things to devtools/server/jar.mn and devtools/shared/jar.mn.
Attachment #8673955 - Flags: review?(mh+mozilla)
Comment on attachment 8673956 [details]
MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium

https://reviewboard.mozilla.org/r/22153/#review19771

::: b2g/app.mozbuild:17
(Diff revision 2)
> +    '/devtools/server',
> +    '/devtools/shared',

I'm not entirely sure this would really be better, but I wonder if it shouldn't be using '/devtools' and devtools/moz.build would add client, server and shared depending on some AC_SUBST defined in confvars.sh (like MOZ_DEVTOOLS=(client|server|both)). Thoughts?
Attachment #8673956 - Flags: review?(mh+mozilla)
Comment on attachment 8673958 [details]
MozReview Request: Bug 1203159 - Update shim install locations. r=glandium,ochameau

https://reviewboard.mozilla.org/r/22157/#review19773
Attachment #8673958 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8673957 [details]
MozReview Request: Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau

https://reviewboard.mozilla.org/r/22155/#review19793

Looks good, but you would need to rebase over bug 1204812.
Note that try is still upset with this change, especially on Android.
Attachment #8673957 - Flags: review?(poirot.alex)
Comment on attachment 8673958 [details]
MozReview Request: Bug 1203159 - Update shim install locations. r=glandium,ochameau

https://reviewboard.mozilla.org/r/22157/#review19795

::: devtools/client/shims/moz.build:18
(Diff revision 2)
> +]

Don't you also need the DIST_SUBDIR trick over here also?
Looks like no, as the URLs seems to work fine here, but any idea why?

::: devtools/shared/shims/moz.build:31
(Diff revision 2)
> +]

You would also need to rebase for Console.jsm changes, sorry about that.
Attachment #8673958 - Flags: review?(poirot.alex) → review+
Attachment #8673959 - Flags: review?(poirot.alex) → review+
Comment on attachment 8673959 [details]
MozReview Request: Bug 1203159 - Update install location for external libs. r=ochameau

https://reviewboard.mozilla.org/r/22159/#review19797

Ok, but why don't they use DevtoolsModules yet?
Is that another followup?
Comment on attachment 8673960 [details]
MozReview Request: Bug 1203159 - Clean up various incorrect paths. r=ochameau

https://reviewboard.mozilla.org/r/22161/#review19799

I think there is other similar errors like this:
http://mxr.mozilla.org/mozilla-central/source/browser/components/newtab/PlacesProvider.jsm#24
(but should be covered by Console.jsm rewrite:)
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#25
Attachment #8673960 - Flags: review?(poirot.alex) → review+
Attachment #8673956 - Flags: review?(poirot.alex)
Comment on attachment 8673956 [details]
MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium

https://reviewboard.mozilla.org/r/22153/#review19801

I delegate my review to glandium for this particular patch which is just moz.builds.
Actually, I quite like his MOZ_DEVTOOLS proposal as it would allows to easily disable devtools, just to see what happens ;)
Comment on attachment 8673955 [details]
MozReview Request: Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

https://reviewboard.mozilla.org/r/22151/#review19803

+1 on having only one: `resource devtools %module` occurences, it is very disturbing to see many!
I'm quite happy to see all devtools resources in browser/chrome/devtools/, it looks like everything is there, right?
Attachment #8673955 - Flags: review?(poirot.alex) → review+
Summary: Investigate using resource://devtools/ → Migrate DevTools resources to resource://devtools/ URLs
https://reviewboard.mozilla.org/r/22151/#review19767

> As you say yourself in the commit message, there only needs to be one of those definitions. Why not just put it in e.g. devtools/shared/?

Makes sense, I'll stick to just one in shared.

> We tend to install all files under chrome/ through jar.mn. But adding 87 jar.mn files is not really an attractive alternative, and maintaining just one or two can be a hassle. So I'd tend to say "meh", but if you feel like it, feel free to move all the DevToolsModules things to devtools/server/jar.mn and devtools/shared/jar.mn.

Right, I was aware of the convention to use jar.mn files for this.  I strongly prefer the moz.build syntax myself, as I find it cleaner, and also we're able to enforce the correct path via the `DevToolsModules` template.  I'll add a comment here and in the jar.mn about the non-standard install process for these files.
https://reviewboard.mozilla.org/r/22153/#review19771

> I'm not entirely sure this would really be better, but I wonder if it shouldn't be using '/devtools' and devtools/moz.build would add client, server and shared depending on some AC_SUBST defined in confvars.sh (like MOZ_DEVTOOLS=(client|server|both)). Thoughts?

I think I like this approach!  I've switched to `MOZ_DEVTOOLS=(all|server|none)`.  The client currently implicitly requires the server, so `all` is a bit more clear about it.
https://reviewboard.mozilla.org/r/22157/#review19795

> Don't you also need the DIST_SUBDIR trick over here also?
> Looks like no, as the URLs seems to work fine here, but any idea why?

It's a bit tricky to follow, but it's correct to **not** do that here.  We want to keep `DIST_SUBDIR='browser'` set in this context because this will ensure the shims are installed under the `resource:///modules` tree (aka `resource://app/modules`) instead of `resource://gre/modules`.  Since the `devtools/client` files we are shimming were always previously accessed as `resource:///modules` we need to ensure the shims are still installed that way too.
https://reviewboard.mozilla.org/r/22159/#review19797

I'm still not sure what to do with them really, but yes, this is what bug 1201710 is about.
Attachment #8673954 - Flags: review?(myk) → review+
https://reviewboard.mozilla.org/r/22161/#review19799

Hmm, I guess this were from people pasting the old URLs into their patches which landed after the migration, but old URLs kept working because of the shims... :/

Hopefully we can enable migration warnings after this!
Comment on attachment 8673954 [details]
MozReview Request: Bug 1203159 - Remove DevTools support from webapprt. r=myk

Bug 1203159 - Remove DevTools support from webapprt. r=myk

To enable resource://devtools, all DevTools files are consolidated at the app
level.  This means they are no longer available for sharing between multiple XUL
apps, such as browser and webapprt.  We'll need to devise a new method to bring
this support back to webapprt if desired.
Attachment #8673954 - Flags: review+ → review?(myk)
Comment on attachment 8673955 [details]
MozReview Request: Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Add resource mapping jar.mn for DevTools.  The DevToolsModules template method
is modified to install the files in their new flattened add-on location.
Attachment #8673955 - Flags: review?(mh+mozilla)
Attachment #8673956 - Attachment description: MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium,ochameau → MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium
Attachment #8673956 - Flags: review?(mh+mozilla)
Comment on attachment 8673956 [details]
MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium

Bug 1203159 - Update each product's DevTools inclusion. r=glandium

A new configure option --with-devtools (which sets MOZ_DEVTOOLS) is added to
control whether all DevTools, just the server, or no DevTools are included.
This defaults to just the server.

Applications should also include /devtools within their moz.build tree, so that
DIST_SUBDIR is in effect for all DevTools files if it is used by the app.
Comment on attachment 8673957 [details]
MozReview Request: Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau

Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau
Comment on attachment 8673958 [details]
MozReview Request: Bug 1203159 - Update shim install locations. r=glandium,ochameau

Bug 1203159 - Update shim install locations. r=glandium,ochameau

Add more shim paths for JSM locations used during the 44 cycle for add-ons that
already migrated.  Also, clear any DIST_SUBDIR value for the shim directory only
to ensure the old paths are reachable.
Comment on attachment 8673959 [details]
MozReview Request: Bug 1203159 - Update install location for external libs. r=ochameau

Bug 1203159 - Update install location for external libs. r=ochameau

External libs that do not yet use DevToolsModules need manual updating to match
the new system.
Comment on attachment 8673960 [details]
MozReview Request: Bug 1203159 - Clean up various incorrect paths. r=ochameau

Bug 1203159 - Clean up various incorrect paths. r=ochameau
Attachment #8673960 - Attachment description: MozReview Request: Bug 1203159 - Correct module path in toolbox.reload. r=ochameau → MozReview Request: Bug 1203159 - Clean up various incorrect paths. r=ochameau
Comment on attachment 8673954 [details]
MozReview Request: Bug 1203159 - Remove DevTools support from webapprt. r=myk

https://reviewboard.mozilla.org/r/22149/#review19883

Myk already marked this r+ before, it's unchanged.
Attachment #8673954 - Flags: review+
Comment on attachment 8674709 [details]
MozReview Request: Bug 1203159 - Clean up various tests after DevTools resource move. r=me

https://reviewboard.mozilla.org/r/22279/#review19885
Attachment #8674709 - Flags: review+
Comment on attachment 8673954 [details]
MozReview Request: Bug 1203159 - Remove DevTools support from webapprt. r=myk

Bug 1203159 - Remove DevTools support from webapprt. r=myk

To enable resource://devtools, all DevTools files are consolidated at the app
level.  This means they are no longer available for sharing between multiple XUL
apps, such as browser and webapprt.  We'll need to devise a new method to bring
this support back to webapprt if desired.
Comment on attachment 8673955 [details]
MozReview Request: Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Add resource mapping jar.mn for DevTools.  The DevToolsModules template method
is modified to install the files in their new flattened add-on location.
Comment on attachment 8673956 [details]
MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium

Bug 1203159 - Update each product's DevTools inclusion. r=glandium

A new configure option --with-devtools (which sets MOZ_DEVTOOLS) is added to
control whether all DevTools, just the server, or no DevTools are included.
This defaults to just the server.

Applications should also include /devtools within their moz.build tree, so that
DIST_SUBDIR is in effect for all DevTools files if it is used by the app.
Comment on attachment 8673957 [details]
MozReview Request: Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau

Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau
Comment on attachment 8673958 [details]
MozReview Request: Bug 1203159 - Update shim install locations. r=glandium,ochameau

Bug 1203159 - Update shim install locations. r=glandium,ochameau

Add more shim paths for JSM locations used during the 44 cycle for add-ons that
already migrated.  Also, clear any DIST_SUBDIR value for the shim directory only
to ensure the old paths are reachable.
Comment on attachment 8673959 [details]
MozReview Request: Bug 1203159 - Update install location for external libs. r=ochameau

Bug 1203159 - Update install location for external libs. r=ochameau

External libs that do not yet use DevToolsModules need manual updating to match
the new system.
Comment on attachment 8673960 [details]
MozReview Request: Bug 1203159 - Clean up various incorrect paths. r=ochameau

Bug 1203159 - Clean up various incorrect paths. r=ochameau
Comment on attachment 8674709 [details]
MozReview Request: Bug 1203159 - Clean up various tests after DevTools resource move. r=me

Bug 1203159 - Clean up various tests after DevTools resource move. r=me
Bug 1203159 - Tweak environment test for appdir change. r=gfritzsche

Some DevTools files are changing how they are installed.  This environment test
relies on add-on manager infrastructure, which also loads some DevTools files.

To access these DevTools files in XPCShell from a Firefox build, we need to set
`firefox-appdir = browser` in the test manifest.  However, this then causes
`GMPProvider` to become enabled where it was not before, changing the values of
the test.

The test is adjusted to reflect the values `GMPProvider` reports.
Attachment #8675011 - Flags: review?(gfritzsche)
Comment on attachment 8673954 [details]
MozReview Request: Bug 1203159 - Remove DevTools support from webapprt. r=myk

Bug 1203159 - Remove DevTools support from webapprt. r=myk

To enable resource://devtools, all DevTools files are consolidated at the app
level.  This means they are no longer available for sharing between multiple XUL
apps, such as browser and webapprt.  We'll need to devise a new method to bring
this support back to webapprt if desired.
Comment on attachment 8673955 [details]
MozReview Request: Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Add resource mapping jar.mn for DevTools.  The DevToolsModules template method
is modified to install the files in their new flattened add-on location.
Comment on attachment 8673956 [details]
MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium

Bug 1203159 - Update each product's DevTools inclusion. r=glandium

A new configure option --with-devtools (which sets MOZ_DEVTOOLS) is added to
control whether all DevTools, just the server, or no DevTools are included.
This defaults to just the server.

Applications should also include /devtools within their moz.build tree, so that
DIST_SUBDIR is in effect for all DevTools files if it is used by the app.
Comment on attachment 8673957 [details]
MozReview Request: Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau

Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau
Comment on attachment 8673958 [details]
MozReview Request: Bug 1203159 - Update shim install locations. r=glandium,ochameau

Bug 1203159 - Update shim install locations. r=glandium,ochameau

Add more shim paths for JSM locations used during the 44 cycle for add-ons that
already migrated.  Also, clear any DIST_SUBDIR value for the shim directory only
to ensure the old paths are reachable.
Comment on attachment 8673959 [details]
MozReview Request: Bug 1203159 - Update install location for external libs. r=ochameau

Bug 1203159 - Update install location for external libs. r=ochameau

External libs that do not yet use DevToolsModules need manual updating to match
the new system.
Comment on attachment 8673960 [details]
MozReview Request: Bug 1203159 - Clean up various incorrect paths. r=ochameau

Bug 1203159 - Clean up various incorrect paths. r=ochameau
Comment on attachment 8674709 [details]
MozReview Request: Bug 1203159 - Clean up various tests after DevTools resource move. r=me

Bug 1203159 - Clean up various tests after DevTools resource move. r=me
Comment on attachment 8675011 [details]
MozReview Request: Bug 1203159 - Tweak environment test for appdir change. r=gfritzsche

Bug 1203159 - Tweak environment test for appdir change. r=gfritzsche

Some DevTools files are changing how they are installed.  This environment test
relies on add-on manager infrastructure, which also loads some DevTools files.

To access these DevTools files in XPCShell from a Firefox build, we need to set
`firefox-appdir = browser` in the test manifest.  However, this then causes
`GMPProvider` to become enabled where it was not before, changing the values of
the test.

The test is adjusted to reflect the values `GMPProvider` reports.
I believe all the telemetry test issues are finally resolved...

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1049e464eeb9
Comment on attachment 8673957 [details]
MozReview Request: Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau

https://reviewboard.mozilla.org/r/22155/#review20019

Please avoid modifying Console.jsm URLs.
Also, I imagine that's just a missing last moment rebase, but I can see old URLs in:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/shared/redux/middleware/test/head.js

::: browser/base/content/sanitize.js:25
(Diff revision 5)
> -                                  "resource://gre/modules/devtools/Console.jsm");
> +                                  "resource://devtools/Console.jsm");

You shouldn't update all Console.jsm paths.
Attachment #8673957 - Flags: review?(poirot.alex) → review+
https://reviewboard.mozilla.org/r/22155/#review20019

Okay, I'll watch out for that when I rebase again before landing.

> You shouldn't update all Console.jsm paths.

This particular case *should* have been `resource://gre/modules/Console.jsm`, if it were completely up to date with our latest file moving fun.  Since it was not, it was caught by the automated script here.

I chose to patch up the one-off cases separately, so this patch is automated changes only.

You can see that this case is fixed later on[1].

[1]: https://reviewboard.mozilla.org/r/22161/diff/5#index_header
Comment on attachment 8673955 [details]
MozReview Request: Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

https://reviewboard.mozilla.org/r/22151/#review20059
Attachment #8673955 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8673956 [details]
MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium

https://reviewboard.mozilla.org/r/22153/#review20061

::: configure.in:7463
(Diff revision 5)
> +dnl = Developer Tools
> +dnl ========================================================
> +
> +MOZ_ARG_WITH_STRING(devtools,
> +[  --with-devtools=(all|server|none)
> +                          Include all DevTools (with client), just the server,
> +                          or no tools.  Defaults to "server".],
> +    MOZ_DEVTOOLS=$withval)
> +
> +AC_SUBST(MOZ_DEVTOOLS)
> +
> +dnl ========================================================

I'm not entirely convinced an actual configure argument is useful. I'd tend to say configuration through confvars.sh is more than enough.

::: devtools/moz.build:12
(Diff revision 5)
> +if CONFIG['MOZ_DEVTOOLS'] == 'all' or CONFIG['MOZ_DEVTOOLS'] == 'server':

if CONFIG['MOZ_DEVTOOLS'] in ('all', 'server'):
Attachment #8673956 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8675011 [details]
MozReview Request: Bug 1203159 - Tweak environment test for appdir change. r=gfritzsche

https://reviewboard.mozilla.org/r/22315/#review20093

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:561
(Diff revision 2)
> +  // GMP plugin version defaults to null until GMPDownloader runs to update it.
> +  if (data.version) {

This sounds like a bug.
If the version is `null` before that, then either the plugin shouldn't be present in the list (i.e. treated as not installed) or this is some other bug possibly affecting normal Firefox builds too.

As it is now we a) might send wrong data and b) have incomplete test-coverage.
I think we need to find out if this a real issue or xpcshell-specific before this can go on the trains.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:600
(Diff revision 2)
> +    do_print(gmPlugin);

Left-over? Please remove this.
Attachment #8675011 - Flags: review?(gfritzsche)
https://reviewboard.mozilla.org/r/22315/#review20093

> This sounds like a bug.
> If the version is `null` before that, then either the plugin shouldn't be present in the list (i.e. treated as not installed) or this is some other bug possibly affecting normal Firefox builds too.
> 
> As it is now we a) might send wrong data and b) have incomplete test-coverage.
> I think we need to find out if this a real issue or xpcshell-specific before this can go on the trains.

We discussed this at more length over IRC.  The `firefox-appdir = browser` change alter the XPCShell test environment to also load browser modules it was not loading before.  This is triggering `GMPProvider` to enable and report 1 plugin available, while previously 0 plugins were available and `checkActiveGMPlugin` was skipped entirely.

However, the plugin version is currently set asynchronously by `GMPDownloader`[1] when it runs, which does not appear to happen during the test run.  This means the test can't access the plugin version.

:gfritzsche said they would file a follow up bug to improve the test after the next review.

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/GMPInstallManager.jsm#508

> Left-over? Please remove this.

Yes, I'll remove it, thanks!
https://reviewboard.mozilla.org/r/22153/#review20061

> I'm not entirely convinced an actual configure argument is useful. I'd tend to say configuration through confvars.sh is more than enough.

Okay, I removed the configure arg.

> if CONFIG['MOZ_DEVTOOLS'] in ('all', 'server'):

Thanks, changed to this version.
Comment on attachment 8673954 [details]
MozReview Request: Bug 1203159 - Remove DevTools support from webapprt. r=myk

Bug 1203159 - Remove DevTools support from webapprt. r=myk

To enable resource://devtools, all DevTools files are consolidated at the app
level.  This means they are no longer available for sharing between multiple XUL
apps, such as browser and webapprt.  We'll need to devise a new method to bring
this support back to webapprt if desired.
Comment on attachment 8673955 [details]
MozReview Request: Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Bug 1203159 - Add resource:// mapping in all DevTools directories. r=glandium,ochameau

Add resource mapping jar.mn for DevTools.  The DevToolsModules template method
is modified to install the files in their new flattened add-on location.
Comment on attachment 8673956 [details]
MozReview Request: Bug 1203159 - Update each product's DevTools inclusion. r=glandium

Bug 1203159 - Update each product's DevTools inclusion. r=glandium

A new configure option --with-devtools (which sets MOZ_DEVTOOLS) is added to
control whether all DevTools, just the server, or no DevTools are included.
This defaults to just the server.

Applications should also include /devtools within their moz.build tree, so that
DIST_SUBDIR is in effect for all DevTools files if it is used by the app.
Comment on attachment 8673957 [details]
MozReview Request: Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau

Bug 1203159 - Rewrite DevTools resource URLs. r=ochameau
Comment on attachment 8673958 [details]
MozReview Request: Bug 1203159 - Update shim install locations. r=glandium,ochameau

Bug 1203159 - Update shim install locations. r=glandium,ochameau

Add more shim paths for JSM locations used during the 44 cycle for add-ons that
already migrated.  Also, clear any DIST_SUBDIR value for the shim directory only
to ensure the old paths are reachable.
Comment on attachment 8673959 [details]
MozReview Request: Bug 1203159 - Update install location for external libs. r=ochameau

Bug 1203159 - Update install location for external libs. r=ochameau

External libs that do not yet use DevToolsModules need manual updating to match
the new system.
Comment on attachment 8673960 [details]
MozReview Request: Bug 1203159 - Clean up various incorrect paths. r=ochameau

Bug 1203159 - Clean up various incorrect paths. r=ochameau
Comment on attachment 8674709 [details]
MozReview Request: Bug 1203159 - Clean up various tests after DevTools resource move. r=me

Bug 1203159 - Clean up various tests after DevTools resource move. r=me
Comment on attachment 8675011 [details]
MozReview Request: Bug 1203159 - Tweak environment test for appdir change. r=gfritzsche

Bug 1203159 - Tweak environment test for appdir change. r=gfritzsche

Some DevTools files are changing how they are installed.  This environment test
relies on add-on manager infrastructure, which also loads some DevTools files.

To access these DevTools files in XPCShell from a Firefox build, we need to set
`firefox-appdir = browser` in the test manifest.  However, this then causes
`GMPProvider` to become enabled where it was not before, changing the values of
the test.

The test is adjusted to reflect the values `GMPProvider` reports.
Attachment #8675011 - Flags: review?(gfritzsche)
Attachment #8675011 - Flags: review?(gfritzsche) → review+
Comment on attachment 8675011 [details]
MozReview Request: Bug 1203159 - Tweak environment test for appdir change. r=gfritzsche

https://reviewboard.mozilla.org/r/22315/#review20343

Thanks, we will follow up on the test issue found in bug 1133310.
(In reply to J. Ryan Stinnett from comment #20)
> Applications that want any part of DevTools must now include it explicitly, so
> that DIST_SUBDIR is in effect for all DevTools files if it is used by the app.

This fails badly for Thunderbird and SeaMonkey because the paths are wrong when you try to include mozilla-central files from comm-central files. Since then, devtools has had some ipdl added, which now completely breaks their builds.

(Why does devtools need to live in DIST_SUBDIR anyway?)
(In reply to neil@parkwaycc.co.uk from comment #98)
> (In reply to J. Ryan Stinnett from comment #20)
> > Applications that want any part of DevTools must now include it explicitly, so
> > that DIST_SUBDIR is in effect for all DevTools files if it is used by the app.
> 
> This fails badly for Thunderbird and SeaMonkey because the paths are wrong
> when you try to include mozilla-central files from comm-central files. Since
> then, devtools has had some ipdl added, which now completely breaks their
> builds.

Hmm, I don't know much about how those apps are built.  Let's file a new bug to discuss it.  Ideally DevTools should become easier to include, not harder.

With products inside m-c, I needed to add /devtools to all of them, as in https://hg.mozilla.org/mozilla-central/rev/3f174efd3a89.

> (Why does devtools need to live in DIST_SUBDIR anyway?)

It does not *have to*, and in fact it only does so in /browser.  For other products like FxOS, Android, etc. the files are still at the root level.

The main reasoning for it was to keep DevTools out of the root (non-browser) omni.ja from Firefox builds so that other XUL apps don't incur file size cost if they wish to update by grabbing the root omni.ja to apply toolkit changes.
Depends on: 1217661
Depends on: 1217660
(In reply to comment #98)
> This fails badly for Thunderbird and SeaMonkey because the paths are wrong
> when you try to include mozilla-central files from comm-central files.

My bad, I was including from the wrong file; there's a special file to include mozilla-central files from.
(In reply to neil@parkwaycc.co.uk from comment #100)
> (In reply to comment #98)
> > This fails badly for Thunderbird and SeaMonkey because the paths are wrong
> > when you try to include mozilla-central files from comm-central files.
> 
> My bad, I was including from the wrong file; there's a special file to
> include mozilla-central files from.

We'll likely be fixing this even for apps outside of m-c in bug 1217687.
Blocks: 1218103
Depends on: 1231759
Was this change supposed to land in Fx 44?  Fx 45.0a2 Developer Edition cannot find resource://devtools/Console.jsm.  It can, however find both resource://gre/modules/devtools/Console.jsm and resource://gre/modules/Console.jsm.

The notice I received stated: Error: The paths for all devtools JS modules have changed from resource://gre/modules/devtools/* to resource://devtools/*.See https://bugzil.la/1203159 for more information.
(In reply to Chuck Baker from comment #102)
> Was this change supposed to land in Fx 44?

Yes, but I am not sure what you mean by "supposed to" exactly...  Anyway, 44 and later are meant to handle the change properly.

> Fx 45.0a2 Developer Edition
> cannot find resource://devtools/Console.jsm.  It can, however find both
> resource://gre/modules/devtools/Console.jsm and
> resource://gre/modules/Console.jsm.
> 
> The notice I received stated: Error: The paths for all devtools JS modules
> have changed from resource://gre/modules/devtools/* to
> resource://devtools/*.See https://bugzil.la/1203159 for more information.

Console.jsm did end up moving to a different place, separate from the rest of DevTools.  The error you are seeing I guess is too general to describe every file.  You can use "resource://gre/modules/Console.jsm" to access it in 44 and later.  There is also a shim in place that keeps the old path "resource://gre/modules/devtools/Console.jsm" working, but it's preferred to move away from that if possible.

For any further issues, please file new bugs or ask in #devtools on IRC / dev-developer-tools mailing list.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.