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

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

: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?

: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

(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.

(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.

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.

> 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!

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?

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.

(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.

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.

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.

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

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

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?

(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.

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).

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 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?

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)

(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.

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.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e21a53b6903e add a screenshots item to the library, r=jaws

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.

(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. :-)

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?

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.

(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.

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

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