596249 - Show the Error Console after toggling the pref instead of on new window

Status: ASSIGNED

(Firefox :: Menus, defect)

Description

[Ed Lee :Mardak]

Right now existing windows don't get the menu until restart or a new window is created.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Bug 595096 was marked WONTFIX, and I think this should be too.

Comment 3

[Ed Lee :Mardak]

gavin: Not sure which bug you were referring to (you linked the dupe bug), and I have no strong opinions either way, but beltzner said "We should file a bug on that; it should appear instantly."

Comment 4

[:Gavin Sharp [email: [e-mail]]]

It's the right bug - it was marked WONTFIX before being marked DUPLICATE. Are you going to write a patch?

Comment 5

[Ed Lee :Mardak]

(In reply to comment #4)
> It's the right bug - it was marked WONTFIX before being marked DUPLICATE.
Oh, not sure why dao duped it this way then.

> Are you going to write a patch?
Like I said I have no strong opinions either way. It was requested by beltzner that a bug be filed.

I suppose I could write a patch, but it's not my call if this is a blocker or will get approval for review cycles.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I'll review and approve it.

Comment 7

[Ed Lee :Mardak]

Attachment: v1

I started writing a test, but then got scared by non en-US issues of trying to trigger cmd-shift-j. Should I just check if the menuitem/command are correctly disabled/hidden?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> Should I just check if the menuitem/command are correctly disabled/hidden?

Yes!

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: v1.1

updated this to apply on trunk, and renamed "unloaders" -> "browserShutdownCallbacks".

Comment 10

[Ed Lee :Mardak]

Comment on attachment 478367 [details] [diff] [review]
v1.1

>+  // Add an unloader to remove this pref observer
>+  unloaders.push(function() gPrefService.removeObserver(pref, checkPref));
Don't land quite yet. This unloaders reference needs to be updated. I suppose my test can land separately.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: v1.2

Sigh, forgot to qref...

Comment 12

[Ed Lee :Mardak]

Attachment: for checkin

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 478377 [details] [diff] [review]
for checkin

>diff --git a/browser/base/content/test/browser_toggle_enabled.js b/browser/base/content/test/browser_toggle_enabled.js

>+function _() dump(Array.join(arguments, " ") + "\n");

You shouldn't use dump() in a test - use info() if you really want output in the log, but I think these should just be comments.

Comment 14

[Ed Lee :Mardak]

Attachment: for checkin

Comment 15

[Ed Lee :Mardak]

Seems to be bitrotten by bug 601201 and probably will be by bug 602006.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 478367 [details] [diff] [review]
v1.1

(bitrotten)