Closed Bug 1136784 Opened 5 years ago Closed 5 years ago

Add screenshot to logshake produced data

Categories

(Firefox OS Graveyard :: Developer Tools, defect)

x86
macOS
defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
2.2 S8 (20mar)
Tracking Status
firefox39 --- fixed

People

(Reporter: daleharvey, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 6 obsolete files)

The screenshot can be very valuable when debugging user facing bugs, I think it would be a good addition to the logs
I'm not sure, we already have an easy way to take screenshots
> 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.
(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
Will do, thanks
Flags: needinfo?(dale)
Attached patch screenshot-logshake.patch (obsolete) — Splinter Review
Attachment #8574410 - Flags: feedback?(poirot.alex)
Attached patch screenshot-logshake.patch (obsolete) — Splinter Review
Assignee: nobody → lissyx+mozillians
Attachment #8574410 - Attachment is obsolete: true
Attachment #8574410 - Flags: feedback?(poirot.alex)
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.
Attachment #8574411 - Flags: feedback?(poirot.alex)
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;
Attachment #8574411 - Flags: feedback?(poirot.alex) → feedback-
Thanks, that was exactly what I feared :)
Attachment #8574411 - Attachment is obsolete: true
(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 on attachment 8574653 [details] [diff] [review]
Add screenshot to LogShake produced data r=...

That works for me like this.
Attachment #8574653 - Flags: feedback?(poirot.alex)
Attachment #8574653 - Flags: feedback?(dale)
Attachment #8574653 - Flags: feedback?(poirot.alex) → feedback+
(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
Attachment #8574653 - Attachment is obsolete: true
Attachment #8574653 - Flags: feedback?(dale)
Attachment #8574762 - Attachment is obsolete: true
Attachment #8574861 - Flags: review?(poirot.alex)
Attachment #8574861 - Flags: review?(anygregor)
Attachment #8574861 - Attachment is obsolete: true
Attachment #8574861 - Flags: review?(poirot.alex)
Attachment #8574861 - Flags: review?(anygregor)
(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.
Attachment #8575115 - Flags: review?(poirot.alex)
Attachment #8575115 - Flags: review?(anygregor)
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.
Attachment #8575115 - Flags: review?(poirot.alex) → review+
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.
Attachment #8575115 - Flags: review?(anygregor) → review+
Attachment #8575115 - Attachment is obsolete: true
Comment on attachment 8575849 [details] [diff] [review]
Add screenshot to LogShake produced data r=...

Carrying r+, addressed comments.
Attachment #8575849 - Flags: review+
Green try in comment 20
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6f6ff7993a81
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
Whiteboard: [systemsfe]
Blocks: 1151963
No longer blocks: 1151963
You need to log in before you can comment on or make changes to this bug.