Support removal of built-in buttons

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 years ago
Posted 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 1

4 years ago
Comment on attachment 8693727 [details] [diff] [review]
cui-obsolete

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+
Assignee

Comment 2

4 years ago
Posted patch cui-obsoleteSplinter Review
comments addressed.
Assignee: nobody → mixedpuppy
Attachment #8693727 - Attachment is obsolete: true
Attachment #8693767 - Flags: review+
Blocks: 1229351

Comment 4

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/97258d49fa01
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Updated

3 years ago
Blocks: 1236014
You need to log in before you can comment on or make changes to this bug.