CustomizableUI.registerWindow() isn't always called

RESOLVED FIXED in Firefox 28



6 years ago
5 years ago


(Reporter: Unfocused, Assigned: mconley)


Firefox 28
Windows 8

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

Seems this was added in the initial review, and I missed it... CustomizeMode is calling CustomizableUI.registerWindow(), therefore it only gets called on windows that enter the customization mode. Instead, it needs to be called for every window that has a toolbar/panel that gets built by CustomizableUI.

registerWindow() really just exists to ensure unregisterBuildWindow() gets called when the window closes - that cleans up the internal state of CustomizableUI that keeps track of build areas and widget instances. This was why the original code was hooking that up automatically whenever a build area (toolbar/panel) instance was registered - its cleaning up after those disappear (it's not so much about the window, but whats in the window). I think the intent was to decouple that, but I'm not convinced it should be decoupled.


6 years ago
Blocks: 770135

Comment 1

6 years ago
Created attachment 735340 [details] [diff] [review]
Patch v1

Ah, yep - I see now. This patch moves the cleanup event handler registration back to when we register the toolbar.
Assignee: nobody → mconley
Attachment #735340 - Flags: review?(bmcbride)
Comment on attachment 735340 [details] [diff] [review]
Patch v1

Review of attachment 735340 [details] [diff] [review]:

Should also:
* remove the public API for registerWindow (don't think it will cause harm by someone calling it, but it's still not an API that should be called by outside code)
* rename registerWindow to registerBuildWindow, so it matches unregisterBuildWindow
* move registerBuildWindow so it's right before unregisterBuildWindow

r+ with those tweaks.
Attachment #735340 - Flags: review?(bmcbride) → review+

Comment 3

6 years ago
Created attachment 735507 [details] [diff] [review]
Patch v1.1 (r+'d by Unfocused)

Thanks Blair!
Attachment #735340 - Attachment is obsolete: true
Attachment #735507 - Flags: review+

Comment 4

6 years ago
Landed on jamun:
Whiteboard: [fixed in jamun]

Comment 5

6 years ago
Landed on UX:
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed in ux]

Comment 6

5 years ago
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in jamun][fixed in ux]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.