In Santa Clara it was decided that scnreeshots only return the viewport. We need to update our implementation
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
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.
(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.
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. :)
Attachment #8556968 - Flags: feedback?(dburns)
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.
Attachment #8556968 - Flags: feedback?(dburns) → review+
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.
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 ?
Oh, I am thinking that my test case may not work cross platform, depending on the screen / browser size.
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 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.
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...
(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
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 ?
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
First patch r+ by David.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.