In order to ship DevTools faster, we need to convert DevTools as an add-on. Ideally DevTools would still ship into Nightly and Dev-Edition. So it would be great if DevTools is integrated into mozilla-central as an add-on as well. That would help also hosting DevTools codebase on github as both repos would generate an add-on. System-addon, i.e. the addons put into /browser/features folder in the firefox package, look like the only way to distibute built-in add-on into Firefox. That, because of the required add-on signature. Only system-addon do not require to be signed as we consider Firefox installation package as safe. So do we consider the system add-ons contained in the installation package.

Comment on attachment 8873899 [details] Bug 1369801 - DevTools as system add-on This patch tweaks moz.build and jar.mn in order to change how devtools is distributed. It is quite conservative. It mostly move files from browser/omni.jar (chrome/devtools/) to browser/features/devtools@mozilla.org.xpi. It cleans up allowed-dupes as all devtools files changed to new paths and it looks like many dupes went away. package-manifest.in is much simplier as we just ship whatever is in devtools@mozilla.org.xpi! I had to tweak things for the preference files in order to have static URL to them, so I put them in chrome://devtools/content/. Also, we no longer can use EXTRA_COMPONENTS for xpcom written in JS. I haven't found any way to ensure shipping them in the devtools xpi. So instead I manually put them in it via FINAL_TARGET_FILES instruction and reference their .manifest files from jar.mn. Otherwise here is the file layout in the final addon: * /chrome/content/ contains all files referenced by /devtools/client/jar.mn content lines * /chrome/skin/ contains all files referenced by /devtools/client/jar.mn skin lines * /chrome/locale/ contains locales specified by /devtools/client/locales/jar.mn * /modules contains all files references in moz.build via DevToolsModules() (implemented in templates.mozbuild) * bootstrap.js * install.rdf * All JS XPCOM and their manifests A quick note about l10n repack because it is extremelly tricky. This line seems to be all but optional: http://searchfox.org/mozilla-central/source/browser/locales/Makefile.in#110 @$(MAKE) -C ../../devtools/client/locales AB_CD=$* XPI_NAME=locale-$* XPI_ROOT_APPID='$(XPI_ROOT_APPID)' And adding a locale rule for each @AB_CD@ in the preprocessed jar.mn here: http://searchfox.org/mozilla-central/source/devtools/client/locales/jar.mn looks also extremelly important to pass the l10n-repack step! We have to have a valid line like this for each @AB_CD@: % locale devtools @AB_CD@ %locale/@AB_CD@/devtools/client/ It can them map to only one locale folder, like what I did in this patch. I mapped everything to en-US. But there has to be a definiton for each @AB_CD@ and only for @AB_CD@. That's the main weirdness. We can't just hardcode the things like this: % locale devtools en-US %locale/en-US locale/en-US/ (en-US/*) % locale devtools fr %locale/fr locale/fr/ (fr/*) ... and so on ... Instead, we have to only define one and only one, the @AB_CD@ one! For now, if we don't go to github just yet, we should revert my patch to: features/devtools@mozilla.org] chrome.jar: # Still move the locale to devtools xpi % locale devtools @AB_CD@ %locale/@AB_CD@/devtools/client/ locale/@AB_CD@/devtools/client/ (%*) But still support all m-c locale and still fetch them from m-c localization. I imagine (%*) do the trick. But we would have to ensure it still correctly pull localization. If we do go to github and devtools-l10n repo starts becoming the canonical repo for translation devtools, we would have to land localization into /devtools/client/locales, next to en-US and have a jar.mn that looks like pocket's one: http://searchfox.org/mozilla-central/source/browser/extensions/pocket/locale/jar.mn

