Closed Bug 875775 Opened 10 years ago Closed 10 years ago

Allow widgets created by createWidget to specify if they're removable or not.


(Firefox :: Toolbars and Customization, defect)

Not set



Firefox 28


(Reporter: mconley, Assigned: jaws)


(Blocks 1 open bug)


(Whiteboard: [Australis:M6])


(2 files, 1 obsolete file)

Since bug 866978, we respect the removable="true" attribute on XUL nodes to know whether or not we can take certain items out of their customizable areas.

We do not, however allow widget created via createWidget to set this attribute. We should allow this.
Is this something that is needed for m6 though? I don't think we have any widgets that need this API change right away.
Good point. I was being overzealous. :)
Whiteboard: [Australis:M6] → [Australis:M?]
Taking this since I just wontfix'ed bug 875809.
Assignee: mconley → jaws
Hardware: x86_64 → All
Attached patch PatchSplinter Review
Up for grabs to whoever can review it sooner :)
Attachment #754869 - Flags: review?(mconley)
Attachment #754869 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 754869 [details] [diff] [review]

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +684,5 @@
>      node.setAttribute("widget-type", aWidget.type);
>      if (aWidget.disabled) {
>        node.setAttribute("disabled", true);
>      }
> +    node.setAttribute("removable", !!aWidget.removable);

Do we really need the coercion here? We seem to have done a pretty good job of ensuring that it's only a boolean that gets assigned to aWidget.removable.
Attachment #754869 - Flags: review?(mconley) → review+
Attachment #754869 - Flags: review?(gijskruitbosch+bugs)
Removed the coercion and landed on the UX branch,
Whiteboard: [Australis:M?] → [Australis:M?][fixed-in-ux]
Whiteboard: [Australis:M?][fixed-in-ux] → [Australis:M6][fixed-in-ux]
(In reply to Jared Wein [:jaws] from comment #6)
> Removed the coercion and landed on the UX branch,

I believe this block needed to be updated, too:
Attached patch Patch part 2, followup (obsolete) — Splinter Review
Thanks for catching that Gijs. I didn't notice it because all of our current widgets are removable :)
Attachment #755370 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 755370 [details] [diff] [review]
Patch part 2, followup

Hrm. r+ if there is really no sane way to get the spec of the API-provided widget and actually check the removable property. Which would seem strange to me in and of itself, tbh...
Attachment #755370 - Flags: review?(gijskruitbosch+bugs) → review+
Yeah, this is better.
Attachment #755370 - Attachment is obsolete: true
Attachment #755382 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 755382 [details] [diff] [review]
Patch part 2, followup v1.1

>diff --git a/browser/components/customizableui/src/CustomizableUI.jsm b/browser/components/customizableui/src/CustomizableUI.jsm

>+      return gPalette.get(aWidgetId).removable;
>+    } else if (provider == CustomizableUI.PROVIDER_XUL) {

Nit: please don't use else after a return.

With that, r=me
Attachment #755382 - Flags: review?(gijskruitbosch+bugs) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.