1237087 - Add ability for WebCompatReporter ("Report Site Issue") to send screenshots to new issue form

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Taylor [:miketaylr]]

We've updated webcompat.com to have better image uploading capabilities and would like reports that come from the Nightly "Report Site Issue" to attach a screenshot by default  - "anonymous" reports often contain very little context. 

Users will have the opportunity to remove it before submitting.

See https://github.com/webcompat/webcompat.com/issues/879#issuecomment-169107610 for a gif of the same functionality in our Chrome add-on.

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: Part 1 - Refactor WebCompatReport.reportIssue to take an object argument. r=?

Comment 2

[Mike Taylor [:miketaylr]]

Attachment: Part 2 - Send screenshot of visible window when reporting site issue. r=?

Comment 3

[Mike Taylor [:miketaylr]]

Attachment: Part 1 - Refactor WebCompatReport.reportIssue to take an object argument. r=?

Comment 4

[Mike Taylor [:miketaylr]]

Attachment: Part 2 - Send screenshot of visible window when reporting site issue. r=?

Comment 5

[Mike Taylor [:miketaylr]]

Comment on attachment 8727472 [details] [diff] [review]
Part 1 - Refactor WebCompatReport.reportIssue to take an object argument. r=?

Sebastian, would you mind reviewing?

Comment 6

[Mike Taylor [:miketaylr]]

In case it's helpful as context, here's where we handle incoming message events for the bug form (including making sure we only accept our own origin): https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/bugform.js#L52-L63

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8727472 [details] [diff] [review]
Part 1 - Refactor WebCompatReport.reportIssue to take an object argument. r=?

