1112913 - Screenshots should return only the view port

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[David Burns :automatedtester]

In Santa Clara it was decided that scnreeshots only return the viewport. We need to update our implementation

Comment 1

[David Burns :automatedtester]

just for clarification, we will still return full screenshots to those who are expecting it so the Marionette client needs to send through full screenshot argument which the server will fulfill

Comment 2

[Andreas Tolfsen ❲:ato❳]

An argument would conflict with the specification though, surely?  I suggested at the F2F to expose it using the extension technique described in the spec.

Comment 3

[David Burns :automatedtester]

(In reply to Andreas Tolfsen (:ato) from comment #2)
> An argument would conflict with the specification though, surely?  I
> suggested at the F2F to expose it using the extension technique described in
> the spec.

no it won't because I don't expect internal teams to be using the httpd. The extension technique is only for extending items through the httpd.

Comment 5

[Julien Pagès (:parkouss)]

Attachment: Screenshots should return only the view port

I looked at it and tried something with the following patch. It works well locally with a firefox binary. If you could tell me if I'm on the right way, what's missing and so on, I may be able to handle this bug. :)

Comment 6

[David Burns :automatedtester]

Comment on attachment 8556968 [details] [diff] [review]
Screenshots should return only the view port

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

looks good enough to ship. If you can create a test that works cross platform that would be great for this area but I know how difficult tests for this can be so don't worry.

Comment 7

[Julien Pagès (:parkouss)]

Cool :)

Yeah, I thought about a test, but I agree it may be quite complex. Well I will think a bit about it more and come back here in a few days.

Comment 8

[Julien Pagès (:parkouss)]

Attachment: Screenshots should return only the view port

So I started to write a test for this. I did this by using the html page "element_outside_viewport.html". It seemed great because it already draw things outside the viewport. It works well locally.

Some things to note:

 - I tried to put the expected data inside the test script, but this is really big. So I added the expected png file next to the test script instead, I hope it's OK.
 - While I was at it, I tried to do a full screenshot of the same page. It does not work, empty string is returned as binary data. I tried without the patch, same result. Maybe it is a bug ? I did not created a bug entry, as I'm not sure, but I can do this if you want.

I have a last question. :) Can you give me the common try command you use (if any) when you want to test a marionette patch on multiple platforms ?

Comment 9

[Julien Pagès (:parkouss)]

Oh, I am thinking that my test case may not work cross platform, depending on the screen / browser size.

Comment 10

[David Burns :automatedtester]

can you push this to try and see how try handles this. I think that the sheriffs will be upset if it is flaky

Comment 11

[Julien Pagès (:parkouss)]

Comment on attachment 8557486 [details] [diff] [review]
Screenshots should return only the view port

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60cfa7b625e3

It seems that it does not works - I suspect that this is because of browser resolution. I will push a new try soon to see if that's correct.

Comment 12

[Julien Pagès (:parkouss)]

Still no luck: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b55c28b679f6

I don't know why it is not working - I added a self.marionette.set_window_size(640, 480) call in tests, thinking it was due to browser size, but I don't know anymore - If you have more ideas...

Comment 13

[David Burns :automatedtester]

(In reply to Julien Pagès from comment #12)
> Still no luck:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b55c28b679f6
> 
> I don't know why it is not working - I added a
> self.marionette.set_window_size(640, 480) call in tests, thinking it was due
> to browser size, but I don't know anymore - If you have more ideas...

It's not really worth the test at the moment then until we can have a mechanism that tests cross platform that we trust

Comment 14

[Julien Pagès (:parkouss)]

Ok - so Attachment 8556968 [details] [diff] is good enough for now I suppose. Do I need to send it again ? Or add the checkin-needed flag ?

Comment 15

[David Burns :automatedtester]

put the patch from comment 6 up and carry the r+ forward on that and then checkin-needed.

Thanks for all the hard work that you have put in! <3

Comment 16

[Julien Pagès (:parkouss)]

Attachment: Screenshots should return only the view port

First patch r+ by David.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Carsten Book [:Tomcat]]

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