Closed Bug 883294 Opened 11 years ago Closed 10 years ago

Add ability to take full viewport screenshots

Categories

(Remote Protocol :: Marionette, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

(Whiteboard: [affects=b2g])

Attachments

(1 file, 4 obsolete files)

The screenshots from Marionette appear to be the current frame cropped to the available viewport. Sometimes it can be helpful to see the full viewport (for example to see the wifi/cell signal status). This is what is saved when holding home+power [1].

[1] http://hg.mozilla.org/mozilla-central/file/be23c06aab96/b2g/chrome/content/shell.js#l1067
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
This is actually distinct from bug 874868, which is about getting an image representing the full canvas, so would include content beyond the viewport that the user would need to scroll to see. This bug is about getting a screenshot of exactly what the user currently sees.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Whiteboard: [screenshot]
Whiteboard: [screenshot] → [screenshot][affects=webqa]
Wondering if we can increase the priority of this bug, as it would considerably improve the image comparison tests that No-Jun Park is working on. He currently has to simulate the hardware events for taking a screenshot on the device, and then has to pull the files off the device. Also, the code for this already exists, I think we'd just need to determine the API and implement it.
Flags: needinfo?(dburns)
Due to the reorg unfortunately I can not set priority on this. I can say that the A-Team will prioritise code reviews over feature work so will be reviewed pretty quickly by the A-Team.

James can you discuss priority of this bug with your new folk?
Flags: needinfo?(dburns) → needinfo?(jlal)
Whiteboard: [screenshot][affects=webqa]
please please please please!!
Just to give an update... I spoke to mdas a few days ago we should do this but waiting to talk to Brain/Dave (tomorrow) for priority. Either way in the near future we will have this.

If there are strong usecases outside of immediate needs for the reftest like work going on we may bump this.
Flags: needinfo?(jlal)
[affects=b2g] the most because of b2g's use of iframes.
Whiteboard: [affects=b2g]
Assignee: nobody → zcampbell
Attached patch bug_883294.diff (obsolete) — Splinter Review
(just working on the Try now)
Attachment #8479005 - Flags: review?(mdas)
Attachment #8479005 - Flags: review?(dburns)
So we are missing the right canvas flags I believe see here http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/device.js#168 the really important one is DRAWWINDOW_USE_WIDGET_LAYERS; (see reftest code comments) basically we copy/paste this all over the place but we want to do a very similar thing to what is done in the reftest code base (which I believe those flags where invented for)
Comment on attachment 8479005 [details] [diff] [review]
bug_883294.diff

Doesn't work
Attachment #8479005 - Attachment is obsolete: true
Attachment #8479005 - Flags: review?(mdas)
Attachment #8479005 - Flags: review?(dburns)
I'm passing this on to anyone else who would prefer to work on it.
Assignee: zcampbell → nobody
Taking this as I have something working locally. I'll hopefully have a patch up later today.
Assignee: nobody → dave.hunt
Status: REOPENED → ASSIGNED
Here's a patch that gives us full viewport screenshots in the HTML report. I'd like to continue to see if I can fix this within Marionette's screenshot code, but if I can't this will at least improve our HTML reports in the short term.
Attachment #8483457 - Flags: review?(mdas)
Comment on attachment 8483457 [details] [diff] [review]
Take full viewport screenshots for the HTML report. v1.0

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

works great!
Attachment #8483457 - Flags: review?(mdas) → review+
(In reply to Malini Das [:mdas] from comment #15)
> Comment on attachment 8483457 [details] [diff] [review]
> Take full viewport screenshots for the HTML report. v1.0
> 
> Review of attachment 8483457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> works great!

So do you think this is a worthwhile stop-gap Malini? Ideally we'd update the screenshot code in Marionette to take full viewport screenshots. I suspect (but have not tested) that we just need to set the correct flags and ensure that the consumer switches to the chrome context.
Flags: needinfo?(mdas)
Good point, the original problem here is to create a screenshot using the Marionette API... I'd prefer to see a patch for this ability. Do you have the time to look into it? If not, I'm happy to have this in here until the screenshot code is added, but this patch shouldn't resolve this bug.
Flags: needinfo?(mdas)
I'm going to be on PTO from tomorrow, so unassigning so someone else can take this. What we have could land as is but not resolve the bug, but it would be great if we can fix this in Marionette - I just haven't had time to test a fix.
Assignee: dave.hunt → nobody
Status: ASSIGNED → NEW
Taking this. I have a working patch that just needs a little more testing.
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
This adds the ability to take full viewport screenshots, which requires that you first switch to CHROME context.
Attachment #8483457 - Attachment is obsolete: true
Attachment #8497633 - Flags: review?(mdas)
Attachment #8497633 - Flags: feedback?(dburns)
Comment on attachment 8497633 [details] [diff] [review]
Add ability to take full viewport screenshots. v1.0

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

r- only for test_screenshot.py. We need to recover into the right context if the test fails.

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py
@@ +5,5 @@
>  GREEN_ELEMENT_BASE64 = 'iVBORw0KGgoAAAANSUhEUgAAADIAAAAyCAYAAAAeP4ixAAAAV0lEQVRoge3PQRGAQAwAsWINvXgsNnI3+4iAzM7sDWZn9vneoxXRFNEU0RTRFNEU0RTRFNEU0RTRFNEU0RTRFNEU0RTRFNEU0RTRFNEU0RTRFNHcF7nBD/Ha5Ye4BbsYAAAAAElFTkSuQmCC'
>  
>  class ScreenshotTests(MarionetteTestCase):
>  
> +    def testWeCanTakeAScreenShotOfEntireViewport(self):

Can you move this to a separate test class? Reason being, if the test fails after you set the context to chrome, the subsequent tests will be in chrome context. You can also have a tearDown()/setUp() force this, but in other marionette test files, we have separate classes from chrome/content since they usually have independent setUp/tearDown functions

::: testing/marionette/marionette-server.js
@@ +2407,5 @@
> +          context.DRAWWINDOW_DRAW_VIEW |
> +          context.DRAWWINDOW_USE_WIDGET_LAYERS;
> +      } else {
> +        // For some reason using the same flags against desktop Firefox
> +        // the screenshot is distorted.

Did you file a bug about this? Can you add that Bug number in this comment?
Attachment #8497633 - Flags: review?(mdas) → review-
fyi, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1075006 about enabling element screenshots in chrome
(In reply to Malini Das [:mdas] from comment #21)
> Comment on attachment 8497633 [details] [diff] [review]
> Add ability to take full viewport screenshots. v1.0
> 
> Review of attachment 8497633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- only for test_screenshot.py. We need to recover into the right context if
> the test fails.
> 
> ::: testing/marionette/client/marionette/tests/unit/test_screenshot.py
> @@ +5,5 @@
> >  GREEN_ELEMENT_BASE64 = 'iVBORw0KGgoAAAANSUhEUgAAADIAAAAyCAYAAAAeP4ixAAAAV0lEQVRoge3PQRGAQAwAsWINvXgsNnI3+4iAzM7sDWZn9vneoxXRFNEU0RTRFNEU0RTRFNEU0RTRFNEU0RTRFNEU0RTRFNEU0RTRFNEU0RTRFNHcF7nBD/Ha5Ye4BbsYAAAAAElFTkSuQmCC'
> >  
> >  class ScreenshotTests(MarionetteTestCase):
> >  
> > +    def testWeCanTakeAScreenShotOfEntireViewport(self):
> 
> Can you move this to a separate test class? Reason being, if the test fails
> after you set the context to chrome, the subsequent tests will be in chrome
> context. You can also have a tearDown()/setUp() force this, but in other
> marionette test files, we have separate classes from chrome/content since
> they usually have independent setUp/tearDown functions

As discussed on IRC the MarionetteTestCase.setUp takes care of resetting the context to content. We agreed to keep the tests all in one file. I did noticed a couple of issues with the test though, which I've now fixed up.
 
> ::: testing/marionette/marionette-server.js
> @@ +2407,5 @@
> > +          context.DRAWWINDOW_DRAW_VIEW |
> > +          context.DRAWWINDOW_USE_WIDGET_LAYERS;
> > +      } else {
> > +        // For some reason using the same flags against desktop Firefox
> > +        // the screenshot is distorted.
> 
> Did you file a bug about this? Can you add that Bug number in this comment?

Not yet. I'd like to work on some simple steps to reproduce, once I have those I'll raise a bug and update this patch with a reference.
Attachment #8497633 - Attachment is obsolete: true
Attachment #8497633 - Flags: feedback?(dburns)
Attachment #8497661 - Flags: review?(mdas)
Attachment #8497661 - Flags: feedback?(dburns)
Comment on attachment 8497661 [details] [diff] [review]
Add ability to take full viewport screenshots. v1.1

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

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py
@@ +12,5 @@
> +        self.marionette.navigate(test_url)
> +        content = self.marionette.screenshot()
> +        self.marionette.set_context(self.marionette.CONTEXT_CHROME)
> +        chrome = self.marionette.screenshot()
> +        self.assertTrue('iVBORw0KGgo' in chrome)

could you add a comment explaining why we check this?
Attachment #8497661 - Flags: review?(mdas) → review+
Requesting review again due to the test changes.
Attachment #8497661 - Attachment is obsolete: true
Attachment #8497661 - Flags: feedback?(dburns)
Attachment #8497800 - Flags: review?(mdas)
Attachment #8497800 - Flags: feedback?(dburns)
Comment on attachment 8497800 [details] [diff] [review]
Add ability to take full viewport screenshots. v1.2

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

lgtm thanks!
Attachment #8497800 - Flags: review?(mdas) → review+
Comment on attachment 8497800 [details] [diff] [review]
Add ability to take full viewport screenshots. v1.2

Looks good to me
Attachment #8497800 - Flags: feedback?(dburns) → feedback+
https://hg.mozilla.org/mozilla-central/rev/87916d61ca99
Status: ASSIGNED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Shouldn't also the DRAWWINDOW_DRAW_VIEW flag be used to be able to test for painting bugs?
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl#23
(In reply to Martijn Wargers [:mwargers] (QA) from comment #31)
> Shouldn't also the DRAWWINDOW_DRAW_VIEW flag be used to be able to test for
> painting bugs?
> http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/canvas/
> nsIDOMCanvasRenderingContext2D.idl#23

That was set in this patch:
https://hg.mozilla.org/mozilla-central/rev/87916d61ca99#l3.67
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.