1366026 - Add a Screenshots item to the library panel

Status: VERIFIED FIXED

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

Description

[bbell]

Screenshots are going to be a system-add-on when 57 lands. The Library needs to link to a user's list of screenshots. 

URL: https://screenshots.firefox.com/shots

Comment 1

[bbell]

Mock up: https://mozilla.invisionapp.com/share/87BS424CV#/234494878_Explanation_-_Library_Menu_-_Screenshots

Comment 2

[:Gijs (he/him)]

This is going to require screenshots to move away from webextensions, or for there to be a webextensions API / changes to the page_action webextension API so they integrate with the menu.

I don't know if the former is going to happen, and I would be surprised if the photon structure team have time to work on the latter. Andy, can the webextensions team help?

Comment 3

[John Gruen [:jgruen]]

:Gijs seems like this could be a separate thing managed outside the Screenshots add-on. Functionally all the button would need to do would be to point at the screenshots server. Am i missing something?

Comment 4

[:Gijs (he/him)]

(In reply to [:jgruen] from comment #3)
> :Gijs seems like this could be a separate thing managed outside the
> Screenshots add-on. Functionally all the button would need to do would be to
> point at the screenshots server. Am i missing something?

How does it get the relevant URL? How does it know when to show/hide, or show an error that links to some kind of setup thing - otherwise, what is "the user's" list of screenshots (presumably there's sign-in state somewhere? Or are all the screenshots public? That seems unlikely, but we'd still need some kind of user-specific identifier or we'd just show all the public screenshots, not just those from the user)

If the webextension can expose APIs for all those bits somehow (which I'm not even sure it technically can, from inside the webextension sandbox without being an embedded webextension) then yes, it could live outside it. We still likely wouldn't have time to do all that work.

Or is comment #0 just wrong and should it take a screenshot? Even for that we'd need APIs, plus checking if the add-on is even enabled, configured, etc.

Comment 5

[John Gruen [:jgruen]]

:Gijs might be helpful to understand how Pocket is integrating into the library for Photon. Is that integration part of the Pocket add-on?

And comment zero is correct here, :bbell also filed 1366041 to track the page action piece which would trigger the WebExtension piece of Screenshots

Comment 6

[:Gijs (he/him)]

(In reply to [:jgruen] from comment #5)
> :Gijs might be helpful to understand how Pocket is integrating into the
> library for Photon. Is that integration part of the Pocket add-on?

Pocket is a bootstrapped add-on. I don't know how we'll implement this, but it could easily be a part of the add-on, yes.

Comment 7

[:Gijs (he/him)]

(In reply to [:jgruen] from comment #5)
> And comment zero is correct here, :bbell also filed 1366041 to track the
> page action piece which would trigger the WebExtension piece of Screenshots

Gah, my bugmail confused me about which menu we were talking about, sorry.

So, there's an existing web extension API for page actions. This could hook into the things in bug 1363188 to have webextension-based page actions. So that would work for bug 1366041 (but it would still be work that needs doing and that might be non-trivial).

For this bug, we'd need a new (Firefox-specific) webextension API, unless we either hand-roll some kind of solution specific to screenshots, or we make screenshots a bootstrapped add-on.

Comment 8

[John Gruen [:jgruen]]

Ah, well we're Bootstrapping Dcreenshots to mitigate some of the performance issues we experienced in 54. Maybe that will obviate the question of new WebExtension APIs for registering in the Library.* I'll let Ian or Jared comment on this issue so that we can start to figure out the steps we'll need to take on our end to make this feasible. It sounds like effort sharing with the Pocket folks might be in order. 

*If i understand :bbell's vision for Photon, i suspect such an API might be a good thing to have going forward.

Comment 9

[John Gruen [:jgruen]]

> Gah, my bugmail confused me about which menu we were talking about, sorry.

No sweat

> For this bug, we'd need a new (Firefox-specific) webextension API, unless we either hand-roll some kind of solution specific to screenshots, or we make screenshots a bootstrapped add-on.

Per my comment above, we are!

Comment 10

[Andy McKay]

Just looking at the library in the invision doc, without any other context, it seems like a really useful place that we should add an extension point for WebExtension authors, including Test Pilot.

There's nothing in that UI demonstrating what it would look like for WebExtension authors. Would they be able insert themselves in the same place that screenshots is meant to be inserted? Would there be any more APIs around like setting badge text or something?

Comment 11

[Ian Bicking (:ianbicking)]

A WebExtension API would be ideal of course, then we could simply register ourselves and rely on the API to do the rest. But we certainly have the option of doing things in bootstrap.js until that point.

Ideally there would be an API (WebExtension or not) where Screenshots could register its menu item, and so if Screenshots is not activated then the menu item won't be added. 

I don't see anything actionable for Screenshots right now, so we will wait for further instructions.

Comment 12

[bbell]

(In reply to Andy McKay [:andym] from comment #10)
> Just looking at the library in the invision doc, without any other context,
> it seems like a really useful place that we should add an extension point
> for WebExtension authors, including Test Pilot.
> 
> There's nothing in that UI demonstrating what it would look like for
> WebExtension authors. Would they be able insert themselves in the same place
> that screenshots is meant to be inserted? Would there be any more APIs
> around like setting badge text or something?

The answer is yes, both that "Library" (a place dedicated to recovering user generated content) and the "Page Action Menu" (a place committed to saving content) will need extension support.

Comment 13

[Andy McKay]

Probably best to put those APIs in separate bugs, so I filed bug 1366389 and bug 1366391 for creation of the WebExtension APIs with some questions we'll need to answer to build out an API for those.

Comment 14

[:Gijs (he/him)]

Given how much work the page action stuff is turning out to be (and that's *before* we've done any webextensions work), I think the appropriate solution for 57 will be to just add code to bootstrap.js . I'll try to come up with a PR.

Comment 15

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I think the simplest thing would be to make UI changes in the webextension, replacing the browserAction with a pageAction.

Screenshots currently loads its UI (button and context menu items) in the webextension, and defers loading most of the webextension code until that UI is clicked[1].

If you want to create a pageAction in bootstrap.js, you'd also have to send a signal to the webextension to ask it to lazy load the bulk of the code. We currently only do one-way messaging from webextension to bootstrap[2], so I think you'd need to set up a port (via runtime.connect / runtime.onConnect), set up a message listener on the webextension side, then have bootstrap send over a message when the bootstrap-managed pageAction item is clicked.
  You'd also want to guard against creating that pageAction if screenshots is disabled[3], and remove it if screenshots is disabled[4].

I think that pretty much covers it.

[1] https://github.com/mozilla-services/screenshots/blob/master/addon/webextension/background/startBackground.js#L30
[2] https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js#L114
[3] https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js#L105-L106
[4] https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js#L108

Comment 16

[Ian Bicking (:ianbicking)]

I think it's reasonable to do it without pageAction, though for Screenshots either is acceptable.  There's some code here (in a no-longer-active branch) that opens up a postMessage channel: https://github.com/mozilla-services/screenshots/blob/lazybutton/addon/bootstrap.js#L94-L105

On that branch, this is the receiving end: https://github.com/mozilla-services/screenshots/blob/lazybutton/addon/webextension/background/main.js#L310-L320

The code has been changed since then, and now the receiving end of postMessage would go in this file: https://github.com/mozilla-services/screenshots/blob/master/addon/webextension/background/startBackground.js

Comment 17

[:Gijs (he/him)]

So, I caused some confusion here in comment #2 and later (I'm going to mark those comments obsolete to try to help clarify), but I think the page action stuff is bug 1366041, which could be worked on independently and should indeed replace the browser action with a page action (that shows up in the page action menu, ideally).

This bug is specifically about the item in the library menu. I think that's orthogonal to the page action stuff and should be created together with the browser/page action, when we call webextension startup. :-)

Does that make sense?

Comment 18

[:Gijs (he/him)]

(In reply to :Gijs (no reviews, mostly out until Jul 17) from comment #14)
> Given how much work the page action stuff is turning out to be (and that's
> *before* we've done any webextensions work), I think the appropriate
> solution for 57 will be to just add code to bootstrap.js . I'll try to come
> up with a PR.

To be clear, I meant this in an analogous fashion (because X is a lot of work, I expect Y is also a lot of work) - this bug doesn't depend on the page action work.

Comment 19

[Ian Bicking (:ianbicking)]

Ah, you are right.  Then simply navigating to {backend}/shots is correct.  We have some bugs around navigating directly to that page when you haven't used Screenshots before, but we should fix those in Screenshots.  Then the only hard part is finding the value of backend (though it will always be https://screenshots.firefox.com, because it's not really configurable except via a build process outside of the Firefox tree).

Comment 20

[:Gijs (he/him)]

Attachment: Bug 1366026 - add a screenshots item to the library,

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

Comment 21

[:Gijs (he/him)]

This patch includes screenshots changes, but I will just land the panelUI change here, and then wait for the github PR to land against screenshots and then make it to central independently (we should probably keep the bug open for this to happen). I simply included a (manually mirrored) set of changes here because it's simpler to test them that way.

Comment 22

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

Comment on attachment 8887989 [details]
Bug 1366026 - add a screenshots item to the library,

https://reviewboard.mozilla.org/r/158860/#review164770

::: browser/extensions/screenshots/bootstrap.js:20
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
> +                                  "resource:///modules/CustomizableUI.jsm");

I often see these in alphabetical order of the module name. Could you fix that here?

Comment 23

[Ian Bicking (:ianbicking)]

We've applied the patch to Screenshots. After landing an eslint error was reported, and I believe it refers to an actual bug in the code: https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js#L85-L90 (win is assigned but unused)

Comment 24

[:Gijs (he/him)]

(In reply to Ian Bicking (:ianb) from comment #23)
> We've applied the patch to Screenshots. After landing an eslint error was
> reported, and I believe it refers to an actual bug in the code:
> https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.
> js#L85-L90 (win is assigned but unused)

I've created a PR to fix this and reorder the imports as suggested in comment #22.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8887989 [details]
Bug 1366026 - add a screenshots item to the library,

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

Comment 26

[:Gijs (he/him)]

I've reduced the mozreview thing to just the CSS change and requested autoland on that. The actual fix will merge whenever a screenshots release with it in it makes it to m-c. I'll mark leave-open so we can close when that happens.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e21a53b6903e
add a screenshots item to the library, r=jaws

Comment 28

[Ian Bicking (:ianbicking)]

FYI, our release plan for Screenshots is to focus on version 10 releases until things settle down (right now we're deploying 10.8.0 and hope that it's what we ship in 55). After that we'll export a version (probably called version 12) into Nightly with this change and others. We'll be targeting Firefox 56 for version 12 of Screenshots.

Comment 29

[:Gijs (he/him)]

(In reply to Ian Bicking (:ianb) from comment #28)
> FYI, our release plan for Screenshots is to focus on version 10 releases
> until things settle down (right now we're deploying 10.8.0 and hope that
> it's what we ship in 55). After that we'll export a version (probably called
> version 12) into Nightly with this change and others. We'll be targeting
> Firefox 56 for version 12 of Screenshots.

I think that works. The code in screenshots should be fine even if the library panel is not being used (the UI opening it won't ship until 57). Though obviously, the sooner this is at least on nightly, the sooner it can be tested. :-)

Comment 30

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

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

Comment 31

[:Gijs (he/him)]

This landed in m-c on the 29th with bug 1390985! \o/

Marco, I reset the iteration flag, is that right and/or is there anything else that needs doing?

Comment 32

[Kanchan Kumari QA]

This is to confirm that there is screenshot item in library panel but I can't verify this bug completely as of yet due to unavailability to screenshot tool in the latest nightly. I filed a bug 1397921 for this.

I also encountered bug 1396370 which happens if there is no screenshot saved. If I save screenshot from latest Fx beta (which has screenshot tool) and use same profile with the latest nightly, I don't get server error and it shows saves screenshot.

I will retest this bug again once bug 1397921 get fixed.

Comment 33

[:Gijs (he/him)]

(In reply to Kanchan Kumari QA from comment #32)
> This is to confirm that there is screenshot item in library panel but I
> can't verify this bug completely as of yet due to unavailability to
> screenshot tool in the latest nightly. I filed a bug 1397921 for this.

As noted in that bug, the screenshots tool is in the page actions panel now (see bug 1366041). Please retest when that's possible.

Comment 34

[Kanchan Kumari QA]

Thanks! I verified this bug fix on the latest nightly.

Comment 35

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.