Review of attachment 8727472 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/WebcompatReporter.js
@@ +58,5 @@
>    addMenuItem: function(visible) {
>      this.menuItem = NativeWindow.menu.add({
>        name: this.strings.GetStringFromName("webcompat.menu.name"),
>        callback: () => {
> +        this.reportIssue({url: BrowserApp.selectedTab.browser.currentURI.spec;}});

This line seems to be malformed (semicolon and double closing "}"). But it looks like the next commit fixes this. Maybe collapse them?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8727473 [details] [diff] [review]
Part 2 - Send screenshot of visible window when reporting site issue. r=?

Review of attachment 8727473 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM but I would like Margaret to take a quick look too.

Comment 9

[Mike Taylor [:miketaylr]]

Thanks for review!

(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> This line seems to be malformed (semicolon and double closing "}"). But it
> looks like the next commit fixes this. Maybe collapse them?

Ah, dang. I tried to fix that up in a rebase and naturally screwed it up. Will fix (and check it multiple times). I'm not sure there's a ton of value in keeping this tiny refactor in a different commit, so maybe I will just combine both patches.

Comment 10

[Mike Taylor [:miketaylr]]

Attachment: Send screenshot of visible window when reporting site issues.

Comment 11

[Mike Taylor [:miketaylr]]

Comment on attachment 8727516 [details] [diff] [review]
Send screenshot of visible window when reporting site issues.

Combined commits (which fixes the semi-colon brace carnage).

Comment 12

[Vlad Filippov (:vladikoff)]

Comment on attachment 8727473 [details] [diff] [review]
Part 2 - Send screenshot of visible window when reporting site issue. r=?

Review of attachment 8727473 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/WebcompatReporter.js
@@ +68,5 @@
>      });
>    },
>  
> +  getScreenshot: (tab) => {
> +    return new Promise((resolve, reject) => {

nit: reject is not used.

@@ +84,5 @@
> +        canvas.width = dpr * w;
> +        canvas.height = dpr * h;
> +        ctx.scale(dpr, dpr);
> +        ctx.drawWindow(win, x, y, w, h, '#ffffff');
> +        let screenshot = canvas.toDataURL();

nit: Do you need this `let` here, or can you just put this into resolve below?

@@ +89,5 @@
> +        resolve({url: tab.browser.currentURI.spec, data: screenshot});
> +      } catch (e) {
> +        // drawWindow can fail depending on memory or surface size. Rather than reject, we resolve
> +        // the URL so the user can continue to file an issue without a screenshot.
> +        Cu.reportError("Call to CanvasRenderingContext2d.drawWindow failed: " + e);

nit: this means something in the above `try` failed, not just `drawWindow`.

@@ +114,5 @@
>      Snackbars.show(message, Snackbars.LENGTH_LONG, options);
>    },
>  
> +  reportIssue: (tabData) => {
> +    return new Promise((resolve, reject) => {

nit: reject is not used.

Comment 13

[Mike Taylor [:miketaylr]]

Thanks Vlad, also just noticed the following:

> let isPrivateTab = PrivateBrowsingUtils.isBrowserPrivate(selectedTab.browser) ? true : false;

Seems like a useless ternary, isBrowserPrivate returns a boolean already.

Comment 14

[Mike Taylor [:miketaylr]]

Comment on attachment 8727516 [details] [diff] [review]
Send screenshot of visible window when reporting site issues.

Removing review request until I fix up Vlad's nits.

Comment 15

[Mike Taylor [:miketaylr]]

Attachment: Send screenshot of visible window when reporting site issues.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8727541 [details] [diff] [review]
Send screenshot of visible window when reporting site issues.

Review of attachment 8727541 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, this is neat.

::: mobile/android/chrome/content/WebcompatReporter.js
@@ +116,5 @@
>  
> +  reportIssue: (tabData) => {
> +    return new Promise((resolve) => {
> +      const WEBCOMPAT_ORIGIN = "https://webcompat.com";
> +      let selectedTab = BrowserApp.selectedTab;

It seems a bit odd that consumers are passing the selected tab's URL directly into this method, but then it still uses the selected tab itself for other things. Could you avoid passing along the URL altogether and just get it directly here? Or pass in the tab itself?

But this is already an issue with this code, so not a big deal.

@@ +126,5 @@
> +
> +          if (event.target.defaultView.location.origin === WEBCOMPAT_ORIGIN) {
> +            // Waive Xray vision so event.origin is not chrome://browser on the other side.
> +            let win = Cu.waiveXrays(event.target.defaultView);
> +            win.postMessage(tabData.data, WEBCOMPAT_ORIGIN);

I'm not familiar with waiveXrays, but I'll trust it does what you intend it to do :)

Comment 17

[Mike Taylor [:miketaylr]]

Thanks for review!

(In reply to :Margaret Leibovic from comment #16)
> Comment on attachment 8727541 [details] [diff] [review]
> ::: mobile/android/chrome/content/WebcompatReporter.js
> @@ +116,5 @@
> >  
> > +  reportIssue: (tabData) => {
> > +    return new Promise((resolve) => {
> > +      const WEBCOMPAT_ORIGIN = "https://webcompat.com";
> > +      let selectedTab = BrowserApp.selectedTab;
> 
> It seems a bit odd that consumers are passing the selected tab's URL
> directly into this method, but then it still uses the selected tab itself
> for other things. Could you avoid passing along the URL altogether and just
> get it directly here? Or pass in the tab itself?
> 
> But this is already an issue with this code, so not a big deal.

Good point! We should just grab the tab once and pass it around. 

(I'll change that in a new commit since it touches the reportDesktopModePrompt code too.)

> @@ +126,5 @@
> > +
> > +          if (event.target.defaultView.location.origin === WEBCOMPAT_ORIGIN) {
> > +            // Waive Xray vision so event.origin is not chrome://browser on the other side.
> > +            let win = Cu.waiveXrays(event.target.defaultView);
> > +            win.postMessage(tabData.data, WEBCOMPAT_ORIGIN);
> 
> I'm not familiar with waiveXrays, but I'll trust it does what you intend it
> to do :)

Famous last words...

Comment 18

[Mike Taylor [:miketaylr]]

Attachment: Send screenshot of visible window when reporting site issues. r=sebastian, margaret

Comment 19

[Mike Taylor [:miketaylr]]

Comment on attachment 8727631 [details] [diff] [review]
Send screenshot of visible window when reporting site issues. r=sebastian, margaret

Updating commit message to reflect Margaret's review and carrying forward r+.

Comment 20

[Mike Taylor [:miketaylr]]

Attachment: Consistently pass tab references when reporting site issues. r=?

Comment 21

[Mike Taylor [:miketaylr]]

Comment on attachment 8727633 [details] [diff] [review]
Consistently pass tab references when reporting site issues. r=?

Margaret, mind reviewing here?

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9164326fdfdc

Comment 22

[:Margaret Leibovic]

Comment on attachment 8727633 [details] [diff] [review]
Consistently pass tab references when reporting site issues. r=?

Review of attachment 8727633 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup.

Comment 23

[Mike Taylor [:miketaylr]]

Attachment: Consistently pass tab references when reporting site issues.

Comment 24

[Mike Taylor [:miketaylr]]

Thanks again Sebastian and Margaret for feedback and reviews.

(Updating commit msg and carrying forward r+ on the last patch)

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4f29173049f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac236f12205

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/e4f29173049f
https://hg.mozilla.org/mozilla-central/rev/6ac236f12205