Ship DevTools as a system-addon

RESOLVED WONTFIX

Status

enhancement
P3
normal
RESOLVED WONTFIX
2 years ago
11 months ago

People

(Reporter: ochameau, Unassigned)

Tracking

(Blocks 1 bug)

unspecified
Firefox 56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(9 attachments, 19 obsolete attachments)

59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
bgrins
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
ochameau
: review+
Details
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Reporter

Description

2 years ago
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 hidden (mozreview-request)
Reporter

Comment 2

2 years ago
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.
Reporter

Comment 4

2 years ago
(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).
Severity: normal → enhancement
Priority: -- → P3
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
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
Depends on: 1371298
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.
Reporter

Comment 10

2 years ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8876467 - Attachment is obsolete: true
Attachment #8876897 - Attachment is obsolete: true
Attachment #8876462 - Attachment is obsolete: true
Attachment #8877540 - Attachment is obsolete: true
Attachment #8877541 - Attachment is obsolete: true
Attachment #8880530 - Attachment is obsolete: true
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.
Reporter

Comment 113

2 years ago
(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],
Reporter

Comment 114

2 years ago
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.
Reporter

Comment 115

2 years ago
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.
Reporter

Comment 116

2 years 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?!
Reporter

Updated

2 years ago
Attachment #8873899 - Attachment is obsolete: true
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8878513 - Attachment is obsolete: true
Attachment #8878514 - Attachment is obsolete: true
Attachment #8876466 - Attachment is obsolete: true
Attachment #8880528 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Reporter

Comment 172

2 years ago
mozreview-review
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.
Attachment #8885648 - Flags: review?(poirot.alex) → review+
Reporter

Comment 173

2 years ago
mozreview-review
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.
Reporter

Comment 174

2 years ago
mozreview-review
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...
Reporter

Comment 175

2 years ago
mozreview-review
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.
Reporter

Comment 176

2 years ago
mozreview-review
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.
Reporter

Comment 177

2 years ago
mozreview-review
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?
Attachment #8880529 - Flags: review?(poirot.alex) → review+
Reporter

Comment 178

2 years ago
mozreview-review
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.
Reporter

Comment 179

2 years ago
mozreview-review
Comment on attachment 8876465 [details]
Bug 1369801 - dt-addon: set extensions.startupScanScopes in BrowserToolbox profile;

https://reviewboard.mozilla.org/r/147810/#review161654

::: commit-message-47474:8
(Diff revision 10)
> +This is needed to:
> +1. force the devtools system addon installation when starting the BT instance of FF
> +2. avoid the addon update prompt
> +
> +Another approach is to create a separate user.js pref file to fix 1. However with this
> +approach there is currently no way to fix 2.

Setting extensions.lastAppVersion to null/undefined/false in user.js doesn't work when putting it in user.js?
What happens when doing the current patch? Is there a value set prefs.js?

This patch stress me out as it may introduce some unexpected behavior for browser toolbox users, including add-on developers.
Reporter

Comment 180

2 years ago
mozreview-review
Comment on attachment 8876463 [details]
Bug 1369801 - dt-addon-prefs: move jsonview enabled pref outside of devtools addon;

https://reviewboard.mozilla.org/r/147806/#review161656

::: devtools/bootstrap.js:263
(Diff revision 10)
>  
>    startCommandLineHandler(DevToolsStartup);
>  
> +  // Load preferences as early as possible in case another code path tries to initialize
> +  // DevTools related code (e.g. toolbox-process-window.js for the BrowserToolbox).
> +  DevToolsPreferences.loadPrefs();

wouldn't it be fixed if we move the call to loadPrefs into devtools-browser?

::: devtools/bootstrap.js:275
(Diff revision 10)
> -  reload();
> +      reload();
> +    };
> +    Services.obs.addObserver(onBrowserStartup, "browser-delayed-startup-finished");
> +  } else {
> +    reload();
> +  }

