Closed Bug 1398375 Opened 7 years ago Closed 7 years ago

PageActions forgets about actions added after bug 1397501 landed

Categories

(Firefox :: Menus, defect)

57 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(1 file)

I broke this in bug 1397501.  I removed this line but didn't replace it with an equivalent: https://hg.mozilla.org/mozilla-central/rev/1f4df3c6e697#l3.62

The result is that for actions that you add after that bug landed, PageActions forgets whether you've added them to or removed them from the urlbar -- whether you've changed their shownInUrlbar.  Newly registered actions aren't added to _persistedActions.ids, so PageActions always thinks they're new, so it always obeys their initial shownInUrlbar.  Fortunately this ids array is only used to determine whether to obey the initial shownInUrlbar, so there's nothing more serious that's broken.

Someday I'll get this right.
(In reply to Drew Willcoxon :adw from comment #0)
> The result is that for actions that you add after that bug landed,
> PageActions forgets whether you've added them to or removed them from the
> urlbar -- whether you've changed their shownInUrlbar.

So I assume this means that if you add such an action that isn't builtin (which I think is what screenshots does, for instance?) and then add it to the urlbar, next startup it won't be there anymore? Can we explicitly test for this feature working (ie the node being in the urlbar after reinitializing the page action module from the prefs, or something) in the test?
Comment on attachment 8906154 [details]
Bug 1398375 - PageActions forgets about actions added after bug 1397501 landed.

https://reviewboard.mozilla.org/r/177880/#review182986
Attachment #8906154 - Flags: review?(gijskruitbosch+bugs) → review+
Yes, that's right.  In fact I noticed this while working on bug 1395387, where I'm testing by making screenshots use the WebExtensions page action API.  I gave its action a new ID, and it didn't stick around in the urlbar in between app restarts.

I'll see about a test before landing.
OK, this modifies the first big test task, the one that adds a non-built-in action and tests all its properties.  It simulates app restart by unregistering the action and then registering it again.  I'll land this now.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9a45df8d68a
PageActions forgets about actions added after bug 1397501 landed. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/d9a45df8d68a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.