(In reply to Alexandre Poirot [:ochameau] (Back on 26th) from comment #2) > Also, we no longer can use EXTRA_COMPONENTS for xpcom written in JS. I > haven't found any way to ensure shipping them in the devtools xpi. So > instead I manually put them in it via FINAL_TARGET_FILES instruction and > reference their .manifest files from jar.mn. This works well in Lightning, maybe there are a few things you can copy from there. What we do is to export XPI_NAME and USE_EXTENSION_MANIFEST as in https://dxr.mozilla.org/comm-central/source/calendar/lightning/moz.build#19 but I don't know if one of those is what makes it work. We are then using EXTRA_COMPONENTS in other files (e.g. /calendar/import-export/moz.build) that are included as DIRS to calendar/lightning. Happy to chat if there is something I can help with.

(In reply to Philipp Kewisch [:Fallen] from comment #3) > This works well in Lightning, maybe there are a few things you can copy from > there. What we do is to export XPI_NAME and USE_EXTENSION_MANIFEST as in > https://dxr.mozilla.org/comm-central/source/calendar/lightning/moz.build#19 > but I don't know if one of those is what makes it work. We are then using > EXTRA_COMPONENTS in other files (e.g. /calendar/import-export/moz.build) > that are included as DIRS to calendar/lightning. Happy to chat if there is > something I can help with. Thanks a lot for shiming in on such complex subject!! I tried setting XPI_NAME but got into this check: http://searchfox.org/mozilla-central/source/config/rules.mk#1264-1271 Then I tried to reset DIST_SUBDIR to '', but things broke even more. The jar.mn were behaving very incorrectly and the final xpi in browser/features/devtools@mozilla.org was missing various ressources. I haven't tried to look deeper. I just supposed [features/devtools@mozilla.org] syntax in jar.mn was /browser/ specific, but I may be completely wrong. TBH, these magic build system tricks (EXTRA_COMPONENTS, XPI_NAME and even jar.mn) are not necessarely helpful. The current setup (using FINAL_TARGET_FILES) is closer to what happen at runtime and may help devtools getting away of mozilla-central build system tricks. I mean that folders and files layout is more similar between sources and runtime. I think that most addons from /browser/extensions tend to just drop xpi sources as-is in mozilla-central and just want to package the add-on back into a xpi in browser/features/system-addon@mozilla.org. I would even prefer using chrome.manifest instead of jar.mn, but it is impossible (or really not obvious) because of the l10n repack.

So Thunderbird actually sets XPI_ROOT_APPID here: https://dxr.mozilla.org/comm-central/source/mail/defs.mk#8 If FINAL_TARGET_FILES is also suggested by build peers that would be fine with me, maybe it would then make sense to remove the XPI_NAME hacks and also make the changes in the comm repos.

DevTools bug triage (filter on CLIMBING SHOES).

Some regressions linked to preference files processing, tracked in Bug 1371298. Try (linux64 opt) after a rebase on top of the patches attached to Bug 1371298: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d53a89fa2672bab77f1e9accb578af6bb32489a1

Previous try was only including the patches from Bug 1371298, and nothing from this bug, so please ignore it. Made another try push after that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b3313745b07bd55b55742947b693df2d05d6e91 All dt* jobs failed. Tried to package locally, it was failing due to undeclared duplicated files. Added them to the allowed_dupes.mn and pushed again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11317cf4a7ee65485f942a8fcc5b943175ca9c0c Still failures for all dt jobs. When trying the application built by ./mach package, devtools are actually missing. When building locally with ./mach build, everything seems to work, except for the Browser Toolbox which is broken at the moment.

(In reply to Julian Descottes [:jdescottes] from comment #8) > Previous try was only including the patches from Bug 1371298, and nothing > from this bug, so please ignore it. > > Made another try push after that: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=0b3313745b07bd55b55742947b693df2d05d6e91 > > All dt* jobs failed. Tried to package locally, it was failing due to > undeclared duplicated files. Added them to the allowed_dupes.mn and pushed > again: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=11317cf4a7ee65485f942a8fcc5b943175ca9c0c > > Still failures for all dt jobs. When trying the application built by ./mach > package, devtools are actually missing. > > When building locally with ./mach build, everything seems to work, except > for the Browser Toolbox which is broken at the moment. From the symptoms, it sounds a lot like something is missing from package-manifest.ini, but I see it lists `@RESPATH@/browser/features/*`, so not sure exactly what. It might help look at objdir/dist locally and see if there are additional DevTools files that need to be allowed.

It fails on devtools.js line 10 http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools.js#10 Isn't there something wrong with "devtools-shim" registration? I don't remember having done anything specific for this... It may miss that if I stripped it: http://searchfox.org/mozilla-central/source/browser/installer/package-manifest.in#634

Thank you both for having a look! (In reply to Alexandre Poirot [:ochameau] (Back on 26th) from comment #10) > It fails on devtools.js line 10 > http://searchfox.org/mozilla-central/source/devtools/client/framework/ > devtools.js#10 > > Isn't there something wrong with "devtools-shim" registration? I don't > remember having done anything specific for this... > > It may miss that if I stripped it: > http://searchfox.org/mozilla-central/source/browser/installer/package- > manifest.in#634 Good call, it's just that! How did you find the information about the failure? I was struggling to get any log since DevTools were not available. try https://treeherder.mozilla.org/#/jobs?repo=try&revision=794ad8de634ec3fef079ca8cd26b37c0a597b56b

Ok so almost all dt* failed, but the situation is actually much better. All/most of the errors are related to the same root cause: TypeError: 'getInterface' called on an object that does not implement interface Window. at getDOMWindowUtils@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/stylesheet/utils.js:19:10

Another weird thing, aboutdebugging tests did not fail on try, but they fail locally because they don't manage to open the toolbox for addon debugging.

Regarding the getInterface error: The window/this object available in devtools/client/webconsole/net/main.js is no longer a Window object but a Sandbox object. When passing it to stylesheet/utils::loadSheet, the following code fails: > return window.QueryInterface(Ci.nsIInterfaceRequestor). > getInterface(Ci.nsIDOMWindowUtils); For now I worked around this by passing this.document.defaultView instead of this when calling loadSheet (https://hg.mozilla.org/try/diff/159afc35f5cc/devtools/client/webconsole/net/main.js). After this change, tests look much better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0e6bf2d9a4bc51b011d991b3464b82efaea2035 We have failures in browser_all_files_referenced.js, as well as in browser toolbox related files. (In reply to Julian Descottes [:jdescottes] from comment #13) > Another weird thing, aboutdebugging tests did not fail on try, but they fail > locally because they don't manage to open the toolbox for addon debugging. My previous comment was actually incorrect. I was expecting aboutdebugging tests to be in the first dt* buckets, but they ended up being in the last ones. Which means that the browser toolbox is also busted on try. I'll work on fixing that now.

From what I can see so far, the profile we use to boot a new instance of Firefox for the Browser Toolbox is not compatible with the DevTools addon. When starting the Browser Toolbox, a new instance of Firefox pops up but no window appears. If I create a new window, I get a regular Firefox window, but DevTools are not available. I extracted the profile from my local objdir/tmp/scratch_user/chrome_debugger_profile and reused it with a locally packaged Firefox and DevTools are not starting there either.

When we start the Browser Toolbox we create a profile based on the current profile. We mainly copy the prefs.js to a new folder which will be the home of the profile dedicated to the BrowserToolbox. The following preference `user_pref("extensions.lastAppVersion", "55.0a1");` seems to prevent the addonmanager from booting the system addons. [Note that the application is also starting in safe mode. This doesn't seem to prevent using system addons so we should be fine in Nightly/DevEdition, however I'm wondering how this will work out for release? DevTools will be installed as a "regular" extension, and safe mode will most likely disable them? We might have to special case devtools@mozilla.org here.]

Try push with an ugly workaround to allow the browser toolbox to boot: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba3de3e9e85ed883e64094a836b38ff2eaa25548 (In reply to Julian Descottes [:jdescottes] from comment #16) > > [Note that the application is also starting in safe mode. This doesn't seem > to prevent using system addons so we should be fine in Nightly/DevEdition, > however I'm wondering how this will work out for release? DevTools will be > installed as a "regular" extension, and safe mode will most likely disable > them? We might have to special case devtools@mozilla.org here.] As usual, part of my previous comment is wrong. We actually don't start in safe mode. However the use case is still interesting for Release and Beta. In safe mode, the DevTools addon will most likely be disabled and we might have to do something to workaround that.

Green try for devtools tests on linux64-opt https://treeherder.mozilla.org/#/jobs?repo=try&revision=94b1fe00ffecba1995d2227504459e441c1ffa5f

Almost everything is working now except for the two components (webideCLI.js and devtools-startup.js) which were mentioned by Alex in comment 2. They are actually not registered and are not starting at all (which means --jsconsole, --jsdebugger, --webide etc... don't work right now). They are included by adding % manifest instructions in devtools/client/jar.mn: > % manifest devtools-startup.manifest > % manifest webideComponents.manifest devtools-startup.manifest lives next to this jar.mn file while the webide one is devtools/client/webide/components. Both are normally moved up to the root via their respective moz.build files: > FINAL_TARGET_FILES.features['devtools@mozilla.org'] += [ > 'devtools-startup.js', > 'devtools-startup.manifest', > ] (same goes for webide) For reference the content of the devtools-startup.manifest file is: > component {9e9a9283-0ce9-4e4a-8f1c-ba129a032c32} devtools-startup.js > contract @mozilla.org/devtools/startup-clh;1 {9e9a9283-0ce9-4e4a-8f1c-ba129a032c32} > category command-line-handler m-devtools @mozilla.org/devtools/startup-clh;1 There are no errors during build or package, but the component is not loaded (called dump() at the beginning of the file, but can't see anything in the logs).

I think I understood why my components are not registered. The issue could come from the fact that we have a bootstrap addon. It seems you can't use chrome.manifest to define command line handlers in bootstrap addons. Instead they should be dynamically registered in bootstrap.js [1] (asked for confirmation in #addons but no reply so far) Another option is to have a cli component that stays in m-c and that calls the DevToolsShim. But I am a bit worried about the startup sequence in this case. [1] example at https://github.com/Noitidart/Restore--remote/blob/master/bootstrap.js

(In reply to Julian Descottes [:jdescottes] from comment #48) > I think I understood why my components are not registered. The issue could > come from the fact that we have a bootstrap addon. > It seems you can't use chrome.manifest to define command line handlers in > bootstrap addons. Instead they should be dynamically registered in > bootstrap.js [1] Hmm, it seems like it _should_ be possible from an add-on, or at least it was at some point in the past: https://dxr.mozilla.org/addons/search?q=command-line-handler&redirect=true Hard to tell if it is expected to still work now, though.

(In reply to Julian Descottes [:jdescottes] from comment #48) > I think I understood why my components are not registered. The issue could > come from the fact that we have a bootstrap addon. > It seems you can't use chrome.manifest to define command line handlers in > bootstrap addons. Instead they should be dynamically registered in > bootstrap.js [1] Okay, we learned more on MDN[1], the issue is `category` is not processed in the manifest for bootstrapped add-ons. [1]: https://developer.mozilla.org/en-US/docs/Chrome_Registration#Instructions_supported_in_bootstrapped_add_ons

Thanks for finding the actual documentation! I went ahead and registered the components in the bootstrap. Testing locally all the command-line flags seem to work fine. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d90a74d5105b2fe734e1436a727081b8ff5bd68

(In reply to Julian Descottes [:jdescottes] from comment #51) > > I went ahead and registered the components in the bootstrap. Testing locally > all the command-line flags seem to work fine. > try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=5d90a74d5105b2fe734e1436a727081b8ff5bd68 Mochitests are fine, but xpcshell tests are failing: they can't load any devtools resource. (Note that this is not a new issue related to the latest changes attached today, I simply did not try runnning xpc-shell tests before)

I tried to manually add `manifest browser/features/devtools@mozilla.org/chrome.manifest` in the root chrome.manifest used for the xpc-shell tests. With this the devtools xpcshell tests run fine. Looking at other system addons, it looks like formautofill manages to run xpcshell tests by loading their system addon in the head.js of the tests [1]. I will try to use the same approach here. [1] http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/browser/extensions/formautofill/test/unit/head.js#47-58

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9841d13b20efa53d264d9737696b31674a53dc86

One last remaining issue related to legacy addon debugging, is actually linked to a missed dependency on devtools: http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2232-2243 The codepath above is never executed with devtools built as an addon. We need to use the DevToolsShim here instead.

(mostly) green try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=af2d7ecb13b4cc80c20e5a2adaafa8eb4f34f135 with no skipped test

Ran another try this time on all platforms: a lot of android test failures. To be investigated. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc19b1d1652e8b841c608e038d5e8eaa8445f012

There is still an issue for xpc-shell tests: using --jsdebugger when starting xpcshell tests expects devtools to be available which is not the case by default when starting xpcshell tests: http://searchfox.org/mozilla-central/source/testing/xpcshell/head.js#400 For devtools xpcshell tests, we load and start the addon explicitly in the head executed before any xpcshell test. We probably need to do something similar here.

Two things that prevented loading devtools resources on android: - install.rdf targets the application id for Firefox (ec8030f7-c20a-464f-9b0e-13a3a9e97384), while Fennec is aa3c5121-dab2-40e2-81ca-7ea25febc110 - bootstrap.js requires devtools/client files on startup. Since devtools/client is not shipped on Fennec, this fails. I know :ochameau had plans in mind for the Fennec build, but in the meantime I am trying to do as follows: when building as 'server', replace install.rdf and bootstrap.js with custom versions that can work for Fennec. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15c2725f0d9601d04670163f4630a71bac324515

Mochitests are now passing fine on android builds, however I still can't make the xpcshell tests run for android. For xpcshell tests running on Desktop builds, we install the DevTools addon explicitly via a new head.js file inspired from http://searchfox.org/mozilla-central/source/browser/extensions/shield-recipe-client/test/unit/head_xpc.js#11-21. Basically we navigate to the xpi for devtools and provide it to the addon manager. For now I can't make this work on Android. The folder corresponding to "GreD" does not contain the addon. If I first run Fennec before starting the xpcshell test, then this folder will contain the xpi (under /features/devtools.xpi instead of /browser/features/devtools.xpi on desktop). But when running the test in isolation this isn't the case. I asked on #mobile to see if anyone had an idea on how to retrieve/load our xpi here, we'll see if we can find a workaround. Otherwise, maybe we need to reevaluate the importance of running our xpcshell tests on android?

I should add that this only concerns 4 xpcshell tests: - devtools/shared/discovery/tests/unit/test_discovery.js - devtools/shared/platform/content/test/test_stack.js - devtools/shared/qrcode/tests/unit/test_encode.js - devtools/shared/security/tests/unit/test_encryption.js All the other tests are either in /client or skipped if android.

Still two weird browser toolbox bugs: - with the latest implementation (creating a user.js dynamically) we get a prompt about "checking addon compatiblity" - if you quit the BT and reopen it quickly after, it will be blank. Waiting ~5seconds before reopening it bypasses the issue

> - if you quit the BT and reopen it quickly after, it will be blank. > Waiting ~5seconds before reopening it bypasses the issue Nevermind, this is actually already happening in today's nightly.

(In reply to Julian Descottes [:jdescottes] from comment #111) > Still two weird browser toolbox bugs: > - with the latest implementation (creating a user.js dynamically) we get a > prompt about "checking addon compatiblity" Since we have a custom pref file now, we can probably tweak some of them like these ones: http://searchfox.org/mozilla-central/source/testing/marionette/server.js#209-211 // Turn off extension updates so they do not bother tests ["extensions.update.enabled", false], ["extensions.update.notifyUser", false], http://searchfox.org/mozilla-central/source/testing/marionette/server.js#59-69 // Disable automatic downloading of new releases. // // This should also be set in the profile prior to starting Firefox, // as it is picked up at runtime. ["app.update.auto", false], // Disable automatically upgrading Firefox. // // This should also be set in the profile prior to starting Firefox, // as it is picked up at runtime. ["app.update.enabled", false],

Comment on attachment 8876466 [details] Bug 1369801 - dt-addon-xpcshell: move memory xpcshell test to avoid loading devtools preferences I'll try to move forward with bug 1364876 which is reduntant here.

Comment on attachment 8876464 [details] Bug 1369801 - dt-addon: use window object in webconsole/net/main to load stylesheets; This can be landed separately, I already had to land some modification like that somewhere else a long time ago.

Comment on attachment 8880529 [details] Bug 1369801 - dt-addon-tests: update specificity of selector in firebug-theme.css; Whaa, is it related to becoming a system add-on, it is scary if it is?!

Thanks for having a look! (In reply to Alexandre Poirot [:ochameau] from comment #116) > Comment on attachment 8880529 [details] > Bug 1369801 - update specificity of selector in firebug-theme.css > > Whaa, is it related to becoming a system add-on, it is scary if it is?! That's one of the sad ones. This is a workaround to avoid failing browser_parsable_css.js. I would love to have a good explanation here, but the thing is that this consistently/100% failed on try (eg https://treeherder.mozilla.org/#/jobs?repo=try&revision=42d48dc0d393b3d573ca71bfb08198a06fbb4ee3) on all platforms, but never failed locally. In theory, the current rule is incorrect, since --proportional-font-family is only defined in `:root.theme-firebug` in common.css. > :root { > font-size: 11px; > font-family: var(--proportional-font-family); > } But if firebug-theme.css is loaded, the .theme-firebug class should also be applied on the root element so we should not have this issue in a real use case. But the test doesn't care about that as it loads all css files regardless of any logic we might have. Why is it only failing on try? I don't know! Even if I add `font-size: var(--i-do-not-exist);` in the :root selector, the test doesn't complain locally. But if I change :root to * I start to get failures. We are using a HiddenFrame for this test (http://searchfox.org/mozilla-central/source/toolkit/modules/HiddenFrame.jsm), would it behave differently so that :root would match something on try but not locally?? Why is this only failing with the system addon? I don't know either :( One thing I noted is that if I pause the test and open devtools manually, they are unstyled for 2-3 seconds before getting their styles. On mozilla central they are styled immediately. Sorry I don't have a better explanation, but as I failed to reproduce and investigate locally I gave up and simply updated the rule. As a sanity check, here is a try push with this workaround reverted => https://treeherder.mozilla.org/#/jobs?repo=try&revision=183eaff934933091a49c35f13686c588849020a9 It should normally fail the browser_parsable_css.js test. (In reply to Alexandre Poirot [:ochameau] from comment #115) > Comment on attachment 8876464 [details] > Bug 1369801 - hack to get real window from sandbox > > This can be landed separately, I already had to land some modification like > that somewhere else a long time ago. So to be clear I *can* land this patch and the one about firebug separately since they won't make any test fail. They just seem a bit random (especially the sandbox->window one) when taken out of context.

Some good news about my issues related to loading the browser toolbox with a delayed addon startup. The issue is actually quite simple. TooboxProcess.jsm initializes an instance of Firefox that loads on "devtools/client/framework/toolbox-process-window.xul". This XUL file loads several scripts, including one devtools script "toolbox-process-window.js". This is usually this file which "boots" DevTools in the case of the BrowserToolbox. (for the record, today none of the usual code from devtools-startup is executed because we never get the browser-delayed-startup-finished event in this case) If the devtools addon is not loaded from the beginning, then the preferences are not loaded either and the file fails to load. Simply loading the preferences in this file too fixes the problem. > let {DevToolsPreferences} = > Cu.import("chrome://devtools/content/preferences/DevToolsPreferences.jsm", {}); > DevToolsPreferences.loadPrefs(); Here is a try push with this change + reverting to my earlier workaround for skipping the addon update message (I don't think we can bypass it by playing with existing prefs, this will need an update on the addons side). https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b67a60e6057956005f456281ea5855f0343b1e7 (In reply to Julian Descottes [:jdescottes] from comment #117) > > As a sanity check, here is a try push with this workaround reverted => > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=183eaff934933091a49c35f13686c588849020a9 > It should normally fail the browser_parsable_css.js test. Small follow up here, the browser_parsable_css test is in dt5 (the job retriggered a bunch of times). All dt5s are orange, but that's because another test was failing with this revision. In total I think browser_parsable_css failed in 30% of the tests. So despite my initial impression it's actually a devtools-addon-only-intermittent.

Here is a first green try push, linux64 only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2328444d52d9a0a7cb83308f8ad79f16c877570&selectedJob=113583485 Alex: for your first patch, I'd like you to first review all the changesets I've annotated dt-addon-build. Think we should just merge them with your first initial patch and then maybe get a final review from someone else in the team.

Comment on attachment 8885648 [details] Bug 1369801 - dt-addon: remove dump() from devtools addon bootstrap; https://reviewboard.mozilla.org/r/156156/#review161658 You can merge that into the first changeset. Or do a "cleanup bootstrap.js" changeset where you remove all the useless bits.

Comment on attachment 8876461 [details] Bug 1369801 - DevTools as system add-on; https://reviewboard.mozilla.org/r/147802/#review161596 First batch of comments, I obviously need some more time to test all that. ::: browser/installer/package-manifest.in:571 (Diff revision 8) > #endif > > +; [DevTools Shim Files] > +@RESPATH@/browser/chrome/devtools-shim@JAREXT@ > +@RESPATH@/browser/chrome/devtools-shim.manifest > + nit: this didn't changed, instead, here we are just moving it for no good reason. We should move this block back to line 624 to avoid seeing any diff about that. ::: devtools/bootstrap.js:24 (Diff revision 8) > function actionOccurred(id) { > let {require} = Cu.import("resource://devtools/shared/Loader.jsm", {}); > let Telemetry = require("devtools/client/shared/telemetry"); > let telemetry = new Telemetry(); > telemetry.actionOccurred(id); > } We should be dropping that, it was only useful for the development workflow. ::: devtools/bootstrap.js:169 (Diff revision 8) > let window = e.getNext(); > observer(window, "domwindowclosed", null); > } > } > }; > } Same for the key to reload, was related to development workflow. We can remove that helper, its callsites and also the reload method. At the end, bootstrap.js would only register the devtools-startup.js and webide component and delegate to it the call to initDevTools. The pref loading may also be done from devtools-startup.

Comment on attachment 8877539 [details] Bug 1369801 - dt-addon-build: additional build fixes for system addon; https://reviewboard.mozilla.org/r/149002/#review161634 ::: devtools/client/jar.mn:8 (Diff revision 8) > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > [features/devtools@mozilla.org] chrome.jar: > % content devtools %content/ > - content/preferences/debugger.js (preferences/debugger.js) > - content/preferences/devtools.js (preferences/devtools.js) > +* content/preferences/debugger.js (preferences/debugger.js) > +* content/preferences/devtools.js (preferences/devtools.js) With that we can consider that pref files won't contain any preprocessing rules. i.e. they will be processed before being put into the add-on. If that happen to simplify the preferences parsing...

Comment on attachment 8878512 [details] Bug 1369801 - dt-addon: simplify devtools addon bootstrap and extract prefs loading; https://reviewboard.mozilla.org/r/149866/#review161662 Looks good overall, I'm wondering how much bootstrap.js could be just that and delegate everything to devtools-startup.js.

Comment on attachment 8885646 [details] Bug 1369801 - dt-addon-prefs: load DevTools prefs when starting Loader.jsm; https://reviewboard.mozilla.org/r/156152/#review161636 ::: devtools/bootstrap.js:166 (Diff revision 1) > Services.obs.notifyObservers(null, "startupcache-invalidate"); > > unload("reload"); > > // Update the preferences before starting new code > - setPrefs(); > + DevToolsPreferences.loadPrefs(); As said in previous patch, it may be moved to devtools-startup.js ::: devtools/client/preferences/DevToolsPreferences.jsm:49 (Diff revision 1) > + const ifMap = { > + "#if MOZ_UPDATE_CHANNEL == beta": AppConstants.MOZ_UPDATE_CHANNEL === "beta", > + "#if defined(NIGHTLY_BUILD)": AppConstants.NIGHTLY_BUILD, > + "#ifdef MOZ_DEV_EDITION": AppConstants.MOZ_DEV_EDITION, > + "#ifdef RELEASE_OR_BETA": AppConstants.RELEASE_OR_BETA, > + }; We can consider there is no preprocessing condition given the previous patch.

Comment on attachment 8880529 [details] Bug 1369801 - dt-addon-tests: update specificity of selector in firebug-theme.css; https://reviewboard.mozilla.org/r/151860/#review161668 Can we do that now?

Comment on attachment 8876464 [details] Bug 1369801 - dt-addon: use window object in webconsole/net/main to load stylesheets; https://reviewboard.mozilla.org/r/147808/#review161670 We may land that now.