I got issues with this code. startingUp doesn't seem to be any related to browser-delayed-startup-finished.
Such code was more reliable:
let window = Services.wm.getMostRecentWindow("navigator:browser");
if (window && window.delayedStartupPromise) {
  await window.delayedStartupPromise;
  // READY
} else {
  Services.obs.addObserver(this, "browser-delayed-startup-finished");
  // READY once event fires
}
Reporter

Comment 181

2 years ago
mozreview-review
Comment on attachment 8885149 [details]
Bug 1369801 - dt-addon: move devtools-startup to devtools shim folder

https://reviewboard.mozilla.org/r/156016/#review161672

I imagine you may land this patch early.
Attachment #8885149 - Flags: review?(poirot.alex) → review+
Reporter

Comment 182

2 years ago
Anything that touches only bootstrap.js would be good candidate to be landed before the actual move.

Comment 183

2 years ago
mozreview-review
Comment on attachment 8880531 [details]
Bug 1369801 - dt-addon-fennec: package specific version of devtools addon for fennec;

https://reviewboard.mozilla.org/r/151864/#review161686

Thanks, this looks good to me. :)
Attachment #8880531 - Flags: review?(jryans) → review+

Comment 184

2 years ago
mozreview-review
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/#review161688

Let's try changing to `window` as discussed on IRC, since that's a bit easier to follow.

I'd still like to know more about what step is creating the sandbox for resource:// loads into the add-on...  Seems like it would have to be some step in the resource protocol handler[1], but I don't see it...  Maybe add-ons set a flag that affects this somehow.

[1]: http://searchfox.org/mozilla-central/source/netwerk/protocol/res/nsResProtocolHandler.h
Attachment #8876464 - Flags: review?(jryans) → review-

Comment 185

2 years ago
mozreview-review
Comment on attachment 8880532 [details]
Bug 1369801 - dt-addon-fennec: skip xpcshell devtools tests on android;

https://reviewboard.mozilla.org/r/151866/#review161692

::: devtools/shared/discovery/tests/unit/xpcshell.ini:5
(Diff revision 6)
>  [DEFAULT]
>  tags = devtools
>  head = ../../../tests/shared-xpcshell-head.js
>  firefox-appdir = browser
> +skip-if = toolkit == 'android' # Bug 1369801 - xpcshell-tests can not run for system addons on android

