818287 - Screenshots include data URL prefix

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The screenshots method states that it returns a base64 encoding string representation of the element/canvas, however it actually returns a data URL. This differs from WebDriver, and means we have to strip the leading characters when decoding the image.

By skipping the first 22 characters I'm able to use base64.decodestring in Python:

The data URL prefix:
data:image/png;base64,

We should either correct this to return a base64 encoded string, which can be prefixed for using as a data URL, or adjust the comment to indicate that a data URL will be returned.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Same we had with Mozmill. Please see bug 783223 how to do it.

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> Same we had with Mozmill. Please see bug 783223 how to do it.

How to do what? I already have working code as mentioned in comment 0. The fix here is either to return a base64 string as the documentation suggests, or adjust the documentation.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See the referenced bug and the NetUtil.asyncFetch() call which correctly transforms the dataURL to a stream you need.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This bug is concerning the Python client. Sorry for not being clear.

Comment 5

[Jonathan Griffin (:jgriffin)]

We should do whatever standard webdriver implementations do, which I'd guess is omit the data url prefix.  AutomatedTester?

Comment 6

[David Burns :automatedtester]

(In reply to Jonathan Griffin (:jgriffin) from comment #5)
> We should do whatever standard webdriver implementations do, which I'd guess
> is omit the data url prefix.  AutomatedTester?

Yea, I had originally left it in there so its useful for tbpl but we should follow spec and easily add in the prefix in the client code if need be

Comment 7

[sud (:sudshekhar)]

Hi, I am new to the mozilla codebase and would like to work on this bug. I have worked in python previously but have no idea how to go about this. Can someone please help? 
Thank you.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Hey sud! Sorry for the delay, I just noticed your comment. What needs to happen here is the data URL prefix needs to be removed from the screenshots returned by Marionette.

The relevant code for Marionette is here: http://hg.mozilla.org/mozilla-central/file/065727546e13/testing/marionette/marionette-listener.js#l1851

Along with this change, we'll need to update gaiatest, which currently strips the data URL prefix. The code for this is here: https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/runtests.py#L60

We'll need to submit the change to gaia-ui-tests at the same time as the server-side change to avoid test failures.

Let me know if you have any questions. You can also find me on IRC in #automation or #ateam.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I just noticed your mention of Python, however you'll find the server side is actually JavaScript. Let me know if this is an issue, otherwise I'm happy to mentor you through it! :)

Comment 10

[sud (:sudshekhar)]

Hey Dave,

I thought the bug was in python. But I have worked in JavaScript before and would be happy to help. Will catch you on the irc then. :)

Comment 11

[sud (:sudshekhar)]

Hi,
I went through the code and realized that simply doing :

var base64str = canvas.toDataURL("image/png","")[22:]
sendResponse({value: base64str}, msg.json.command_id);

would be enough as the gaia tests add the prefix when they check the Data/Url.

Please confirm whether this is all that's required or have I misunderstood something?

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Rather than hard-code the number of characters, could we split on the first occurrence of comma? Otherwise, your approach looks fine to me.

Comment 13

[sud (:sudshekhar)]

Hey Dave,
Sorry for the delay, I was out of town for a while.

I believe the following lines:

var str = canvas.toDataURL("image/png","");
var URLlength =str.indexOf(",");
var base64str = str.substring(n+1);
sendResponse({value: base64str}, msg.json.command_id);


Finish your problem. Please confirm this once. Also, please guide me on how to proceed from here. I tried to catch you on the irc but you weren't active at that time.

Comment 14

[sud (:sudshekhar)]

Sorry that was 

base64str = str.substring(URLlength+1); 

in the code above.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This approach looks good to me. I would use a more descriptive name than just 'str' though, perhaps 'data_url'. Also, you don't need to store URLlength or base64str as they're only used once, so you can just use:

var data_url = canvas.toDataURL("image/png","");
sendResponse({value: data_url.substring(data_url.indexOf(",") + 1)}, msg.json.command_id);

If you're ready to work on a patch, you will need to clone the mozilla-central repository, make your changes, and attach a diff to this bug. Follow the guide here, and ping me if you get stuck: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ

Note that you will also need to update the tests so they're no longer expecting the data url prefix. You can find these here: http://hg.mozilla.org/mozilla-central/file/default/testing/marionette/client/marionette/tests/unit/test_screenshot.py

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Hey sud, are you still interesting on fixing this? If not, we can free it up for someone else to look at.

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Assigning this to :kyr0 as he's expressed interest in it, and there's been no response from sud. If you're still interested, sud, I'm sure we can find something else for you to work on.

Comment 18

[Sebastiaan de Haan [:kyr0]]

Attachment: Stage 1 Add condition to facilitate pre-update marionette.

Stage 2 will be the marionette update.
Stage 3 will be the removal of this condition

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 825896 [details] [review]
Stage 1 Add condition to facilitate pre-update marionette.

Landed as:
https://github.com/mozilla-b2g/gaia/commit/4defd9c814dbeaad426b1d9d05cba1192c9710ce (master)
https://github.com/mozilla-b2g/gaia/commit/d654c10a7acaec240b04b373e7232142fbc7bb9a (v1.2)

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/3ea1f8e46b87

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

There are still a couple more patches to land before this can be considered resolved.

Comment 22

[Sebastiaan de Haan [:kyr0]]

Attachment: 818287.patch

Stage 2: The change to the return of the screenshot function in marionette.

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 826755 [details] [diff] [review]
818287.patch

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

The change looks good but we need to update the tests too. See comment 15 for the location of these.

::: testing/marionette/marionette-listener.js
@@ +1934,5 @@
>        ctx.strokeRect(rect.left + offsetX, rect.top + offsetY, rect.width, rect.height);
>      }
>    }
>  
> +  // Return the data URL String back to the client bindings and they can manage

We're actually only now passing back the Base64 string. Previously we were passing back the whole data URL.

Comment 24

[Sebastiaan de Haan [:kyr0]]

Attachment: Corrected the patch with regards to comment 15.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 828626 [details] [diff] [review]
Corrected the patch with regards to comment 15.

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

Just a minor nit with the comment.

::: testing/marionette/marionette-listener.js
@@ +1934,5 @@
>        ctx.strokeRect(rect.left + offsetX, rect.top + offsetY, rect.width, rect.height);
>      }
>    }
>  
> +  // Return the data URL String back to the client bindings and they can manage

As mentioned in previous review, please revert the change to the comment.

Comment 26

[Sebastiaan de Haan [:kyr0]]

Attachment: nit addressed.

Comment 27

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 829311 [details] [diff] [review]
nit addressed.

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

Looks great. Triggered a try run:
https://tbpl.mozilla.org/?tree=Try&rev=244d344c2aa8

Comment 28

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fbb4e40ee25

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not landed on mozilla-central yet. Reopening until that is the case.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fbb4e40ee25

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Hopefully you meant b2g26.

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6d2b68960a78