Add a Screenshots item to the library panel

ASSIGNED
Assigned to

Status

()

Firefox
Toolbars and Customization
P1
normal
ASSIGNED
3 months ago
4 days ago

People

(Reporter: bbell, Assigned: Gijs)

Tracking

(Blocks: 2 bugs, {leave-open})

unspecified
leave-open
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
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
(Reporter)

Updated

3 months ago
Blocks: 1352110
Whiteboard: [photon-structure]
(Reporter)

Comment 1

3 months ago
Mock up: https://mozilla.invisionapp.com/share/87BS424CV#/234494878_Explanation_-_Library_Menu_-_Screenshots
(Reporter)

Updated

3 months ago
Blocks: 1354155
(Reporter)

Updated

3 months ago
No longer blocks: 1354155
(Reporter)

Updated

3 months ago
Blocks: 1354155

Updated

3 months ago
Flags: qe-verify?
Priority: -- → P2
Comment hidden (obsolete)

Comment 3

3 months ago
: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?
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (obsolete)

Comment 5

3 months ago
: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
(Assignee)

Comment 6

3 months ago
(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.
(Assignee)

Comment 7

3 months ago
(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.
Flags: qe-verify? → qe-verify+

Comment 8

3 months ago
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.
Flags: qe-verify+ → qe-verify?

Comment 9

3 months ago
> 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!
(Assignee)

Updated

3 months ago
Flags: qe-verify? → qe-verify+

Updated

3 months ago
QA Contact: gwimberly

Comment 10

3 months ago
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?
Flags: needinfo?(amckay)
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.
(Reporter)

Comment 12

3 months ago
(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

3 months ago
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.
(Assignee)

Updated

2 months ago
Whiteboard: [photon-structure] → [reserve-photon-structure]
(Assignee)

Updated

2 months ago
Priority: P2 → P3

Updated

2 months ago
Blocks: 1371543

Updated

2 months ago
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
(Assignee)

Comment 14

a month ago
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
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
(Assignee)

Comment 17

a month ago
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?
Flags: needinfo?(ianb)
(Assignee)

Comment 18

a month ago
(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).
Flags: needinfo?(ianb)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

a month ago
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

29 days ago
mozreview-review
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?
Attachment #8887989 - Flags: review?(jaws) → review+
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)
(Assignee)

Comment 24

29 days ago
(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 hidden (mozreview-request)
(Assignee)

Comment 26

29 days ago
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.
Keywords: leave-open

Comment 27

29 days ago
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.
(Assignee)

Comment 29

28 days ago
(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

28 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e21a53b6903e

Updated

25 days ago
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1

Updated

17 days ago
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Blocks: 1387512

Updated

11 days ago
No longer blocks: 1371543
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
You need to log in before you can comment on or make changes to this bug.