1389721 - Right-click > Save Link to Pocket doesn't work

Status: VERIFIED FIXED

(Firefox :: Pocket, defect, P1)

Description

[Emmy]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170811100330

Steps to reproduce:

– Right-click a link on a webpage
– Select "Save Link to Pocket" in context menu


Actual results:

There is no indication that the action has been completed, and upon checking the Pocket list, the link is not present.


Expected results:

The link should be saved to the user's Pocket list.

Comment 1

[[:philipp]]

the issue is reproducible and running mozregression on it points to bug 1355922 as the source of the regression.

the browser console logs the following while attempting to save a link to pocket through the context menu:
TypeError: event is undefined  browser-pageActions.js:464:1
	doCommandForAction chrome://browser/content/browser-pageActions.js:464:1
	doCommand resource:///modules/PageActions.jsm:705:5
	savePage chrome://pocket/content/Pocket.jsm:106:7
	oncommand chrome://browser/content/browser.xul:1:1

Comment 2

[:Gijs (he/him)]

Photon is disabled and Philipp confirmed this doesn't actually reproduce on release, so updating some tracking info. :-)

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1389721 - fix page action menu to deal with not being passed an event, so the 'save link to pocket' context menu works,

Review commit: https://reviewboard.mozilla.org/r/185808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185808/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8914479 [details]
Bug 1389721 - fix page action menu to deal with not being passed an event, so the 'save link to pocket' context menu works,

https://reviewboard.mozilla.org/r/185808/#review190900

Thanks for fixing this and sorry for breaking it :(

::: commit-message-97efd:1
(Diff revision 1)
> +Bug 1389721 - fix 'save link as' pocket context menu functionality, r?jaws

Please change the commit message to be more descriptive.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8914479 [details]
Bug 1389721 - fix page action menu to deal with not being passed an event, so the 'save link to pocket' context menu works,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185808/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63c7cfadae7b
fix page action menu to deal with not being passed an event, so the 'save link to pocket' context menu works, r=jaws

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/63c7cfadae7b

Comment 8

[:Gijs (he/him)]

Comment on attachment 8914479 [details]
Bug 1389721 - fix page action menu to deal with not being passed an event, so the 'save link to pocket' context menu works,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1355922 and then some other photon-related changes
[User impact if declined]: completely broken context menu item (save page/link to pocket)
[Is this code covered by automated tests?]: no. pocket code doesn't have enough automated tests, unfortunately. I'll file a follow-up for this. We shouldn't be able to break things this badly without the tests yelling at us.
[Has the fix been verified in Nightly?]: not yet.
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: well, it's hard to get more broken than completely broken. I guess crashing the browser... In all seriousness, this is a straightforward small nullcheck in the right place, plus removing some dead code. Happily, I'm not trying to crashland this into rc either, but even then I'd be pretty OK with saying we should take this (!).
[String changes made/needed]: none

Comment 9

[:Gijs (he/him)]

I filed bug 1405477 for adding more automated tests for pocket functionality.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8914479 [details]
Bug 1389721 - fix page action menu to deal with not being passed an event, so the 'save link to pocket' context menu works,

We probably want that to be fixed for 57. Taking it to 57b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a766e6d5e5e7

Comment 12

[Emil Ghitta, QA [:emilghitta]]

I have reproduced the issue mentioned in comment 0, using an affected Firefox 57.0a1 build (BuildId:20170811100330).

I have verified that the issue is not reproducible using Firefox 57.0b6 (Build Id:20171005195903) and Firefox 58.0a1 (BuildId:20171009100134) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.