Closed Bug 1229123 Opened 7 years ago Closed 7 years ago

Support removal of built-in buttons


(Firefox :: Toolbars and Customization, defect)

Not set



Firefox 45
Tracking Status
firefox45 --- fixed


(Reporter: mixedpuppy, Assigned: mixedpuppy)




(1 file, 1 obsolete file)

Attached patch cui-obsolete (obsolete) — Splinter Review
This is important for hello and pocket functionality to be moved into addons.

If you remove a built in button, and add it into an addon, there is no way for the addon or CUI to determine whether the button had been placed into the palette.  built-in buttons are not added to the "seen" state.  This patch adds an obsolete object that will be checked on startup, if a button is removed in that version then it is added to the "seen" state.  With this, createWidget will properly place the button into the palette if that was the prior location.

This patch was developed and tested as part of bug 1215694 but is separated out to allow hello to address the same issue.  As such, the obsolete object is currently empty and will be addressed in the respective bugs for pocket and hello.
Attachment #8693727 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8693727 [details] [diff] [review]

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

::: browser/components/customizableui/CustomizableUI.jsm
@@ +370,5 @@
> +      return;
> +    // we're upgrading, update state if necessary
> +    for (let id in ObsoleteBuiltinButtons) {
> +      let version = ObsoleteBuiltinButtons[id]
> +      if (version == kVersion && !gSeenWidgets.has(id)) {

Nit: don't need to check gSeenWidgets.has(), it's a set, so .add() will just no-op if the item exists already.

@@ +374,5 @@
> +      if (version == kVersion && !gSeenWidgets.has(id)) {
> +        gSeenWidgets.add(id);
> +      }
> +    }
> +  },

I wonder if you need to set gDirty to true, and if that will be enough to ensure the state is saved. Can you check what happens if you:

1) create new profile without this patch
2) remove pocket
3) run on a build with pocket in the list of obsolete thingies in here
4) restart on that build without entering customization mode or whatever

do we save the state back to the pref? How?

I think right now we won't, and so we should make sure that that happens one way or another.

r=me with that addressed, if it's much more work than just adding gDirty (or if my assumption that we're not saving is totally wrong) it might be good for me to have another look, but up to you.
Attachment #8693727 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch cui-obsoleteSplinter Review
comments addressed.
Assignee: nobody → mixedpuppy
Attachment #8693727 - Attachment is obsolete: true
Attachment #8693767 - Flags: review+
Blocks: 1229351
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Blocks: 1236014
You need to log in before you can comment on or make changes to this bug.