Open Bug 596249 Opened 9 years ago Updated 7 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

ASSIGNED

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file, 4 obsolete files)

Right now existing windows don't get the menu until restart or a new window is created.
Duplicate of this bug: 595096
Bug 595096 was marked WONTFIX, and I think this should be too.
Blocks: 596361
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."
It's the right bug - it was marked WONTFIX before being marked DUPLICATE. Are you going to write a patch?
(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.
I'll review and approve it.
Attached patch v1 (obsolete) — Splinter Review
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?
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #475457 - Flags: review?(gavin.sharp)
(In reply to comment #7)
> Should I just check if the menuitem/command are correctly disabled/hidden?

Yes!
Attached patch v1.1 (obsolete) — Splinter Review
updated this to apply on trunk, and renamed "unloaders" -> "browserShutdownCallbacks".
Attachment #475457 - Attachment is obsolete: true
Attachment #478367 - Flags: review+
Attachment #475457 - Flags: review?(gavin.sharp)
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.
Attached patch v1.2 (obsolete) — Splinter Review
Sigh, forgot to qref...
Attachment #478367 - Attachment is obsolete: true
Attachment #478375 - Flags: review+
Attachment #478375 - Flags: approval2.0+
Attached patch for checkin (obsolete) — Splinter Review
Attachment #478375 - Attachment is obsolete: true
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.
Attached patch for checkinSplinter Review
Attachment #478377 - Attachment is obsolete: true
Seems to be bitrotten by bug 601201 and probably will be by bug 602006.
Depends on: 601201, 602006
You need to log in before you can comment on or make changes to this bug.