Closed Bug 1498914 Opened 3 years ago Closed 3 years ago
Removal of Pocket icon from address bar does not persist across a restart
46 bytes, text/x-phabricator-request
|Details | Review|
+++ 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.
[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.
I can look into this.
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. :)
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.
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?
Status: NEW → ASSIGNED
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/43db36acc04f Removal of Pocket icon from address bar persist across a restart r=adw
Hi, I checked this issue in Firefox Nightly 64.0a1 (2018-10-19) and the issue no longer occurs, I will mark this accordingly.
You need to log in before you can comment on or make changes to this bug.