Closed Bug 1498914 Opened 11 months ago Closed 10 months ago

Removal of Pocket icon from address bar does not persist across a restart

Categories

(Firefox :: Toolbars and Customization, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 + verified

People

(Reporter: MikkCZ, Assigned: thecount)

References

Details

(Keywords: nightly-community, regression)

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1483033 +++

The same what happened with Screenshots page action is now happening with Pocket page action. After rester, it shows in the address bar again, despite I have hidden it the previous run.
Duplicate of this bug: 1498895
[Tracking Requested - why for this release]:
Functionality regression that we shouldn't ship (ideally, even to beta).

Scott, can you investigate this? Drew (:adw) should be able to help if you're stuck, and bug 1483033 might offer clues about why this is broken.
Blocks: 1488813
Flags: needinfo?(sdowne)
I can look into this.
Flags: needinfo?(sdowne)
One thing that sticks out to me is this is no longer an extension. It was moved into a component, which is what broke this.

I'm not sure if the fixes in bug 1483033 would still apply, given those fixes were for an extension? I'll take a look anyway, but any info Drew has around doing something similar without it being an extension would be appreciated. :)
Flags: needinfo?(adw)
Assignee: nobody → sdowne
From more investigation, it looks like it has something to do with when to call shutdown and when to not, based on the old extension code's reason value? If shutdown is called, then next startup re adds the icon. Looks at the logic of the patch for screenshots, this would make sense.
It looks like you can remove the pageAction.remove() call here: https://hg.mozilla.org/mozilla-central/file/4c11ab0cd989/browser/components/pocket/content/SaveToPocket.jsm#l239

remove() is really only for actions that live in extensions or code that can otherwise be disabled (like behind a pref).  remove() tells the page actions machinery to forget about the action, including whether the action is in the urlbar or not.  If Pocket isn't an extension anymore -- or at least, if it can no longer be disabled -- then you probably don't ever want to call that.  In fact that whole shutdown() method doesn't look necessary anymore.  I wouldn't think you would ever need to remove #pocket-button-box either.
Flags: needinfo?(adw)
I noticed that when the browser is not running, prefs.js does not have "pocket" in the list of "ids" for browser.pageActions.persistedActions like it did before. Manually adding it and starting the browser causes the page action to behave correctly and abide by "idsInUrlbar" until the next restart.
Yes, that's due to the remove() call, which makes Firefox forget about the Pocket page action.
Great, this all makes sense to me now, I should have a fix in a bit.
@adw Cool if I request review from you for this?
Flags: needinfo?(adw)
Thanks, r+'ed
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
Keywords: checkin-needed
Pushed by elee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43db36acc04f
Removal of Pocket icon from address bar persist across a restart r=adw
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/43db36acc04f
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Hi, I checked this issue in Firefox Nightly 64.0a1 (2018-10-19) and the issue no longer occurs, I will mark this accordingly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.