996236 - screenshot() should allow decoding from base64 automatically

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[William Lachance (:wlach)]

Right now marionette's screenshot() member returns a screenshot as a base64 string:

http://marionette-client.readthedocs.org/en/latest/#marionette.Marionette.screenshot

This is the right thing for some applications, but it would be nice to be able to automatically decode it to a binary png if (e.g.) saving the screenshot to disk for further inspection.

I propose changing the API to include a keyword argument called "format" which can take one of two possible values: "base64" (the default) and "binary".

If it is "base64", return the screenshot as a base64-string as we do now. If "binary", convert to binary as follows first using something like this:

base64.b64decode(screenshot_data.encode('ascii'))

Documentation, tests, etc should also be updated.

Pointer to source file: http://hg.mozilla.org/mozilla-central/file/83ae54e18689/testing/marionette/client/marionette/marionette.py#l1378
Pointer to tests: http://hg.mozilla.org/mozilla-central/file/83ae54e18689/testing/marionette/client/marionette/tests/unit/test_screenshot.py
Instructions on setting up marionette for development: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Developer_setup

Comment 1

[Julien Pagès (:parkouss)]

Attachment: add format parameter to screenshot method, allowing to decode b64 automatically

I have added tests for this feature.
The doc must be automatically updated since the screenshot() method was
already taken in account in the doc/index.rst file. I just updated docstring and I think it will do the job.

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8420067 [details] [diff] [review]
add format parameter to screenshot method, allowing to decode b64 automatically

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

Awesome. I have some minor requests for updates. Could you make these changes and upload a new patch?

::: testing/marionette/client/marionette/marionette.py
@@ +1399,5 @@
>              box around in the returned screenshot.
> +        
> +        :param format: if "base64" (the default), returns the screenshot
> +            as a base64-string. If "binary", the data is decoded and
> +            returned as a raw binary string.

a raw binary string -> raw binary

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py
@@ +27,5 @@
> +        test_url = self.marionette.absolute_url('html5Page.html')
> +        self.marionette.navigate(test_url)
> +        el = self.marionette.find_element('id', 'red')
> +        binary_data = self.marionette.screenshot(element=el, format="binary")
> +        self.assertEqual('iVBORw0KGgoAAAANSUhEUgAAADIAAAAyCAYAAAAeP4ixAAAAVUlEQVRoge3PsQ0AIAzAsI78fzBwBhHykD2ePev80LweAAGJB1ILpBZILZBaILVAaoHUAqkFUgukFkgtkFogtUBqgdQCqQVSC6QWSC2QWiC1QGp9A7ma+7nyXgOpzQAAAABJRU5ErkJggg==',

Could you refactor this test a bit so we define the base64 data as constants in the top of the file?

e.g.

RED_ELEMENT_BASE64='iVBORw0KGgoAAAANSUhEUgAAAD...'
GREEN_ELEMNT_BASE64='iVBORw0KGgoAAAANSUhEUgAAAD...'

I think it would make things easier to read.

@@ +34,5 @@
> +    def testNotAllowedScreenshotFormatRaiseValueError(self):
> +        test_url = self.marionette.absolute_url('html5Page.html')
> +        self.marionette.navigate(test_url)
> +        el = self.marionette.find_element('id', 'red')
> +        try:

I think I'd prefer a simple assertRaises here. I don't think we need to care about the exact string in the exception.

http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_execute_async_script.py#22

Comment 3

[Julien Pagès (:parkouss)]

Attachment: 2: add format parameter to screenshot method, allowing to decode b64 automatically

THanks for the feedback!

I have corrected following your instructions.

Comment 4

[William Lachance (:wlach)]

Comment on attachment 8420231 [details] [diff] [review]
2: add format parameter to screenshot method, allowing to decode b64 automatically

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

Thanks, you can actually carry forward the r+ in this case, since the changes were minor.

Comment 5

[William Lachance (:wlach)]

Let's run this through try, just to be safe: https://tbpl.mozilla.org/?tree=Try&rev=3e2b64d4e977

Comment 6

[William Lachance (:wlach)]

Attachment: Updated patch with author/commit info

Comment 7

[William Lachance (:wlach)]

Try run looks good, let's check this in!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8e1ed6e710

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba8e1ed6e710