1136784 - Add screenshot to logshake produced data

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Developer Tools, defect)

Description

[Dale Harvey (:daleharvey)]

The screenshot can be very valuable when debugging user facing bugs, I think it would be a good addition to the logs

Comment 1

[:gerard-majax]

I'm not sure, we already have an easy way to take screenshots

Comment 2

[Dale Harvey (:daleharvey)]

> I'm not sure, we already have an easy way to take screenshots

We have a seperate way to add screenshots but the screenshot data is likely very valuable when the user is indicating that there is a bug that they would like to capture information about, why wouldnt we also automatically capture it?

If we dont and the user does want to capture a screenshot along with the logs it complicates the flow hugely.

Comment 3

[:gerard-majax]

(In reply to Dale Harvey (:daleharvey) from comment #2)
> > I'm not sure, we already have an easy way to take screenshots
> 
> We have a seperate way to add screenshots but the screenshot data is likely
> very valuable when the user is indicating that there is a bug that they
> would like to capture information about, why wouldnt we also automatically
> capture it?
> 
> If we dont and the user does want to capture a screenshot along with the
> logs it complicates the flow hugely.

Ok, but then we will have to add it to the gecko logshake component

Comment 4

[:gerard-majax]

You'll have to hack around https://dxr.mozilla.org/mozilla-central/source/b2g/components/LogShake.jsm#212

Comment 5

[Dale Harvey (:daleharvey)]

Will do, thanks

Comment 6

[:gerard-majax]

Attachment: screenshot-logshake.patch

Comment 7

[:gerard-majax]

Attachment: screenshot-logshake.patch

Comment 8

[:gerard-majax]

Comment on attachment 8574411 [details] [diff] [review]
screenshot-logshake.patch

I had to move screenshotting from shell.js and put it into SystemAppProxy. But we still need a document and a window. Here is a patch that does the job, but I'm not sure if there is not a cleaner way to do it.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8574411 [details] [diff] [review]
screenshot-logshake.patch

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

::: b2g/chrome/content/shell.js
@@ +602,4 @@
>  
>      this.reportCrash(true);
>  
> +    SystemAppProxy.registerFrame(shell.contentBrowser, document, window);

No need to pass document/window, we can query all that from contentBrowser (that is an iframe).

::: b2g/components/SystemAppProxy.jsm
@@ +139,5 @@
>      }
>      return list;
> +  },
> +
> +  getScreenshot: function systemApp_getScreenshot() {

That has nothing to do in SystemAppProxy. This module is just an helper to track the system app frame. Better make it expose SystemAppProxy._frame publicly, than doing this!
So that from somewhere else (LogShake.jsm?), you would get the system app frame like this:
  let systemAppFrame = SystemAppProxy.frame;
  let window = systemAppFrame.contentWindow;
  let document = window.document;

Comment 10

[:gerard-majax]

Thanks, that was exactly what I feared :)

Comment 11

[:gerard-majax]

Attachment: Add screenshot to LogShake produced data r=...

Comment 12

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Created attachment 8574653 [details] [diff] [review]
> Add screenshot to LogShake produced data r=...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7bbcd85cb1f

Comment 13

[:gerard-majax]

Comment on attachment 8574653 [details] [diff] [review]
Add screenshot to LogShake produced data r=...

That works for me like this.

Comment 14

[:gerard-majax]

Attachment: Add screenshot to LogShake produced data r=...

Comment 15

[:gerard-majax]

With mochitests as suggested on IRC: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf9d7e8dde3

Comment 16

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #15)
> With mochitests as suggested on IRC:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf9d7e8dde3

typo

Comment 17

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8db2f557f6ef

Comment 18

[:gerard-majax]

Attachment: Add screenshot to LogShake produced data r=...

Comment 19

[:gerard-majax]

Attachment: Add screenshot to LogShake produced data r=...

Comment 20

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dda57bb3233

Comment 21

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #20)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dda57bb3233

test_screenshot.html and test_systemapp.html are both ok.

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8575115 [details] [diff] [review]
Add screenshot to LogShake produced data r=...

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

r+ for SystemAppProxy.jsm and systemapp_helper.js
I'm not reviewer for other files.

Comment 23

[Gregor Wagner [:gwagner]]

Comment on attachment 8575115 [details] [diff] [review]
Add screenshot to LogShake produced data r=...

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

::: b2g/components/test/mochitest/test_screenshot.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=963239

Please update.

Comment 24

[:gerard-majax]

Attachment: Add screenshot to LogShake produced data r=...

Comment 25

[:gerard-majax]

Comment on attachment 8575849 [details] [diff] [review]
Add screenshot to LogShake produced data r=...

Carrying r+, addressed comments.

Comment 26

[:gerard-majax]

Green try in comment 20

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6f6ff7993a81

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/6f6ff7993a81