User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 Build ID: 20120613030535 Steps to reproduce: 1) Open both Web Console and Debugger 2) Close both Web Console and Debugger with the close X button 3) Go to the Web Developer menu Actual results: Web Console and Debugger stay checked in the Web Developer menu (check mark still visible).

I see the same behavior on Windows XP and Ubuntu.

Mozregression range: m-c: good: 2012-05-11 bad: 2012-05-12 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e6a9572b48f7&tochange=22a58090fa70 m-i: good: 2012-05-13 bad: 2012-05-14 http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e0f7b9a39d7&tochange=3eab6ea25342 I'd say the culprit is: Paul Rouget — Bug 749626 - Theme update: make the close button code generic and implement the dark theme for menulists. r=dao

Last Good:f80568dba010 First Bad:2e5b9ba8358b Triggered by:2e5b9ba8358b Joe Walker — Bug 720641 - Integrate GCLI into developer tools global toolbar; r=dcamp,dão,robcee,zpao

Known for the Web Console: bug 759384 And I'm pretty sure we also have a bug for the debugger.

(In reply to Paul Rouget [:paul] from comment #4) > Known for the Web Console: bug 759384 > And I'm pretty sure we also have a bug for the debugger. Bug 760771.

For the web console, we should use setAttribute(checked, false) on the command (not remove attribute). We want the attribute value to propagate. For the debugger, we will have the same problem. But we don't even update the value on tab change. I'll look at this today.

This only works if the toolbar is active. It needs more work.

Comment on attachment 633557 [details] [diff] [review] v2 Can you guys take a look at this patch and tell me if there's a better play to register these listeners (debugger & webconsole)? Thanks.

Comment on attachment 633557 [details] [diff] [review] v2 Little mistake in the patch, re-uploading soon.

Comment on attachment 633567 [details] [diff] [review] v2.1 Paul: this looks good to me, but I'd like a small change such that it fits with the existing onWindowUnload. Also please add the TabSelect event listener alongside the existing TabClose event listener. What happens when the second window is open? Are the event listeners added for the second window as well? Thanks!

Comment on attachment 633567 [details] [diff] [review] v2.1 Review of attachment 633567 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK, DebuggerUI is the right place to handle global listeners. Have you verified it works OK when multiple windows are open (with multiple tabs)?

(In reply to Panos Astithas [:past] from comment #16) > Looks OK, DebuggerUI is the right place to handle global listeners. Have you > verified it works OK when multiple windows are open (with multiple tabs)? I did.

Addressed Mihai's comments. Added tests.

Comment on attachment 633752 [details] [diff] [review] v2.2 Looks good to me. Thanks for the update.

Comment on attachment 633752 [details] [diff] [review] v2.2 Review of attachment 633752 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/browser_dbg_menustatus.js @@ +3,5 @@ > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + */ > + > +function test() { Nit: could you add a one-line comment above that explains the purpose of the test? @@ +28,5 @@ > + is(cmd.getAttribute("checked"), "true", "<command Tools:Debugger> is checked."); > + > + let pane = DebuggerUI.toggleDebugger(); > + > + is(cmd.getAttribute("checked"), "false", "<command Tools:Debugger> is checked once closed."); s/checked/unchecked/

Comment on attachment 634478 [details] [diff] [review] v2.2 - to land [Approval Request Comment] Bug caused by (feature/regressing bug #): The debugger is a new feature, but the console seems to have regressed with bug 749626 User impact if declined: inconsistent state of the web console and debugger check marks in the menu Testing completed (on m-c, etc.): On m-c and fx-team Risk to taking this patch (and alternatives if risky): fairly minimal patch, in two predominantly developer-only features String or UUID changes made by this patch: none

Comment on attachment 634478 [details] [diff] [review] v2.2 - to land [Triage Comment] Very limited scope, and we've got QA/engineer/external testing specifically around the new dev tools changes. I expect we'll find any regressions quickly.

Verified as fixed on: Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20100101 Firefox/15.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/20100101 Firefox/15.0 Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0