Screenshots should return only the view port

RESOLVED FIXED in mozilla38

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: automatedtester, Assigned: parkouss)

Tracking

({pi-marionette-goal, pi-marionette-server})

unspecified
mozilla38
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [marionette=1.0], )

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

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

Updated

5 years ago
Whiteboard: [marionette=1.0]
Reporter

Comment 1

5 years ago
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.
Reporter

Comment 3

5 years ago
(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.
Reporter

Updated

4 years ago
Duplicate of this bug: 1113373
Assignee

Comment 5

4 years ago
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)
Reporter

Comment 6

4 years ago
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+
Assignee

Comment 7

4 years ago
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.
Assignee

Comment 8

4 years ago
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 ?
Assignee: nobody → j.parkouss
Attachment #8556968 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8557486 - Flags: review?(dburns)
Assignee

Comment 9

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

Comment 10

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

Comment 11

4 years ago
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.
Attachment #8557486 - Flags: review?(dburns)
Assignee

Comment 12

4 years ago
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...
Reporter

Comment 13

4 years ago
(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
Assignee

Comment 14

4 years ago
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 ?
Reporter

Comment 15

4 years ago
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
Assignee

Comment 16

4 years ago
First patch r+ by David.
Attachment #8557486 - Attachment is obsolete: true
Attachment #8558131 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/daf4500bdb55
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.