We should move the devtools code--which is currently scattered around various locations, like /browser/devtools and /toolkit/devtools--to a new top-level /devtools directory, to make it easier to reuse the code in multiple applications, like B2G Desktop (and the FxOS Simulator) in /b2g and the desktop webapp runtime in /webapprt.

We might hit similar branding challenges with devtools like we do with webapprt, see bug 846251.

Yay ! Is the browser/themes/shared/devtools/ directory being moved as well ?

(In reply to Tim Nguyen [:ntim] from comment #6) > Yay ! Is the browser/themes/shared/devtools/ directory being moved as well ? Yes, that's the current plan. It depends on Bug 896733

Without spamming the mailing lists, I approve of this proposal from a version control perspective. File copies and moves in Mercurial can blow up repository size due to the way Mercurial stores history. However, the total size of browser/devtools and toolkit/devtools is ~32 MB currently, which is ~1.5% of the ~2000 MB .hg/store directory from a fresh mozilla-central clone. While I wish the size increase were avoidable, the mailing list thread makes it seem like this will unblock even more awesomeness in devtools land, so 1.5% is a minor price to pay. r=gps as long as you `hg mv` the files.

(In reply to Gregory Szorc [:gps] from comment #8) > r=gps as long as you `hg mv` the files. Yes, I am scripting the move carefully with `hg mv` and friends to ensure history is preserved as expected.

I'll state here what I just mentioned in bug 1190024, but in a different way: Please make sure that the resulting build has the same files at the same place, that is, that files from the client side are in browser/omni.ja and files from the server side are in omni.ja (roughly).

(In reply to Mike Hommey [:glandium] from comment #10) > I'll state here what I just mentioned in bug 1190024, but in a different way: > > Please make sure that the resulting build has the same files at the same > place, that is, that files from the client side are in browser/omni.ja and > files from the server side are in omni.ja (roughly). I intend to preserve this split, don't worry. :)

In today's DevTools team meeting, we agreed that we'll review my plan and my migration scripts, as opposed to the patch output directly. I will also post the patch series in MozReview so interested parties can look over the changes, but I won't request specific review of most patches. I may request review of a few specific patches outside the general realm of DevTools, like build config changes.

There are still several unresolved test issues on try, but I believe we're close enough to begin reviews. I'll continue working on the try issues in the mean time. https://treeherder.mozilla.org/#/jobs?repo=try&revision=517f8cb02c89

I attempted to push the patch queue to MozReview to allow reviewing specific pieces, as well as for browsing the automated changes too, but MozReview returned an exception (filed bug 1202309). In place of that, here are two ways to inspect the full patch set: * Try pushlog: https://hg.mozilla.org/try/pushloghtml?changeset=517f8cb02c89 * Bitbucket: https://bitbucket.org/jryans/gecko/commits/all?search=8e6be88%3A%3A I will post specific patches here as regular attachments that I felt did need specific review. Additionally, I will post links to the migration plan and scripts for DevTools to review.

Comment on attachment 8657655 [details] Migration Plan Please review the migration plan[1] I've linked. The attachment is just the link itself. Feel free to comment at GitHub and then record a review result here. [1]: https://github.com/jryans/devtools-migrate/blob/master/README.md

Please review the migration script[1] I've linked. The attachment is just the link itself. Feel free to comment at GitHub and then record a review result here. A few specific patches (build details, etc.) will be reviewed in detail by others. [1]: https://github.com/jryans/devtools-migrate/blob/master/migrate.sh

Comment on attachment 8657659 [details] [diff] [review] Adjust build configs and test manifests Review of attachment 8657659 [details] [diff] [review]: ----------------------------------------------------------------- Please fold when landiung, obviously

Comment on attachment 8657660 [details] [diff] [review] Define DevToolsModules template for installing JS modules Review of attachment 8657660 [details] [diff] [review]: ----------------------------------------------------------------- As this originates from me, I'd rather this were reviewed by some other build peer.

Comment on attachment 8657655 [details] Migration Plan There is some copy paste between Chrome Content and Chrome Themes. Some stuff haven't been correctly updated in Chrome Themes, like the jar path. Also, it may help to add an example about `DevToolsModules`. Like a before/after EXTRA_JS_MODULES/DevToolsModules. What does `(lazy version)` means next to Cu.import/require examples? Overall the plan looks good. But I have some concerns about Cu.import and ressource path. 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.

(In reply to Alexandre Poirot [:ochameau] from comment #24) > Comment on attachment 8657655 [details] > Migration Plan > > There is some copy paste between Chrome Content and Chrome Themes. > Some stuff haven't been correctly updated in Chrome Themes, like the jar > path. Do you mean the path to the jar.mn file? That is accurate for the current patch series, the same jar.mn file is used for content and themes at the moment. Were you expecting separate files? > Also, it may help to add an example about `DevToolsModules`. > Like a before/after EXTRA_JS_MODULES/DevToolsModules. Good idea, I've added a before / after example. > What does `(lazy version)` means next to Cu.import/require examples? I only meant that you can also use the loader.lazy* versions like you would expect. To reduce confusion, I've added both formats.

(In reply to Alexandre Poirot [:ochameau] from comment #24) > Overall the plan looks good. > But I have some concerns about Cu.import and ressource path. > 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). That's true, so far I was assuming we would handle this mapping to local files in the loader. You are right that that does not work for the various resource URLs, since the loader does affect them. I had been assuming we would "solve" this by converting as many JS modules over to the loader as we can. Even if we took that as far as we could though, we would still have at least a few resource:// paths for worker scripts, a few JSMs, etc. so you are right that it's not as complete as it could be. > 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. I believe there is one obstacle that prevents this: currently we package server JS modules to dist/bin/modules, while client JS modules go to dist/bin/browser/modules. They end up as part of separate omni.ja files, which :glandium has mentioned in comment 10 it's important to maintain. If we wanted to have a single resource://devtools/ instead, we'd need to package all files, client and server, to the same place (most likely dist/bin/modules). As far as I understand, we could package everything to the same place like this and still ensure that non-browser apps only get the server because they would not include /devtools/client as a directory in their build process. However, I am not sure if this is correct. :glandium, am I missing something here? Why is it important to ensure the client files are packaged separately into dist/bin/browser/modules? When I originally read your comment 10, I assumed the reason was to ensure that non-browser apps never receive the client files, but that seems to already be taken care of by just not building the /devtools/client source tree.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26) > (In reply to Alexandre Poirot [:ochameau] from comment #24) > > Overall the plan looks good. > > But I have some concerns about Cu.import and ressource path. > > 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). > > That's true, so far I was assuming we would handle this mapping to local > files in the loader. You are right that that does not work for the various > resource URLs, since the loader does affect them. Ugh, typing. I meant to say: "You are right that that does not work for the various resource URLs, since the DevTools loader does **not** affect them."

Comment on attachment 8657655 [details] Migration Plan > Just devtools is unfortunately not enough: no products directly include the entire DevTools tree, so the build system won't understand you. Is there a plan / idea for a way for the build system to eventually be able to handle `./mach build devtools` without the "/*"? This could be a follow up, but it seems like it be a common problem for people. > To import() a file, the resource:// path is derived directly from the source tree path, but does have the gre difference between client and server as before. In the examples below that, there is one for `Cu.import("resource://gre/modules/devtools/shared/Loader.jsm")` and one for `Cu.import("resource:///modules/devtools/client/framework/gDevTools.jsm")`. To be clear, is this implying that `devtools/server` resources have gre while clients don't, and that `devtools/shared` is lumped into the server group for this distinction? Is there any case where a file in devtools/shared would *not* have the gre?

Comment on attachment 8657655 [details] Migration Plan > File: /devtools/client/themes/images/add.svg > Usage: chrome://devtools/skin/themes/images/add.svg I find the skin/ part a bit redundant. Maybe we can remove either `skin/` or `themes/` from the file path.

(In reply to Tim Nguyen [:ntim] from comment #29) > Comment on attachment 8657655 [details] > Migration Plan > > > File: /devtools/client/themes/images/add.svg > > Usage: chrome://devtools/skin/themes/images/add.svg > I find the skin/ part a bit redundant. It is redundant, I do not disagree, but... > Maybe we can remove either `skin/` or `themes/` from the file path. ...the main issue here is the same as with the chrome content I discussed on the mailing list. Here are the points I considered in more detail: * The chrome:// protocol handler requires chrome://<package>/<type>/foo/bar/bob.svg, where keyword can only be "content", "skin", or "locale", each meant for specific purposes. So, "skin" is fixed where it is for theme files. * The theme files should be placed somewhere under /devtools/client so they are only built with the client, and /devtools/client/themes seems natural. * For the chrome content case, the proposal with the most support was for the chrome:// path to match the source tree, except replacing "client" with the "content" segment, since it is required to be there. * We also hope to move away from chrome://, so try to avoid complexity. So, just as with chrome content, take the file path, replace path segment 2 with magic chrome type, in this case "skin".

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26) > :glandium, am I missing something here? Why is it important to ensure the > client files are packaged separately into dist/bin/browser/modules? When I > originally read your comment 10, I assumed the reason was to ensure that > non-browser apps never receive the client files, but that seems to already > be taken care of by just not building the /devtools/client source tree. non-browser apps include things that use firefox -app foo.ini, and webapprt.

Comment on attachment 8657657 [details] Migration Script Nice, looks good to me

(In reply to Brian Grinstead [:bgrins] from comment #28) > Comment on attachment 8657655 [details] > Migration Plan > > > Just devtools is unfortunately not enough: no products directly include the entire DevTools tree, so the build system won't understand you. > > Is there a plan / idea for a way for the build system to eventually be able > to handle `./mach build devtools` without the "/*"? This could be a follow > up, but it seems like it be a common problem for people. I am not yet sure how hard it is to improve this, but I agree it should be fixed somehow. I've filed bug 1202977. > > To import() a file, the resource:// path is derived directly from the source tree path, but does have the gre difference between client and server as before. > > In the examples below that, there is one for > `Cu.import("resource://gre/modules/devtools/shared/Loader.jsm")` and one for > `Cu.import("resource:///modules/devtools/client/framework/gDevTools.jsm")`. > To be clear, is this implying that `devtools/server` resources have gre > while clients don't, and that `devtools/shared` is lumped into the server > group for this distinction? Yes, server and shared files are packaged for all apps, and so are part of the base Gecko runtime, which are accessible under `resource://gre/`. Client files are packaged for the browser only, which is the Gecko application. Application resources are accessible under `resource://app/`, but this is also aliased to `resource:///` (no package name), which is the much more commonly used version. None of this is specific to DevTools, it's how all browser vs. toolkit modules are packaged today. In comment 24, Alex suggests we should go our own way with `resource://devtools/`. > Is there any case where a file in > devtools/shared would *not* have the gre? No, they would all have `resource://gre/`. I've added more detail on which parts use `gre` in the README.

Comment on attachment 8657655 [details] Migration Plan I've filed bug 1203159 to discuss the resource://devtools/ idea separately. I believe have addressed your other comments on the plan in updates to the document.

Comment on attachment 8657655 [details] Migration Plan > Also, a moz.build file using DevToolsModules must live in the same directory as the files to be > installed. Don't list files from a subdirectory in a moz.build from a parent directory. Just wondering. Could we make `DevToolsModules()` to throw if we pass anything else than just a file name? (../file.js or folder/file.js) May be that's already the case? Totally fine to do that as a followup.

(In reply to Alexandre Poirot [:ochameau] from comment #35) > Comment on attachment 8657655 [details] > Migration Plan > > > Also, a moz.build file using DevToolsModules must live in the same directory as the files to be > > installed. Don't list files from a subdirectory in a moz.build from a parent directory. > > Just wondering. Could we make `DevToolsModules()` to throw if we pass > anything else than just a file name? (../file.js or folder/file.js) May be > that's already the case? Totally fine to do that as a followup. Already done: https://hg.mozilla.org/try/diff/c85acf32715e/devtools/templates.mozbuild#l1.25

Comment on attachment 8657657 [details] Migration Script > Bug_912121___Create_shims_for_popular_modules_in_add_ons__rs_devtools.patch I wouldn't add deprecation warning until we discussed more in bug 1203159 and we are sure we aren't going to suggest yet another URLs! > Bug_912121___Create_shims_for_popular_DevTools_themes_in_add_ons__rs_devtools.patch > # devtools/client/moz.build > +if CONFIG['MOZ_BUILD_APP'] == 'browser': > + DIRS += ['themes/shims'] > # devtools/client/themes/shims/moz.build > + if CONFIG['MOZ_BUILD_APP'] == 'browser': > + JAR_MANIFESTS += ['jar.mn'] The first `if`, in client/moz.build is enough. > Bug_912121___Misc__DevTools_test_fixes_after_migration__rs_devtools.patch > # devtools/client/commandline/test/browser_cmd_appcache_valid.js > - markup: 'VV....VVVVVVVV', > + markup: 'VVV...VVVVVVV', Oh? The mapping removes a 'V' ? Funny and very unexpected side effect :o Other than that, it looks good. I'm trying to checkout all these changesets to give it a try and take another look at the overall modifications. I won't be able to do that today given the checkout/build time.

(In reply to Alexandre Poirot [:ochameau] from comment #37) > Comment on attachment 8657657 [details] > Migration Script > > > Bug_912121___Create_shims_for_popular_modules_in_add_ons__rs_devtools.patch > > I wouldn't add deprecation warning until we discussed more in bug 1203159 > and we are sure we aren't going to suggest yet another URLs! Okay, I'll land the deprecation warnings now, but disabled behind a pref. Filed bug 1204127 to enable when ready. > > Bug_912121___Create_shims_for_popular_DevTools_themes_in_add_ons__rs_devtools.patch > > # devtools/client/moz.build > > +if CONFIG['MOZ_BUILD_APP'] == 'browser': > > + DIRS += ['themes/shims'] > > # devtools/client/themes/shims/moz.build > > + if CONFIG['MOZ_BUILD_APP'] == 'browser': > > + JAR_MANIFESTS += ['jar.mn'] > > The first `if`, in client/moz.build is enough. Fair enough, removed. > > Bug_912121___Misc__DevTools_test_fixes_after_migration__rs_devtools.patch > > # devtools/client/commandline/test/browser_cmd_appcache_valid.js > > - markup: 'VV....VVVVVVVV', > > + markup: 'VVV...VVVVVVV', > > Oh? The mapping removes a 'V' ? Funny and very unexpected side effect :o Welcome to the strange world of GCLI tests.

> > +++ b/devtools/shared/shims/event-emitter.js > +for (let symbol of this.EXPORTED_SYMBOLS) { > + this[symbol] = module[symbol]; > +} event-emitter.js doesn't exports any symbol. It just exposes EventEmitter on modules.exports. So we should do: this["EventEmitter"] = module;

The previous comment was about this patch: > Bug_912121___Create_shims_for_popular_modules_in_add_ons__rs_devtools.patch

This comes from a scripted commit and would require a manual patch: > Bug 912121 - Package DevTools client themes in devtools.jar. rs=devtools > + skin/themes/images/tool-inspector.svg (themes/images/tool-inspector.svg) > + skin/themes/images/tool-inspector.svg (themes/images/tool-inspector.svg) That doesn't come from your modifition, but in linux and osx jar.mn, this line was already duplicated. > + skin/themes/images/editor-breakpoint@2x.png (themes/images/editor-breakpoint@2x.png) Note that this was missing in linux jar.mn. So your work also helped cleaning this duplicated mess! Thanks for finally merging this while doing "The Big Move™".

Loading devtools from local checkout is broken for various reasons. The first one is that the jar.mn file moved, you need to tweak that for example like this: # devtools/shared/Loader.jsm line 179 - return this._writeManifest(devtoolsDir).then(null, Cu.reportError); + return this._writeManifest(OS.Path.join(devtoolsDir, "client")).then(null, Cu.reportError); Also, this is less obvious, but gDevTools.jsm isn't ready to be used as a commonjs module. So you shouldn't convert Cu.import(gDevTools.jsm) to require(gDevTools.jsm) in this patch queue and let that for a followup. The precise issue is that, when we load from local checkout, we end up having two distinct instances of gDevTools.jsm, one using Cu.import() and the ressource:// URI, and another one spawned by SDK loader with file:///local/checkout/gecko URI. The most important one to modify is this one: # devtools/client/main.js line 9 -const { gDevTools } = require("devtools/client/framework/gDevTools.jsm"); +const { gDevTools } = require("resource:///modules/devtools/client/framework/gDevTools.jsm"); But there is also occurances in: devtools/shared/gcli/commands/csscoverage.js devtools/server/actors/csscoverage.js devtools/client/styleeditor/styleeditor-commands.js devtools/client/inspector/inspector-commands.js devtools/client/webconsole/console-commands.js

Comment on attachment 8657657 [details] Migration Script Biggest review ever. But looks good with the previous comments addressed.

I've also seen potential followups. - preferences: They are currently all in /browser/app/profile/firefox.js. We may move them to /devtools? - dev-edition: There is various files related to devedition in browser/, we may also move them if that's not just browser tweaks but real devtool features? browser/base/content/browser-devedition.js browser/base/content/test/general/browser_devedition.js browser/themes/shared/devedition.inc.css - css I was also wondering if we could prevent having to reference /devtools directly from browser.css? browser/themes/{windows,osx,linux}/browser.css:%include ../../../devtools/client/themes/responsivedesign.inc.css browser/themes/{windows,osx,linux}/browser.css:%include ../../../devtools/client/themes/commandline.inc.css - Console.jsm I have the feeling this module sounds more like a platform/toolkit module than devtools. Ideally, the console object exposed as global would be good enough... I'm even wondering if we should let this module in toolkit as-is. Alone in toolkit/devtools.

(In reply to Alexandre Poirot [:ochameau] from comment #44) > I've also seen potential followups. > - preferences: > They are currently all in /browser/app/profile/firefox.js. > We may move them to /devtools? > > - dev-edition: > There is various files related to devedition in browser/, we may also move > them if that's not just browser tweaks but real devtool features? > browser/base/content/browser-devedition.js > browser/base/content/test/general/browser_devedition.js > browser/themes/shared/devedition.inc.css Let's leave those in browser/, they are more related to browser integration than devtools. > - css > I was also wondering if we could prevent having to reference /devtools > directly from browser.css? > browser/themes/{windows,osx,linux}/browser.css:%include > ../../../devtools/client/themes/responsivedesign.inc.css > browser/themes/{windows,osx,linux}/browser.css:%include > ../../../devtools/client/themes/commandline.inc.css > I guess the suggestion is to move commandline.inc.css and responsivedesign.inc.css into browser/themes/shared, which sort of makes sense based on the document where the CSS is being loaded. Definitely follow-up material if we decided to do that.

(In reply to Alexandre Poirot [:ochameau] from comment #39) > > > > +++ b/devtools/shared/shims/event-emitter.js > > +for (let symbol of this.EXPORTED_SYMBOLS) { > > + this[symbol] = module[symbol]; > > +} > > event-emitter.js doesn't exports any symbol. It just exposes EventEmitter on > modules.exports. > So we should do: > this["EventEmitter"] = module; Right, I just noticed this too. I believe the shim needs the same module boilerplate as the real thing, since the real file supports loading as CommonJS module or Cu.import. Anyway, I'll change it.

(In reply to Alexandre Poirot [:ochameau] from comment #41) > This comes from a scripted commit and would require a manual patch: > > > Bug 912121 - Package DevTools client themes in devtools.jar. rs=devtools > > + skin/themes/images/tool-inspector.svg (themes/images/tool-inspector.svg) > > + skin/themes/images/tool-inspector.svg (themes/images/tool-inspector.svg) > > That doesn't come from your modifition, but in linux and osx jar.mn, this > line was already duplicated. > > > + skin/themes/images/editor-breakpoint@2x.png (themes/images/editor-breakpoint@2x.png) > > Note that this was missing in linux jar.mn. So your work also helped > cleaning this duplicated mess! > Thanks for finally merging this while doing "The Big Move™". I've added a manual jar.mn patch prior to processing to fix these issues.

(In reply to Alexandre Poirot [:ochameau] from comment #42) > Loading devtools from local checkout is broken for various reasons. > The first one is that the jar.mn file moved, you need to tweak that for > example like this: > > # devtools/shared/Loader.jsm line 179 > - return this._writeManifest(devtoolsDir).then(null, Cu.reportError); > + return this._writeManifest(OS.Path.join(devtoolsDir, > "client")).then(null, Cu.reportError); Okay, I've fixed this to use the correct manifest directory. > Also, this is less obvious, but gDevTools.jsm isn't ready to be used as a > commonjs module. > So you shouldn't convert Cu.import(gDevTools.jsm) to require(gDevTools.jsm) > in this patch queue and let that for a followup. > The precise issue is that, when we load from local checkout, we end up > having two distinct instances of gDevTools.jsm, one using Cu.import() and > the ressource:// URI, and another one spawned by SDK loader with > file:///local/checkout/gecko URI. Hmm, this part is pretty surprising to me, but making the changes you suggest does indeed fix things. My understanding of the SDK loader and JSMs was that require(foo.jsm) would resolve the JSM to a resource:// path and Cu.import() the JSM, so that it really ends up as an "alias" for Cu.import, like it appears to[1]. I would not have expected it to load the module twice or anything like that. In fact, even your suggested fix still passes a JSM to require, not Cu.import. We're just changing between module ID and explicit resource path. It seems like it *should* be possible for the SDK loader to handle this case correctly, but it's not obvious to me why it fails, and I don't really want jump down this particular rabbit hole right now. I've updated the require() rewrite script to preserve resource:// inside require() calls that were already written this way. Incidentally, this removes one manual patch I had added to revert back to resource:// for workers where that's the only option anyway, so it does seem simpler go this way, for workers and also now this reason too. [1]: https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/addon-sdk/source/lib/toolkit/loader.js#598

(In reply to Alexandre Poirot [:ochameau] from comment #44) > I've also seen potential followups. > - preferences: > They are currently all in /browser/app/profile/firefox.js. > We may move them to /devtools? Moving the prefs seems reasonable, filed bug 1204808. > > - dev-edition: > There is various files related to devedition in browser/, we may also move > them if that's not just browser tweaks but real devtool features? > browser/base/content/browser-devedition.js > browser/base/content/test/general/browser_devedition.js > browser/themes/shared/devedition.inc.css Agree with bgrins, these seem okay to leave in browser. > - css > I was also wondering if we could prevent having to reference /devtools > directly from browser.css? > browser/themes/{windows,osx,linux}/browser.css:%include > ../../../devtools/client/themes/responsivedesign.inc.css > browser/themes/{windows,osx,linux}/browser.css:%include > ../../../devtools/client/themes/commandline.inc.css Yeah, I wasn't sure what the best way to go with these was. Filed bug 1204810 for more discussion. > - Console.jsm > I have the feeling this module sounds more like a platform/toolkit module > than devtools. > Ideally, the console object exposed as global would be good enough... > I'm even wondering if we should let this module in toolkit as-is. Alone in > toolkit/devtools. Hmm, perhaps. Console.jsm is definitely used far more widely than any of the others. If we want to do that, it's probably simplest to put it in /toolkit/modules (no reason for a DevTools folder with one file) and just install it to its previous path. Since we already have the shims here, let's leave it to a follow up. Filed bug 1204812.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #48) > It seems like it *should* be possible for the SDK loader to handle this case > correctly, but it's not obvious to me why it fails, and I don't really want > jump down this particular rabbit hole right now. Yes, that has nothing to do with the file move. The issues isn't with the SDK loader, it relates more the the DevtoolsLoader which uses a new URI for the Cu.import. a file:// URI, whereas browser code still uses the ressource:// one. Also, I'm not sure gDevTools.jsm really supports reload. If we want to change how we import gDevTools.jsm, we have to ensure all callsites use the exact same way to import it (may be via a shim), and may be also tweak it to support reload. I would like to clean things up in bug 1188405, but I prefer to wait for the file move to happen first.

For anyone wondering, my current plan is to land this right after the merge next week. This will give us time to clean up any fallout of the move more easily, instead of being forced to uplift everything. I'm still fighting some test failures outside of DevTools, hopefully wrapping that up soon (for my own sanity).

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=826bdab3c24c Planning to land soon now that 44 is on fx-team.

https://hg.mozilla.org/integration/fx-team/rev/3b90d45a2bbc https://hg.mozilla.org/integration/fx-team/rev/f3cbe9dd4bb3 https://hg.mozilla.org/integration/fx-team/rev/c98255aa4d4b https://hg.mozilla.org/integration/fx-team/rev/efa64dfee210 https://hg.mozilla.org/integration/fx-team/rev/1145eb49fdc7 https://hg.mozilla.org/integration/fx-team/rev/cedc1af09fce https://hg.mozilla.org/integration/fx-team/rev/1550dafed618 https://hg.mozilla.org/integration/fx-team/rev/7054f652e387 https://hg.mozilla.org/integration/fx-team/rev/f5c50b4960f0 https://hg.mozilla.org/integration/fx-team/rev/ab023cef6c94 https://hg.mozilla.org/integration/fx-team/rev/b0a086cc8fa9 https://hg.mozilla.org/integration/fx-team/rev/8648bb20f956

https://hg.mozilla.org/mozilla-central/rev/3b90d45a2bbc https://hg.mozilla.org/mozilla-central/rev/f3cbe9dd4bb3 https://hg.mozilla.org/mozilla-central/rev/c98255aa4d4b https://hg.mozilla.org/mozilla-central/rev/efa64dfee210 https://hg.mozilla.org/mozilla-central/rev/1145eb49fdc7 https://hg.mozilla.org/mozilla-central/rev/cedc1af09fce https://hg.mozilla.org/mozilla-central/rev/1550dafed618 https://hg.mozilla.org/mozilla-central/rev/7054f652e387 https://hg.mozilla.org/mozilla-central/rev/f5c50b4960f0 https://hg.mozilla.org/mozilla-central/rev/ab023cef6c94 https://hg.mozilla.org/mozilla-central/rev/b0a086cc8fa9 https://hg.mozilla.org/mozilla-central/rev/8648bb20f956

Not even a test failure, congrats!!

(In reply to Tim Nguyen [:ntim] from comment #29) > Comment on attachment 8657655 [details] > Migration Plan > > > File: /devtools/client/themes/images/add.svg > > Usage: chrome://devtools/skin/themes/images/add.svg > I find the skin/ part a bit redundant. > > Maybe we can remove either `skin/` or `themes/` from the file path. Indeed, please remote the "/themes", part. Because then I can redirect the "chrome://devtools/skin/" to "browser/devtools" in my themes, which allows compatibility across version of Firefox.

Created bug 1207976 to remove '/themes' from chrome://devtools/skin/themes/