1386474 - Pocket shouldn't assume that Photon page actions are enabled

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Drew Willcoxon :adw]

Bug 1367927 added a Photon page action for Pocket, but the patch assumed that Photon is enabled, which (I think?) will break on Cedar.  See bug 1386337 comment 3.

When Photon is disabled, Pocket should fall back to the CUI navbar button.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1386474 - Pocket shouldn't assume that Photon page actions are enabled.

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

Comment 2

[Drew Willcoxon :adw]

Fortunately, with the way I fixed bug 1367927, this is only a one-line fix!

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8892712 [details]
Bug 1386474 - Pocket shouldn't assume that Photon page actions are enabled.

https://reviewboard.mozilla.org/r/163708/#review169274

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31b6638af81b
Pocket shouldn't assume that Photon page actions are enabled. r=mixedpuppy

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/31b6638af81b

Comment 6

[:Gijs (he/him)]

Comment on attachment 8892712 [details]
Bug 1386474 - Pocket shouldn't assume that Photon page actions are enabled.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1367927
[User impact if declined]: broken tests, maybe broken behaviour where we try to use page action stuff on 56  beta/release when we shouldn't.
[Is this code covered by automated tests?]: yup
[Has the fix been verified in Nightly?]: not really applicable
[Needs manual test from QE? If yes, steps to reproduce]: check you can pocket something with the button in the toolbar should be more than enough to verify this
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: nope
[Why is the change risky/not risky?]: 1-line change to avoid using the page action code.
[String changes made/needed]: none.

Comment 7

[Liz Henry (:lizzard)]

Comment on attachment 8892712 [details]
Bug 1386474 - Pocket shouldn't assume that Photon page actions are enabled.

Let's avoid breaking some tests. OK to uplift to beta.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Bug 1367927 didn't make it to mozilla-central before 56 merged to Beta, so I don't think there's anything to do here unless we're planning to uplift that bug too?

Comment 9

[Drew Willcoxon :adw]

I think that's right, yeah.  The patch to this bug depends on the patch to bug 1367927, and we're not planning on uplifting that.

Comment 10

[Liz Henry (:lizzard)]

Comment on attachment 8892712 [details]
Bug 1386474 - Pocket shouldn't assume that Photon page actions are enabled.

Sounds like we don't need this after all.

Comment 11

[:Gijs (he/him)]

Oops, sorry for the confusion!