Make the toolboxChanged() callback return useful information.

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Toolbars and Toolbar Customization
--
enhancement
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

(Blocks: 1 bug, {fixed1.9.1})

Trunk
mozilla1.9.2a1
fixed1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 354796 [details] [diff] [review]
Patch v1.0 enhanced toolboxChanged()

SeaMonkey customizable toolbars have attribute settings in additional to those supported by toolkit (e.g. "labelalign" and "moz-collapsed"). We need to reset these to default when the "reset to defaults" button in toolkit customize toolbar window is called.

This patch makes the toolboxChanged() callback return useful additional information that we and other applications can use:

function toolboxChanged(event, cargo)

event: is a string e.g. "reset" "itemadded" "itemmoved" "itemremoved" "newtoolbar"
cargo: the item associated with the event e.g. a toolbarbutton, or a new toolbar. Can be null.
Attachment #354796 - Flags: review?(gavin.sharp)
(In reply to comment #0)
> We need to reset these to default when the "reset to defaults" button in
> toolkit customize toolbar window is called.

Do you have a use planned for the other notifications added in the patch?
(Assignee)

Comment 2

9 years ago
> Do you have a use planned for the other notifications added in the patch?

No, but I think if the callback returns information, it should always return consistent information. Do you think that this is over-engineering? If so I'll file an updated patch with only the reset notification returned.

I've also CCed philor to see if Thunderbird would like to use these callback notifications.

And there are certainly extensions that would like to be notified if their toolbarbuttons are inserted into a toolbar. Currently they just listen for the DOMNodeInserted event.
(Assignee)

Updated

9 years ago
Blocks: 471372
(Assignee)

Comment 3

9 years ago
Created attachment 356515 [details] [diff] [review]
Patch v2.0 (Minimal Fix)

As discussed over IRC with gavin, this is a mimimal patch. Only restoreDefaultSet() passes a parameter "reset" to toolboxChanged() and thence to the callback.
Attachment #354796 - Attachment is obsolete: true
Attachment #356515 - Flags: review?(gavin.sharp)
Attachment #354796 - Flags: review?(gavin.sharp)
Comment on attachment 356515 [details] [diff] [review]
Patch v2.0 (Minimal Fix)

I apparently missed the bugmail for you attaching this minimal patch... I know I've been slow to review patches in the past, but you shouldn't let that prevent you from poking me about it directly before two weeks have elapsed without a response, especially when the patch is this small...
Attachment #356515 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 5

9 years ago
Thanks Gavin. Requesting for 1.9.1 as well since this is needed for SeaMonkey to reset our custom attributes.
Status: NEW → ASSIGNED
Flags: wanted1.9.1?
Keywords: checkin-needed
Whiteboard: checkin comment #4

Comment 6

9 years ago
You need to get it landed on trunk first, then request approval1.9.1? (on the attachment).

Comment 7

9 years ago
http://hg.mozilla.org/mozilla-central/rev/008a692ecbca
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: checkin comment #4
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 8

9 years ago
Comment on attachment 356515 [details] [diff] [review]
Patch v2.0 (Minimal Fix)

Requesting approval for 1.9.1 because SeaMonkey needs this to restore our custom attributes on reset.

1. Has landed on trunk and baked for a few days.
2. Performance impact: Any potential impact will occur only if the reset toolbars to defaults button is pressed which should be minimal.
3. Risk assessment: This is a minimal change/enhancement. All existing consumers (except SeaMonkey) ignore the return value. Regressions are highly unlikely.
Attachment #356515 - Flags: approval1.9.1?
Attachment #356515 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 356515 [details] [diff] [review]
Patch v2.0 (Minimal Fix)

a191=beltzner
Comment on attachment 356515 [details] [diff] [review]
Patch v2.0 (Minimal Fix)

a191=beltzner
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [has approval1.9.1][comment 9]

Comment 11

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/39b6e0193053
Flags: wanted1.9.1?
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [has approval1.9.1][comment 9]
You need to log in before you can comment on or make changes to this bug.