Can you file a new bug about getting this to work, and then use that bug number here instead?
Attachment #8880532 - Flags: review?(jryans) → review-
(In reply to Alexandre Poirot [:ochameau] from comment #179)
> Comment on attachment 8876465 [details]
> Bug 1369801 - dt-addon: clear extensions.lastAppVersion when creating
> browser toolbox profile;
> 
> https://reviewboard.mozilla.org/r/147810/#review161654
> 
> ::: commit-message-47474:8
> (Diff revision 10)
> > +This is needed to:
> > +1. force the devtools system addon installation when starting the BT instance of FF
> > +2. avoid the addon update prompt
> > +
> > +Another approach is to create a separate user.js pref file to fix 1. However with this
> > +approach there is currently no way to fix 2.
> 
> Setting extensions.lastAppVersion to null/undefined/false in user.js doesn't
> work when putting it in user.js?
> What happens when doing the current patch? Is there a value set prefs.js?
> 
> This patch stress me out as it may introduce some unexpected behavior for
> browser toolbox users, including add-on developers.

I could have added more details in the commit message, but the addonmanager code explicitly handles the case where the pref is not set very differently from all the other use cases. See http://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/toolkit/mozapps/extensions/AddonManager.jsm#794-800 where the appChanged variable is created. It can either be undefined or true or false. It will only be undefined if the pref is not set and reading it throws. 

The issue is that later on the code relies on this "undefined" case to consider that the profile is "new" and that addons should be installed/updated without prompting the user. See http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2012. 

No value that can be put into user.js will result in this variable being undefined without modifying the AddonManager's code. I'm ok with asking to support a new custom value for it that would lead to undefined too. I just thought it would prove challenging to get it reviewed.

This pref is only ever read on the addonmanager startup so I don't think it should have any negative user impact, but I agree it's not clean.

Comment 187

2 years ago
mozreview-review
Comment on attachment 8885647 [details]
Bug 1369801 - dt-addon-xpcshell: move child process memory test into separate test suite;

https://reviewboard.mozilla.org/r/156154/#review162626

::: commit-message-c06e6:1
(Diff revision 1)
> +Bug 1369801 - dt-addon-xpcshell: move child process memory test seperate test suite;r=bgrins

Commit message nit: spelling on "separate" and also, look like it's missing an "into" in between "test" and "separate"

::: devtools/shared/heapsnapshot/tests/unit/ipc_head_heapsnapshot.js:1
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.

Nit: rename this to head_ipc_heapsnapshot.js or head_heapsnapshot_ipc.js so it's next to the other head file

::: devtools/shared/heapsnapshot/tests/unit/xpcshell.ini
(Diff revision 1)
>  [test_ReadHeapSnapshot_with_allocations.js]
>  skip-if = os == 'linux' # Bug 1176173
>  [test_ReadHeapSnapshot_worker.js]
>  skip-if = os == 'linux' # Bug 1176173
>  [test_SaveHeapSnapshot.js]
> -[test_saveHeapSnapshot_e10s_01.js]

So I did a quick test, and I think we can do this in a way that requires less file movement. I think this works:

```
[test_saveHeapSnapshot_e10s_01.js]
head=ipc_head_heapsnapshot.js
```

Then get rid of the new ipc_xpcshell.ini file and revert the moz.build change
Attachment #8885647 - Flags: review?(bgrinstead)

Comment 188

2 years ago
mozreview-review
Comment on attachment 8876896 [details]
Bug 1369801 - dt-addon-xpcshell: load devtools addon for xpcshell tests;

https://reviewboard.mozilla.org/r/148182/#review162634

This seems fine, although I wish we could use absolute paths for the head files to make it easier to move things around, and add new test folders.  For the browser.ini files for shared-head.js we can use `!/devtools/client/framework/test/shared-head.js` - can you check and see if that's possible here?  r=me for the current patch if not.

::: devtools/shared/tests/shared-xpcshell-head.js:16
(Diff revision 9)
> +  const EXTENSION_ID = "devtools@mozilla.org";
> +  let extensionDir = Services.dirsvc.get("GreD", Ci.nsIFile);
> +  extensionDir.append("browser");
> +  extensionDir.append("features");
> +  extensionDir.append(EXTENSION_ID);
> +  // If the unpacked extension doesn't exist, use the packed version.

In what cases will we be using the unpacked vs packed extension?
Attachment #8876896 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #187)
> Comment on attachment 8885647 [details]
> Bug 1369801 - dt-addon-xpcshell: move child process memory test seperate
> test suite;
> 
> https://reviewboard.mozilla.org/r/156154/#review162626
> 
> ::: commit-message-c06e6:1
> (Diff revision 1)
> > +Bug 1369801 - dt-addon-xpcshell: move child process memory test seperate test suite;r=bgrins
> 
> Commit message nit: spelling on "separate" and also, look like it's missing
> an "into" in between "test" and "separate"
> 
> ::: devtools/shared/heapsnapshot/tests/unit/ipc_head_heapsnapshot.js:1
> (Diff revision 1)
> > +/* Any copyright is dedicated to the Public Domain.
> 
> Nit: rename this to head_ipc_heapsnapshot.js or head_heapsnapshot_ipc.js so
> it's next to the other head file
> 
> ::: devtools/shared/heapsnapshot/tests/unit/xpcshell.ini
> (Diff revision 1)
> >  [test_ReadHeapSnapshot_with_allocations.js]
> >  skip-if = os == 'linux' # Bug 1176173
> >  [test_ReadHeapSnapshot_worker.js]
> >  skip-if = os == 'linux' # Bug 1176173
> >  [test_SaveHeapSnapshot.js]
> > -[test_saveHeapSnapshot_e10s_01.js]
> 
> So I did a quick test, and I think we can do this in a way that requires
> less file movement. I think this works:
> 
> ```
> [test_saveHeapSnapshot_e10s_01.js]
> head=ipc_head_heapsnapshot.js
> ```
> 
> Then get rid of the new ipc_xpcshell.ini file and revert the moz.build change

When I made this suggestion I wasn't thinking about the second head.js file that we'll need to include. So if you want to stick with your current approach that's fine - just please rename ipc_xpcshell.ini to xpcshell_ipc.ini (similar to the head file rename suggested above)
Thanks for the reviews so far! Since I asked for a first batch of reviews, we started working on Bug 1359855 which has impacts on this bug so I'm waiting until it lands before resubmitting for reviews. I have new patch series ready which are rebased on top of the current work in Bug 1359855 and that also make sure the DevTools addon is not regressing the performances of Firefox startup.

The main challenge here is around loading preferences early enough so that all code paths that require preferences do not fail, but not too early to avoid impacting FF's startup. For now the only compromise I found was to load the preferences from Loader.jsm, as it is really always loaded first when DevTools are used.

I tried using client/framework/devtools.js earlier but that could not fly for builds packaging only the server part of DevTools

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ced7bf80ed209691e3533dfa7e045fa45957067
Try build on all platforms with all tests that seems mostly green so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba221fba037f914b4c742c5eb6a79d20e43b1067

Waiting for Bug 1359855 and Bug 1374735 before landing.
Depends on: 1359855, 1374735
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8885648 - Attachment is obsolete: true

Comment 205

2 years ago
mozreview-review
Comment on attachment 8880532 [details]
Bug 1369801 - dt-addon-fennec: skip xpcshell devtools tests on android;

https://reviewboard.mozilla.org/r/151866/#review165902

::: devtools/shared/discovery/tests/unit/xpcshell.ini:5
(Diff revision 7)
>  [DEFAULT]
>  tags = devtools
>  head = ../../../tests/shared-xpcshell-head.js
>  firefox-appdir = browser
> +skip-if = toolkit == 'android' # Bug 1380688 - xpcshell-tests can not run for system addons on android

That's the duplicate bug number, please replace these with 1380687 instead.
Attachment #8880532 - Flags: review?(jryans) → review+

Comment 206

2 years ago
mozreview-review
Comment on attachment 8885647 [details]
Bug 1369801 - dt-addon-xpcshell: move child process memory test into separate test suite;

https://reviewboard.mozilla.org/r/156154/#review165906
Attachment #8885647 - Flags: review?(bgrinstead) → review+

Comment 207

2 years ago
mozreview-review
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/#review165904

::: commit-message-66f5c:11
(Diff revision 11)
> +
> +Use the window object which points to the actual Window instead.
> +
> +MozReview-Commit-ID: LxDNfDiOso3
> +***
> +yolo

Probably should clean up this message...? :P
Attachment #8876464 - Flags: review?(jryans) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #207)
> 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/#review165904
> 
> ::: commit-message-66f5c:11
> (Diff revision 11)
> > +
> > +Use the window object which points to the actual Window instead.
> > +
> > +MozReview-Commit-ID: LxDNfDiOso3
> > +***
> > +yolo
> 
> Probably should clean up this message...? :P

Wow, thanks for spotting that :)
Reporter

Comment 222

2 years ago
mozreview-review
Comment on attachment 8877539 [details]
Bug 1369801 - dt-addon-build: additional build fixes for system addon;

https://reviewboard.mozilla.org/r/149002/#review166180

LGTM
Attachment #8877539 - Flags: review?(poirot.alex) → review+
Reporter

Comment 223

2 years ago
mozreview-review
Comment on attachment 8876463 [details]
Bug 1369801 - dt-addon-prefs: move jsonview enabled pref outside of devtools addon;

https://reviewboard.mozilla.org/r/147806/#review166184

::: .eslintignore:141
(Diff revision 12)
>  devtools/shared/platform/content/test/test_clipboard.html
>  devtools/shared/qrcode/tests/mochitest/test_decode.html
>  devtools/shared/tests/mochitest/*.html
>  devtools/shared/webconsole/test/test_*.html
>  
>  # Ignore devtools pre-processed files

s/pre-processed/preferences/

::: devtools/shim/devtools-startup-prefs.js:4
(Diff revision 12)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

It would be great to have a comment explaining the purpose of this particular preference file
Attachment #8876463 - Flags: review?(poirot.alex) → review+
Reporter

Comment 224

2 years ago
mozreview-review
Comment on attachment 8876465 [details]
Bug 1369801 - dt-addon: set extensions.startupScanScopes in BrowserToolbox profile;

https://reviewboard.mozilla.org/r/147810/#review166188

Looks good given that you verified it doesn't trigger any listener while firefox is runnin.
Attachment #8876465 - Flags: review?(poirot.alex) → review+
Reporter

Comment 225

2 years ago
mozreview-review
Comment on attachment 8878512 [details]
Bug 1369801 - dt-addon: simplify devtools addon bootstrap and extract prefs loading;

https://reviewboard.mozilla.org/r/149866/#review166196

Looks correct, but what about following up on that?

Like: getting rid of preprocessing, converting to an easier to parse format (json or pure js with `let pref = require("devtools/pref...");`)
Anything but crazy regexp to support very old text format!

::: devtools/client/preferences/DevToolsPreferences.jsm:42
(Diff revision 9)
> +function cleanupPreferencesFileContent(content) {
> +  let lines = content.split("\n");
> +  let newLines = [];
> +  let continuation = false;
> +  for (let line of lines) {
> +    let isPrefLine = /^ *(sticky_)?pref\("([^"]+)"/.test(line);

Could you check that we are not missing something for devtools.theme:
http://searchfox.org/mozilla-central/source/devtools/client/preferences/devtools.js#233
It is far from being clear what sticky_pref really mean. :bgrins may know.

I tracked it down to that:
http://searchfox.org/mozilla-central/source/modules/libpref/prefapi.cpp#791
http://searchfox.org/mozilla-central/source/modules/libpref/prefapi.cpp#801-803
But I'm afraid we just can't set that flag from JS.
Attachment #8878512 - Flags: review?(poirot.alex) → review+
Reporter

Comment 226

2 years ago
mozreview-review
Comment on attachment 8885646 [details]
Bug 1369801 - dt-addon-prefs: load DevTools prefs when starting Loader.jsm;

https://reviewboard.mozilla.org/r/156152/#review166206

::: devtools/bootstrap.js:60
(Diff revision 3)
> +    // Load DevToolsPreferences as soon as DevTools code starts being required.
> +    // With DevTools as an addon, the DevTools preferences need to be dynamically loaded.
> +    const {DevToolsPreferences} =
> +      Cu.import("chrome://devtools/content/preferences/DevToolsPreferences.jsm", {});
> -  DevToolsPreferences.loadPrefs();
> +    DevToolsPreferences.loadPrefs();
> +  }, "devtools-loader-starting");

I would have inlined that into Loader.jsm to avoid putting startup codepath in many places.
You can check for processes with:
const isParentProcess = Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_DEFAULT;
And firefox with:
const isFirefox = Services.appinfo.ID == "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}";
Or Fennec:
const isFennec = Services.appinfo.ID == "{aa3c5121-dab2-40e2-81ca-7ea25febc110}"

But that's not a big deal. We could also followup by reviewing our whole startup codepath in order to clarify/simplify it, if possible.
Attachment #8885646 - Flags: review?(poirot.alex) → review+

Comment 227

2 years ago
mozreview-review-reply
Comment on attachment 8878512 [details]
Bug 1369801 - dt-addon: simplify devtools addon bootstrap and extract prefs loading;

https://reviewboard.mozilla.org/r/149866/#review166196

> Could you check that we are not missing something for devtools.theme:
> http://searchfox.org/mozilla-central/source/devtools/client/preferences/devtools.js#233
> It is far from being clear what sticky_pref really mean. :bgrins may know.
> 
> I tracked it down to that:
> http://searchfox.org/mozilla-central/source/modules/libpref/prefapi.cpp#791
> http://searchfox.org/mozilla-central/source/modules/libpref/prefapi.cpp#801-803
> But I'm afraid we just can't set that flag from JS.

Good point we might have to leave this one out of the addon. (for the record there is some documentation on MDN about sticky_pref: https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/A_brief_guide_to_Mozilla_preferences#Sticky_Preferences)
Brian: can you tell us if the sticky_pref behavior is still required for devtools.theme. If yes, do you know if there is any way to emulate it in JS? Otherwise we will have to move this pref to the file that we will remain outside of the addon.
Flags: needinfo?(bgrinstead)

Comment 229

2 years ago
mozreview-review-reply
Comment on attachment 8885646 [details]
Bug 1369801 - dt-addon-prefs: load DevTools prefs when starting Loader.jsm;

https://reviewboard.mozilla.org/r/156152/#review166206

> I would have inlined that into Loader.jsm to avoid putting startup codepath in many places.
> You can check for processes with:
> const isParentProcess = Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_DEFAULT;
> And firefox with:
> const isFirefox = Services.appinfo.ID == "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}";
> Or Fennec:
> const isFennec = Services.appinfo.ID == "{aa3c5121-dab2-40e2-81ca-7ea25febc110}"
> 
> But that's not a big deal. We could also followup by reviewing our whole startup codepath in order to clarify/simplify it, if possible.

I initially had everything inlined in Loader.jsm, with a check to avoid loading prefs in the content process, I was just missing the part for identifying Fennec. I'll try your approach before landing.
(In reply to Julian Descottes [:jdescottes] from comment #228)
> Brian: can you tell us if the sticky_pref behavior is still required for
> devtools.theme.

For context: Sticky prefs were added in Bug 1098343 as a way to allow different default values on different channels, and not have the profile reset the value when switching between them.  It's well explained here: https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/A_brief_guide_to_Mozilla_preferences#Sticky_Preferences.

As for whether it's still required, that's a product question. As long as we still want the Dev Edition devtools theme (and compact browser theme) to be dark by default and other channels devtools theme to be light then I'd say yes. Also note that the theme is synced to the browser compact theme color, if a compact theme is applied: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#175.

> If yes, do you know if there is any way to emulate it in JS?
> Otherwise we will have to move this pref to the file that we will remain
> outside of the addon.

I assume by 'emulate it in JS' you mean some variation of `let defaultBranch = Services.prefs.getDefaultBranch(""); defaultBranch.setCharPref("devtools.theme", "dark");` that means the pref is sticky. I'm not aware of a way to do that, but forwarding that question to Mark.
Flags: needinfo?(bgrinstead) → needinfo?(markh)
(In reply to Julian Descottes [:jdescottes] from comment #228)
> If yes, do you know if there is any way to emulate it in JS?
> Otherwise we will have to move this pref to the file that we will remain
> outside of the addon.

Just in case it's not available from JS: what would the downsides be to moving that pref into an outside file?

Comment 232

2 years ago
mozreview-review-reply
Comment on attachment 8876463 [details]
Bug 1369801 - dt-addon-prefs: move jsonview enabled pref outside of devtools addon;

https://reviewboard.mozilla.org/r/147806/#review166184

> s/pre-processed/preferences/

updated.

> It would be great to have a comment explaining the purpose of this particular preference file

Added a comment.
(In reply to Brian Grinstead [:bgrins] from comment #231)
> (In reply to Julian Descottes [:jdescottes] from comment #228)
> > If yes, do you know if there is any way to emulate it in JS?
> > Otherwise we will have to move this pref to the file that we will remain
> > outside of the addon.
> 
> Just in case it's not available from JS: what would the downsides be to
> moving that pref into an outside file?

Not many downsides. We are already introducing a "classic" preferences file for a jsonview related pref in https://reviewboard.mozilla.org/r/147806 (it needs to be available too early). So we would add the devtools.theme sticky pref to this file, it would simply follow Firefox release cycle instead of following the addon's release cycle.

Comment 234

2 years ago
mozreview-review-reply
Comment on attachment 8876896 [details]
Bug 1369801 - dt-addon-xpcshell: load devtools addon for xpcshell tests;

https://reviewboard.mozilla.org/r/148182/#review162634

> In what cases will we be using the unpacked vs packed extension?

Added a comment in the head to clarify this.
(In reply to Julian Descottes [:jdescottes] from comment #233)
> (In reply to Brian Grinstead [:bgrins] from comment #231)
> > (In reply to Julian Descottes [:jdescottes] from comment #228)
> > > If yes, do you know if there is any way to emulate it in JS?
> > > Otherwise we will have to move this pref to the file that we will remain
> > > outside of the addon.
> > 
> > Just in case it's not available from JS: what would the downsides be to
> > moving that pref into an outside file?
> 
> Not many downsides. We are already introducing a "classic" preferences file
> for a jsonview related pref in https://reviewboard.mozilla.org/r/147806 (it
> needs to be available too early). So we would add the devtools.theme sticky
> pref to this file, it would simply follow Firefox release cycle instead of
> following the addon's release cycle.

OK, that seems like a good solution for this pref. I'd probably just do that if there's not a trivial way to register it from JS.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 250

2 years ago
mozreview-review
Comment on attachment 8876461 [details]
Bug 1369801 - DevTools as system add-on;

https://reviewboard.mozilla.org/r/147802/#review166288
Attachment #8876461 - Flags: review?(jdescottes) → review+

Comment 251

2 years ago
mozreview-review-reply
Comment on attachment 8878512 [details]
Bug 1369801 - dt-addon: simplify devtools addon bootstrap and extract prefs loading;

https://reviewboard.mozilla.org/r/149866/#review166196

> Good point we might have to leave this one out of the addon. (for the record there is some documentation on MDN about sticky_pref: https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/A_brief_guide_to_Mozilla_preferences#Sticky_Preferences)

Moved this pref to the devtools-startup-prefs.js file in a separate commit: https://reviewboard.mozilla.org/r/160982/diff/1#index_header
Comment hidden (mozreview-request)
Still had a few failures related to the latest changes in Bug 1359855.
New try rebased on recent mozilla-central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05dd8ff03c6816a3e0db1c8a1b3ac0ed94628825
(In reply to Brian Grinstead [:bgrins] from comment #230)
> > If yes, do you know if there is any way to emulate it in JS?
> > Otherwise we will have to move this pref to the file that we will remain
> > outside of the addon.
> 
> I assume by 'emulate it in JS' you mean some variation of `let defaultBranch
> = Services.prefs.getDefaultBranch("");
> defaultBranch.setCharPref("devtools.theme", "dark");` that means the pref is
> sticky. I'm not aware of a way to do that, but forwarding that question to
> Mark.

There's no way to create a sticky pref from JS - I see no reason it couldn't be added other than complexity (eg, it would only make sense on the default branch, so it would mean the API diverges) - FWIW, bug 1098343 is where these "sticky" prefs landed. However, it seems you are fine with using another .js file for this which should solve your problem.
Flags: needinfo?(markh)

Comment 256

2 years ago
mozreview-review
Comment on attachment 8889922 [details]
Bug 1369801 - dt-addon-prefs: move devtools.theme preference to devtools-startup-prefs.js;

https://reviewboard.mozilla.org/r/160982/#review166576

::: devtools/shim/moz.build:9
(Diff revision 1)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  JAR_MANIFESTS += ['jar.mn']
>  
> -JS_PREFERENCE_FILES += [
> +JS_PREFERENCE_PP_FILES += [

What is the purpose of this change?
Attachment #8889922 - Flags: review?(bgrinstead) → review+

Comment 257

2 years ago
mozreview-review-reply
Comment on attachment 8889922 [details]
Bug 1369801 - dt-addon-prefs: move devtools.theme preference to devtools-startup-prefs.js;

https://reviewboard.mozilla.org/r/160982/#review166576

Thanks for the review!

> What is the purpose of this change?

It enables preprocessing for this preference file.
devtools.theme is wrapped in an "#if/#else" preprocessing block.
Reporter

Comment 258

2 years ago
mozreview-review
Comment on attachment 8889982 [details]
Bug 1369801 - Fix devtools shim test now that devtools resource path is not registered;

https://reviewboard.mozilla.org/r/161028/#review166706

::: devtools/shim/tests/unit/test_devtools_shim.js:266
(Diff revision 1)
>    ok(DevToolsShim.isInitialized(), "DevTools are initialized");
>  
>    DevToolsShim.getOpenedScratchpads();
>    checkCalls(mock, "getOpenedScratchpads", 1, []);
> +
> +  restoreDevToolsInstalled();

You don't call mockDevToolsInstalled in this test, so you should not call this restore method here.
Attachment #8889982 - Flags: review?(poirot.alex) → review+
Reporter

Comment 259

2 years ago
mozreview-review
Comment on attachment 8890051 [details]
Bug 1369801 - move devtools.inspector.enabled to devtools-startup-prefs;

https://reviewboard.mozilla.org/r/161106/#review166708

That's fine to polish that in a followup.

I imagine you had issue with this test?
http://searchfox.org/mozilla-central/source/browser/base/content/test/general/contextmenu_common.js#318
The user codepath looks good:
http://searchfox.org/mozilla-central/source/browser/base/content/nsContextMenu.js#317
as it is being guarded by DevToolsShim.isInstalled()
Attachment #8890051 - Flags: review?(poirot.alex) → review+

Comment 260

2 years ago
mozreview-review-reply
Comment on attachment 8889982 [details]
Bug 1369801 - Fix devtools shim test now that devtools resource path is not registered;

https://reviewboard.mozilla.org/r/161028/#review166706

> You don't call mockDevToolsInstalled in this test, so you should not call this restore method here.

It is called at line 245.

Comment 261

2 years ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a8f5e718ddd
DevTools as system add-on;r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/e2100bb65c97
dt-addon-build: additional build fixes for system addon;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/1b1a7c913f13
dt-addon: simplify devtools addon bootstrap and extract prefs loading;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/ad2a532a50f0
dt-addon-prefs: load DevTools prefs when starting Loader.jsm;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d0ed0997f4e0
dt-addon-prefs: move jsonview enabled pref outside of devtools addon;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a31f0c8450a8
dt-addon-prefs: move devtools.theme preference to devtools-startup-prefs.js;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/192d517219c1
dt-addon-xpcshell: load devtools addon for xpcshell tests;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/98e6c8bf442e
dt-addon-xpcshell: move child process memory test into separate test suite;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/898739a110fc
dt-addon-tests: update specificity of selector in firebug-theme.css;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/148d11a549ce
dt-addon-fennec: package specific version of devtools addon for fennec;r=jryans
https://hg.mozilla.org/integration/autoland/rev/435291ffe259
dt-addon-fennec: skip xpcshell devtools tests on android;r=jryans
https://hg.mozilla.org/integration/autoland/rev/3ab3a5481b90
dt-addon: use window object in webconsole/net/main to load stylesheets;r=jryans
https://hg.mozilla.org/integration/autoland/rev/0462e7a66185
dt-addon: clear extensions.lastAppVersion when creating browser toolbox profile;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/082ece5eba4d
dt-addon: move devtools-startup to devtools shim folder r=ochameau
https://hg.mozilla.org/integration/autoland/rev/71b891583296
Fix devtools shim test now that devtools resource path is not registered;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/ad2610a5e6ba
move devtools.inspector.enabled to devtools-startup-prefs;r=ochameau
Reporter

Comment 263

2 years ago
We get many reports of DevTools key shortcuts being missing and menus being empty.

I'm not able to reproduce it in any way.
But I do see this exception on Windows when using the context menu:

  TypeError: this.gDevTools.inspectNode is not a function    DevToolsShim.jsm:233:12

Given the line number it includes this patch.
It may related to bug 1359855, but we only got failure report in the last two hours.
So it may only be this bug's patches.

I think we should back this out and try to spawn another nightly build.
Reporter

Comment 264

2 years ago
Another strange thing is that: chrome://devtools/locale/key-shortcuts.properties
can't be loaded on Windows, but it key shortcuts still work.
Reporter

Updated

2 years ago
Blocks: 1384967