764625 - Web Console and Debugger stay checked in Web Developer menu after closing them with the close X button

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Loic]

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).

Comment 1

[B.J. Herbison]

I see the same behavior on Windows XP and Ubuntu.

Comment 2

[Loic]

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

Comment 3

[Alice0775 White]

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

Comment 4

[Paul Rouget [:paul]]

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

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(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.

Comment 8

[Paul Rouget [:paul]]

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.

Comment 9

[Paul Rouget [:paul]]

Attachment: v1

Comment 10

[Paul Rouget [:paul]]

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

Comment 11

[Paul Rouget [:paul]]

Attachment: v2

Comment 12

[Paul Rouget [:paul]]

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 13

[Paul Rouget [:paul]]

Comment on attachment 633557 [details] [diff] [review]
v2

Little mistake in the patch, re-uploading soon.

Comment 14

[Paul Rouget [:paul]]

Attachment: v2.1

Comment 15

[Mihai Sucan [:msucan]]

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 16

[Panos Astithas (he/him) [:past] (please ni?)]

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)?

Comment 17

[Paul Rouget [:paul]]

(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.

Comment 18

[Paul Rouget [:paul]]

Attachment: v2.2

Addressed Mihai's comments. Added tests.

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 633752 [details] [diff] [review]
v2.2

Looks good to me. Thanks for the update.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

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 22

[Paul Rouget [:paul]]

Attachment: v2.2 - to land

Comment 23

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/f50196512e83

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/f50196512e83

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

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 26

[Alex Keybl [:akeybl]]

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.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e75777b217ea

Comment 28

[Ioana (away)]

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