Last Comment Bug 917706 - Browser Debugger shouldn't require a restart / new window
: Browser Debugger shouldn't require a restart / new window
Status: RESOLVED FIXED
[chrome-debug]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 27
Assigned To: :Gijs
:
: Jason Laster [:jlast]
Mentors:
Depends on:
Blocks: 918240
  Show dependency treegraph
 
Reported: 2013-09-18 01:55 PDT by :Gijs
Modified: 2013-09-19 08:01 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
menu items and shortcuts should start working as soon as the relevant pref(s) are flipped, (10.24 KB, patch)
2013-09-18 03:51 PDT, :Gijs
no flags Details | Diff | Splinter Review
menu items and shortcuts should start working as soon as the relevant pref(s) are flipped, (10.30 KB, patch)
2013-09-18 10:18 PDT, :Gijs
rcampbell: review+
Details | Diff | Splinter Review
add tests for updating disabled/hidden values dynamically, (2.99 KB, patch)
2013-09-18 10:53 PDT, :Gijs
no flags Details | Diff | Splinter Review
add tests for updating disabled/hidden values dynamically, (2.88 KB, patch)
2013-09-18 11:20 PDT, :Gijs
no flags Details | Diff | Splinter Review
menu items and shortcuts should start working as soon as the relevant pref(s) are flipped, (13.97 KB, patch)
2013-09-18 11:21 PDT, :Gijs
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review

Description User image :Gijs 2013-09-18 01:55:34 PDT
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 User image :Gijs 2013-09-18 03:51:29 PDT
Created attachment 806562 [details] [diff] [review]
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 User image :Gijs 2013-09-18 03:53:27 PDT
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 User image Rob Campbell [:rc] (:robcee) 2013-09-18 06:38:19 PDT
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 User image :Gijs 2013-09-18 10:18:07 PDT
Created attachment 806754 [details] [diff] [review]
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 User image :Gijs 2013-09-18 10:53:17 PDT
Created attachment 806777 [details] [diff] [review]
add tests for updating disabled/hidden values dynamically,

Some simple tests.
Comment 6 User image Rob Campbell [:rc] (:robcee) 2013-09-18 11:05:01 PDT
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 User image :Gijs 2013-09-18 11:20:18 PDT
Created attachment 806796 [details] [diff] [review]
add tests for updating disabled/hidden values dynamically,

Test without those two prefs.
Comment 8 User image :Gijs 2013-09-18 11:21:09 PDT
Created attachment 806797 [details] [diff] [review]
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 User image :Gijs 2013-09-18 11:23:16 PDT
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 User image Rob Campbell [:rc] (:robcee) 2013-09-18 12:37:00 PDT
sweet! thanks. :)
Comment 11 User image :Gijs 2013-09-19 01:20:32 PDT
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

Note You need to log in before you can comment on or make changes to this bug.