Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Description • 9 years ago

The Marionette class currently has a screenshot method that just returns the resulting PNG image data. This method should optionally save the data to a specific file path.

	Andreas Tolfsen ❲:ato❳
	Comment 1 • 6 years ago

I was going to close this as WONTFIX, but perhaps having a method
that lets you write to a file handle would be a good idea so that
the following would be possible:

> with open("file.png", "w") as fh:
>     marionette.take_screenshot(fh)

	Manish [:manishkk]
	Comment 2 • 6 years ago

I would like to work on this.

How I can start please let me know more.

Thanks

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 6 years ago

I feel the Marionette.screenshot API [1] is pretty overloaded as it is:

>     def screenshot(self, element=None, highlights=None, format="base64",
>                    full=True, scroll=True):

My personal preference would be to add a new method on the class,
for example:

> Marionette.save_screenshot(self, fh, element=None, highlights=None,
>                            full=True, scroll=True)

This would call Marionette.screenshot internally but instead of
returning the response body, it would write it to the file handle
(fh).

I’ll let maja_zf decide whether this is a good idea.

  [1] https://searchfox.org/mozilla-central/rev/0e8eb01368600eb552dd558c83c64a3b6a0b89b8/testing/marionette/client/marionette_driver/marionette.py#1950-1951

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me) Reporter
	Comment 4 • 6 years ago

Sounds good to me.

	Vangelis Katsikaros Assignee
	Comment 5 • 6 years ago

I couldn't push to try as I temporarily don't have access to my commit access key, sorry for this. Also, this is the fist time I use git (git-cinnabar) so I might have not formatted the patch properly?

Happy to add a test as well if this looks ok.

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 6 years ago

Comment on attachment 8999366 [details] [diff] [review]
0001-Bug-1213008-Add-option-to-save-screenshot-to-a-file.patch

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

This looks OK to me, but I would like to see some tests for it.  Do
you think you could manage that?

When uploading patches to Bugzilla using git, you might be interested
in git-bz from https://github.com/mozilla/moz-git-tools.  I use it
this way:

> % git bz attach -e central..HEAD

(central is my branch point and “master” branch, but generally it
just takes a revset range.)

	Vangelis Katsikaros Assignee
	Comment 7 • 6 years ago

Thanks for the input Andreas! Yes, I'll give a go at test(s).

I would like to ask some questions:
1) I expect the test that needs expansion is testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py ?
2) I plan to compare a) the content of a file saved with 'save_screenshot()' with the data from 'screenshot(format="binary")'. How does that sound?
3) If the above sounds ok, is there some functionality in harness around creating temporary files? 
4) I see that most tests (ie "def test_*" in testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py) do "something" in the beginning, such as:
> dialog = self.open_dialog()
or
> self.marionette.navigate(long)
should I be interested in that?

	Andreas Tolfsen ❲:ato❳
	Comment 8 • 6 years ago

(In reply to Vangelis Katsikaros from comment #7)

> 1) I expect the test that needs expansion is
> testing/marionette/harness/marionette_harness/tests/unit/test_scre
> enshot.py ?

That seems like a reasonable place to put it.

> 2) I plan to compare a) the content of a file
> saved with 'save_screenshot()' with the data from
> 'screenshot(format="binary")'. How does that sound?

That sounds perfect.  All you need to do is ensure there is data
parity, as the particularities of take_screenshot is already tested.

> 3) If the above sounds ok, is there some functionality in harness
> around creating temporary files?

As the Marionette unit tests are written with unittest
(sigh) you can use tempfile from the standard library:
https://docs.python.org/2/library/tempfile.html

For example:

> with tempfile.TemporaryFile() as fh:
>     fh.write(…)

> 4) I see that most tests (ie "def test_*" in
> testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py)
> do "something" in the beginning, such as:
> 
> > dialog = self.open_dialog()
> 
> or
> 
> > self.marionette.navigate(long)
> 
> should I be interested in that?

The tests that do this are testing that it’s possible to take
screenshots of chrome dialogues in Firefox.  Since this is already
tested by the various take_screenshot tests, you don’t need to
explicitly test for that.

	Vangelis Katsikaros Assignee
	Comment 9 • 6 years ago

Thanks for the tips Andreas. I still haven't got access to my bugzilla related keys, so I provide a patch and can't push to try.

These commands are successful:
> ./mach test testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py
> ./mach lint testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py \
> testing/marionette/client/marionette_driver/marionette.py

	Andreas Tolfsen ❲:ato❳
	Comment 10 • 6 years ago

Comment on attachment 9003708 [details] [diff] [review]
0001-Bug-1213008-Add-option-to-save-screenshot-to-a-file.patch

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

Look OK to me, pending a successful try run.  It appears all the
test infrastructure is down today, so I’ll push a try run later.

	Andreas Tolfsen ❲:ato❳
	Comment 11 • 6 years ago

Reminder for myself to push to try when it opens.

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 6 years ago

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

	Pulsebot
	Comment 13 • 6 years ago

Pushed by ato@sny.no:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cae8cdc1e24a
Add Marionette client option to save screenshot to a file. r=ato

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 6 years ago

Sorry it took me so long to get around to pushing this.  I’ve been
having various email related problem over the last two days.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 15 • 6 years ago
bugherder

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