917706 - Browser Debugger shouldn't require a restart / new window

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[:Gijs (he/him)]

STR:

1. Open the Toolbox
2. Enable Chrome Debugging and Remote Debugging
3. Open the Developer Tools menu
4. Become very sad because the items you want aren't in there.

Opening a new window actually shows these items, so we could just add a pref observer that toggles these items instead, and save us some cumbersome in cases like bug 895471 comment 2

Comment 1

[:Gijs (he/him)]

Attachment: menu items and shortcuts should start working as soon as the relevant pref(s) are flipped,

This patch works, but it breaks mochitest-browser for me locally. It hangs waiting to start browser_cmd_settings.js, but only if browser_cmd_screenshot.js has run before it. I have poked at this for about an hour now, but I don't understand why this patch would affect that. I've tried sticking all the code I added in try catch blocks with Cu.reportErrors, and no errors are reported, so I'm a bit lost as to what the issue is.

Comment 2

[:Gijs (he/him)]

Comment on attachment 806562 [details] [diff] [review]
menu items and shortcuts should start working as soon as the relevant pref(s) are flipped,

Rob, any idea why this would break tests as described?

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 806562 [details] [diff] [review]
menu items and shortcuts should start working as soon as the relevant pref(s) are flipped,

Review of attachment 806562 [details] [diff] [review]:
-----------------------------------------------------------------

That test() function in browser_cmd_screenshot.js is doing some funny things with promises.

function test() {
  info("RUN TEST: non-private window");
  let nonPrivDone = addWindow({ private: false }, addTabWithToolbarRunTests);

  let privDone = nonPrivDone.then(function() {
    info("RUN TEST: private window");
    return addWindow({ private: true }, addTabWithToolbarRunTests);
  });

  privDone.then(finish, function(error) {
    ok(false, 'Promise fail: ' + error);
  });
}

nonPrivDone is a deferred promise.

privDone calls nonPrivDone.then() but doesn't add an error function.

You should add

, function(error) {
    ok(false, 'Promise fail: ' + error);
  });

to the end of nonPrivDone.then(... and see what happens. I bet it's failing silently and helpfully eating the error message.

Comment 4

[:Gijs (he/him)]

Attachment: menu items and shortcuts should start working as soon as the relevant pref(s) are flipped,

OK, this works. It seems that DeveloperToolbar.show updates prefs and changes focus, which caused this code to be called multiple times when the test ended, which must have confused the focus checking in mochitest. Simple solution: don't run this code for *every* pref, just for the 'enabled' ones. I'll try to add a test for this functionality in a separate patch. I also believe that with this change, the 'restart now?' butto... err, links, in the settings panel, can disappear. Is that correct?

Comment 5

[:Gijs (he/him)]

Attachment: add tests for updating disabled/hidden values dynamically,

Some simple tests.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 806754 [details] [diff] [review]
menu items and shortcuts should start working as soon as the relevant pref(s) are flipped,

Review of attachment 806754 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with this with a successful try run and those extraneous prefs removed.

::: browser/devtools/framework/gDevTools.jsm
@@ +367,5 @@
> +
> +    // Enable Chrome Debugger?
> +    let chromeEnabled = Services.prefs.getBoolPref("devtools.chrome.enabled");
> +    let devtoolsRemoteEnabled = Services.prefs.getBoolPref("devtools.debugger.remote-enabled");
> +    let remoteEnabled = chromeEnabled && devtoolsRemoteEnabled && 

nit: trailing space.

@@ +377,5 @@
> +    toggleCmd("Tools:ErrorConsole", consoleEnabled);
> +
> +    // Enable Scratchpad in the UI, if the preference allows this.
> +    let scratchpadEnabled = Services.prefs.getBoolPref("devtools.scratchpad.enabled");
> +    toggleCmd("Tools:Scratchpad", scratchpadEnabled);

I think we can probably get rid of this pref altogether.

@@ +384,5 @@
> +    toggleCmd("Tools:DevToolsConnect", devtoolsRemoteEnabled);
> +
> +    // Enable Responsive UI?
> +    let responsiveUIEnabled = Services.prefs.getBoolPref("devtools.responsiveUI.enabled");
> +    toggleCmd("Tools:ResponsiveUI", responsiveUIEnabled);

and this one.

Comment 7

[:Gijs (he/him)]

Attachment: add tests for updating disabled/hidden values dynamically,

Test without those two prefs.

Comment 8

[:Gijs (he/him)]

Attachment: menu items and shortcuts should start working as soon as the relevant pref(s) are flipped,

Patch with those prefs removed. Try run with this patch + the new test: https://tbpl.mozilla.org/?tree=Try&rev=95f279a65f85

Comment 9

[:Gijs (he/him)]

Comment on attachment 806797 [details] [diff] [review]
menu items and shortcuts should start working as soon as the relevant pref(s) are flipped,

Carrying over r+. Will look at try tomorrow morning and land this assuming that's green and the tests look OK, too. :-)

Comment 10

[Rob Campbell [:rc] (:robcee)]

sweet! thanks. :)

Comment 11

[:Gijs (he/him)]

Greenest try run I've seen in a while. Landed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/81eb0c8d020a
remote:   https://hg.mozilla.org/integration/fx-team/rev/344353b9772b

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/81eb0c8d020a
https://hg.mozilla.org/mozilla-central/rev/344353b9772b