1246460 - "View Pocket List" menu item missing since the removal of "Subscribe to this page…" in the bookmark button and menu bar

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Matthew N. [:MattN]]

Attachment: Missing Pocket in the menu

Important: I'm talking about the menu from the bookmark button on a toolbar and the menu bar, not in the app menu panel (which is still has the Pocket item in the subview).

Bug 985659 removed BMB_subscribeToPageMenuitem which Pocket used as an insertion point for it's bookmark menu item: https://mxr.mozilla.org/mozilla-central/source/browser/extensions/pocket/bootstrap.js?rev=cb217f7271c2#424

The result is that the menu item no longer appears. The code gracefully handles this so no errors were output. We need to adjust the insertion point in bootstrap.js.

We should probably add a simple test for this and the other UI entry points of Pocket. I just happened to notice the regression while bisecting a different front-end regression.

[Tracking Requested - why for this release]: Major Pocket UI regression

Comment 1

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #0)
> We should probably add a simple test for this and the other UI entry points
> of Pocket. I just happened to notice the regression while bisecting a
> different front-end regression.

I wrote one, but it won't run properly because pocket isn't enabled at all in our tests. See bug 1235627. Enabling it just for the test fails because it leaks window properties, so I gave up on doing that. Still including the test in the patch because it seems useful to have it.

Comment 2

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1246460 - pocket UI disappeared, r?MattN

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

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> Still including the
> test in the patch because it seems useful to have it.

... and I expect us to fix the test issues in time for 46, and when we do, this will 'just work', ideally.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8716876 [details]
MozReview Request: Bug 1246460 - pocket UI disappeared, r?MattN

https://reviewboard.mozilla.org/r/33971/#review30719

Nit: the commit message could be improved to be more specific.

::: browser/base/content/test/social/browser.ini:35
(Diff revision 1)
> +[browser_pocket_ui_check.js]

I would prefer if this test was in browser/extensions/pocket/… Not sure why this belongs in the social dir.

Comment 5

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #4)
> Comment on attachment 8716876 [details]
> MozReview Request: Bug 1246460 - pocket UI disappeared, r?MattN
> 
> https://reviewboard.mozilla.org/r/33971/#review30719
> 
> Nit: the commit message could be improved to be more specific.
> 
> ::: browser/base/content/test/social/browser.ini:35
> (Diff revision 1)
> > +[browser_pocket_ui_check.js]
> 
> I would prefer if this test was in browser/extensions/pocket/…

There are no tests in there right now. At all. I can start a set of tests, I guess...

> Not sure why
> this belongs in the social dir.

Anything over sticking yet another thing in browser/base/content/test/general/ . Pocket seemed more linked to social than to anything else.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e0be6880f8e0

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0be6880f8e0

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Seems like a core scenario. Tracked for Fx47.

Comment 9

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 47b8 Win 7.