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

Status: VERIFIED FIXED

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

Description

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

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

Comment 2

[:Gijs (he/him)]

[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.

Comment 3

[Scott [:thecount] Downe]

I can look into this.

Comment 4

[Scott [:thecount] Downe]

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. :)

Comment 5

[Scott [:thecount] Downe]

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.

Comment 6

[Drew Willcoxon :adw]

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.

Comment 7

[Kestrel]

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.

Comment 8

[Drew Willcoxon :adw]

Yes, that's due to the remove() call, which makes Firefox forget about the Pocket page action.

Comment 9

[Scott [:thecount] Downe]

Great, this all makes sense to me now, I should have a fix in a bit.

Comment 10

[Scott [:thecount] Downe]

Attachment: Removal of Pocket icon from address bar persist across a restart

Comment 11

[Scott [:thecount] Downe]

@adw Cool if I request review from you for this?

Comment 12

[Drew Willcoxon :adw]

Thanks, r+'ed

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43db36acc04f
Removal of Pocket icon from address bar persist across a restart r=adw

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/43db36acc04f

Comment 15

[Rares Doghi, Desktop QA]

Hi, I checked this issue in Firefox Nightly 64.0a1 (2018-10-19) and the issue no longer occurs, I will mark this accordingly.

Comment 16

[tukusejssirs]

This does not work on FF Nightly 74.0a1. Should I open a new issue?

Comment 17

[Scott [:thecount] Downe]

Yeah, a new issue would be ideal.

Also I just tried this on nightly, and the Pocket icon removal does persist across a restart on my end.

Do you have any steps I need to do to reproduce the issue?

Also are you using firefox sync? It's just a guess, but maybe there is a bug with sync, and if you don't have sync setup that would quickly rule that out as a potential cause.

Comment 18

[tukusejssirs]

Okay, I have created a new issue #1611516.

I tried to list the steps in that issue.

And yes, I am using Firefox sync.

Comment 19

[Scott [:thecount] Downe]

I tried doing this with Pocket on my end and it stays gone. I'll see if I can reproduce the bug playing around with sync settings.