Closed Bug 1136784 Opened 10 years ago Closed 10 years ago

Add screenshot to logshake produced data

Categories

(Firefox OS Graveyard :: Developer Tools, defect)

Product:
Firefox OS Graveyard
Component:
Developer Tools
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 fixed)

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

People

(Reporter: daleharvey, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 6 obsolete files)

Add screenshot to LogShake produced data r=...
(deleted), patch
gerard-majax
: review+
Details | Diff | Splinter Review
The screenshot can be very valuable when debugging user facing bugs, I think it would be a good addition to the logs
Blocks: 1134701
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) (deleted) — — Splinter Review
Attachment #8574410 - Flags: feedback?(poirot.alex)
Attached patch screenshot-logshake.patch (obsolete) (deleted) — — 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: 10 years ago
status-firefox39: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: