Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18886 closed defect (fixed)

consider removing Pocket

Reported by: mcs Owned by: arthuredelstein
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff45-esr, TorBrowserTeam201605R, tbb-6.0-must
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Firefox 45 ESR includes Pocket client code and UI. We may want to remove it, since it encourages use of a third party service that we do not know much about.

Maybe all we need to do is set extensions.pocket.enabled = false to disable it.

It is also worth noting that for Firefox 46, the Pocket code has been pulled out of the core browser and moved to a system extension (see https://bugzilla.mozilla.org/show_bug.cgi?id=1215694)

Child Tickets

Change History (20)

comment:1 Changed 3 years ago by cypherpunks

In Firefox 45 ESR the preference is browser.pocket.enabled. As a defense-in-depth measure, browser.pocket.api and browser.pocket.site should be set to the empty string.

comment:2 Changed 3 years ago by arthuredelstein

Owner: changed from tbb-team to arthuredelstein
Status: newaccepted

comment:3 in reply to:  1 Changed 3 years ago by arthuredelstein

Replying to cypherpunks:

In Firefox 45 ESR the preference is browser.pocket.enabled. As a defense-in-depth measure, browser.pocket.api and browser.pocket.site should be set to the empty string.

Thanks for pointing these out. I agree that disabling Pocket is a good idea. Here's a patch that does that:

https://github.com/arthuredelstein/tor-browser/commit/18886
Hash: 9c1768026600b2f05efa2b31cb9f4a57b23220c2

comment:4 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201604R added
Status: acceptedneeds_review

comment:5 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Fixed with commit d592a0e41881c06e6d93c4f272ea7e4f3ea6e9bb on tor-browser-45.1.0esr-6.0-1, thanks.

comment:6 Changed 3 years ago by gk

Resolution: fixed
Status: closedreopened

Hrm. I still seem to have a "View Pocket List" item visible after clicking on the hamburger menu and "Bookmarks". Arthur, is that taken care by your patch, too? (I just flipped the prefs and restarted to test it)

comment:7 Changed 3 years ago by gk

Keywords: TorBrowserTeam201604 added; TorBrowserTeam201604R removed
Status: reopenedneeds_information

comment:8 Changed 3 years ago by bugzilla

Oh shi*! What's wrong with you, guys?
Mark, you are too shy with your "consider removing". Use "remove that crap to hell" instead!
Mozilla violated a lot of principles when integrated Pocket into Firefox, and then
https://bugzilla.mozilla.org/show_bug.cgi?id=1172126 became the 2nd most voted bug in history of Firefox!
Link in the description has everything you need to properly remove that crap from the codebase.
Just do it!

comment:9 Changed 3 years ago by gk

Keywords: TorBrowserTeam201605 added; TorBrowserTeam201604 removed

Moving tickets

comment:10 in reply to:  6 Changed 3 years ago by arthuredelstein

Replying to gk:

Hrm. I still seem to have a "View Pocket List" item visible after clicking on the hamburger menu and "Bookmarks". Arthur, is that taken care by your patch, too? (I just flipped the prefs and restarted to test it)

You're right - it doesn't hide those properly. Here's a torbutton patch that hides the menu item and button from the hamburger menu whenever the "browser.pocket.enabled" pref is set to false.

https://github.com/arthuredelstein/torbutton/commit/18886
5d8669d20152f2c6ad0f6beb908fbdc077710be7

comment:11 Changed 3 years ago by gk

Keywords: TorBrowserTeam201605R added; TorBrowserTeam201605 removed
Status: needs_informationneeds_review

comment:12 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

It seems it does not work for me as expected. I.e. once I get the Pocket button onto the toolbar disabling the pref removes it but a) after a restart I can't enable it anymore easily (by flipping the pref) and b) the button is still in my bookmarks menu despite the patch.

Setting the prefs in comment:3 results in the pocket but removed from the customizable UI panel and by default it is nowhere visible on Winodws/OS X/Linux. I guess this could be enough for now given all the other things we have to do. Closing this for now but feel free to reopen the ticket if I missed anything.

comment:13 Changed 3 years ago by arthuredelstein

Resolution: fixed
Status: closedreopened

I could have sworn this was working for me before! The bookmarks menu is definitely supposed to disappear. I'm going to reopen it to remind myself to figure out what's wrong. That way we can also commit a single good patch.

comment:14 Changed 3 years ago by arthuredelstein

Status: reopenedneeds_review

I understand the behavior better now. After studying the pocket code carefully I came up with the following patch to tor-browser.git. Please note that the "browser.pocket.enabled" patch only takes effect after restart. That's consistent with the behavior of Pocket itself, which is not enabled or disabled until after restart.

https://github.com/arthuredelstein/tor-browser/commit/18886+3
Hash 6844e1209e7c509c8dc67570567e61c38061f72d

This supersedes the torbutton patch in comment:10, which should not be used.

comment:15 Changed 3 years ago by gk

Keywords: TorBrowserTeam201605 added; TorBrowserTeam201605R removed
Status: needs_reviewneeds_revision

Neat idea and I guess Mozilla would even be amenable to take that patch. However, that still does not seem enough. I stumbled over another Pocket button while looking at the Reader functionality (see: https://mxr.mozilla.org/mozilla-esr45/source/browser/modules/ReaderParent.jsm#81). There are even more instances of CustomizableUI.getPlacementOfWidget("pocket-button") we might want to take care of when upstreaming (even though I did not figure out how to get a related Pocket button shown in Tor Browser for these cases).

comment:16 in reply to:  15 Changed 3 years ago by arthuredelstein

Status: needs_revisionneeds_review

Replying to gk:

Neat idea and I guess Mozilla would even be amenable to take that patch. However, that still does not seem enough. I stumbled over another Pocket button while looking at the Reader functionality (see: https://mxr.mozilla.org/mozilla-esr45/source/browser/modules/ReaderParent.jsm#81). There are even more instances of CustomizableUI.getPlacementOfWidget("pocket-button") we might want to take care of when upstreaming (even though I did not figure out how to get a related Pocket button shown in Tor Browser for these cases).

Thanks for pointing these out. Here is a new version that takes care of the pocket button in Reader Mode and also the unlikely event of Pocket items in the context menu.

https://github.com/arthuredelstein/tor-browser/commit/18886+4
Hash c3fc62020f4b12aaa633d5760ea77999a97c5f11

I haven't modified the UITour code as we are already suppressing the UITour itself.

comment:17 Changed 3 years ago by gk

Keywords: tbb-6.0-must added

comment:18 Changed 3 years ago by gk

Keywords: TorBrowserTeam201605R added; TorBrowserTeam201605 removed

comment:19 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Fine with me (although we probably will need at least the UITour code path patched as well when upstreaming this fix which we should try as the current behavior is weird). Commit 320de5db6d53f53aadfad785ce31826b3080e890 on tor-browser-45.1.0esr-6.0-1 has the fix.

comment:20 in reply to:  19 Changed 3 years ago by gk

Replying to gk:

Fine with me (although we probably will need at least the UITour code path patched as well when upstreaming this fix which we should try as the current behavior is weird). Commit 320de5db6d53f53aadfad785ce31826b3080e890 on tor-browser-45.1.0esr-6.0-1 has the fix.

Ah, no, the code got pulled out into an own extension. Not sure if we want to upstream the changes into that system extension.

Note: See TracTickets for help on using tickets.