headless: Implement screenshot capturing

headless: Implement screenshot capturing

(based on patch from skyostil@, also sets default window size to 800x600 to enable basic snapshot support)

With the --screenshot option, headless shell will save a PNG screenshot
of the loaded page.

patch from issue 2000723002 at patchset 1 (http://crrev.com/2000723002#ps1)

BUG=546953
Committed: https://crrev.com/9ef54766bc24bdeff0fec0a8df5540feb691cfc9
Cr-Commit-Position: refs/heads/master@{#397743}

[Eric Seckler, 2016-06-02 14:21:18]

Description was changed from

==========
headless: Implement screenshot capturing

(based on patch from skyostil@, also sets default window size to 800x600 to
enable basic snapshot support)

With the --screenshot option, headless shell will save a PNG screenshot
of the loaded page.

patch from issue 2000723002 at patchset 1 (http://crrev.com/2000723002#ps1)

BUG=
==========

to

==========
headless: Implement screenshot capturing

(based on patch from skyostil@, also sets default window size to 800x600 to
enable basic snapshot support)

With the --screenshot option, headless shell will save a PNG screenshot
of the loaded page.

patch from issue 2000723002 at patchset 1 (http://crrev.com/2000723002#ps1)
==========

[Eric Seckler, 2016-06-02 14:21:18]

eseckler@chromium.org changed reviewers:
+ alexclarke@chromium.org, altimin@chromium.org, skyostil@chromium.org

[Sami, 2016-06-02 14:22:19]

skyostil@chromium.org changed reviewers:
- alexclarke@chromium.org, altimin@chromium.org

[Sami, 2016-06-02 14:22:19]

Thanks for tracking this down. Would you mind adding a HeadlessWebContentsTest
that keeps this working?

https://codereview.chromium.org/2035733002/diff/1/headless/app/headless_shell.cc
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/2035733002/diff/1/headless/app/headless_shell...
headless/app/headless_shell.cc:199: std::cout << result_json << std::endl;
Could you make this actually write out the png instead dumping the base64?

https://codereview.chromium.org/2035733002/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2035733002/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_browser_impl.cc:7: #include <vector>
unused?

[Eric Seckler, 2016-06-02 17:56:32]

Thanks, Sami :)

Here's a first attempt at the test. I'll look into writing decoded data to a
file tomorrow.

https://codereview.chromium.org/2035733002/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2035733002/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_browser_impl.cc:7: #include <vector>
On 2016/06/02 14:22:19, Sami wrote:
> unused?

I added this b/c GetAllWebContents() uses std::vector and my cpplint complained
:P

I can get rid of it and look at all the linting issues in a separate commit if
that's more sensible.

https://codereview.chromium.org/2035733002/diff/20001/headless/lib/headless_w...
File headless/lib/headless_web_contents_browsertest.cc (right):

https://codereview.chromium.org/2035733002/diff/20001/headless/lib/headless_w...
headless/lib/headless_web_contents_browsertest.cc:28: class
HeadlessAsyncDevTooledBrowserTest : public HeadlessBrowserTest,
I'm following the same pattern as the HeadlessDevToolsClientTest here. If this
is the way to go, I think we might want to move this class (+ the define above)
to headless_browser_test.h/cc and eventually refactor the DevToolsClientTest to
use it as well.

I'm adding this here for now and will move it if you confirm that this is the
correct general direction :)

[Sami, 2016-06-02 18:01:33]

Looks great. I agree that we should factor out the async test base to a common
helper class. Having this duplicate intermediate copy is fine as long as we
clean it up.

https://codereview.chromium.org/2035733002/diff/1/headless/lib/browser/headle...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2035733002/diff/1/headless/lib/browser/headle...
headless/lib/browser/headless_browser_impl.cc:7: #include <vector>
On 2016/06/02 17:56:31, Eric Seckler wrote:
> On 2016/06/02 14:22:19, Sami wrote:
> > unused?
> 
> I added this b/c GetAllWebContents() uses std::vector and my cpplint
complained
> :P
> 
> I can get rid of it and look at all the linting issues in a separate commit if
> that's more sensible.

Oh I see. Fine to fix it here then.

https://codereview.chromium.org/2035733002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2035733002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:92: // TODO(eseckler): allow
configuration of window (viewport) size by embedder
nit: end comments with a period.

[Eric Seckler, 2016-06-03 11:30:40]

Thanks, I moved the async test to a reusable place. We can remove the
DevToolsClientTest in a follow-up commit.

I'm now writing the decoded screenshot to a file, as discussed. Sadly, using the
FileStream and it's async callbacks is a bit cumbersome. I also didn't see a way
to avoid a memcpy between Base64Decode() and the IOBuffer the stream is
expecting. Maybe there's a different decode function for base64 that doesn't
require a string as output?

https://codereview.chromium.org/2035733002/diff/1/headless/app/headless_shell.cc
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/2035733002/diff/1/headless/app/headless_shell...
headless/app/headless_shell.cc:199: std::cout << result_json << std::endl;
On 2016/06/02 14:22:19, Sami wrote:
> Could you make this actually write out the png instead dumping the base64?

Done.

https://codereview.chromium.org/2035733002/diff/20001/headless/lib/browser/he...
File headless/lib/browser/headless_browser_impl.cc (right):

https://codereview.chromium.org/2035733002/diff/20001/headless/lib/browser/he...
headless/lib/browser/headless_browser_impl.cc:92: // TODO(eseckler): allow
configuration of window (viewport) size by embedder
On 2016/06/02 18:01:33, Sami wrote:
> nit: end comments with a period.

Done.

https://codereview.chromium.org/2035733002/diff/20001/headless/lib/headless_w...
File headless/lib/headless_web_contents_browsertest.cc (right):

https://codereview.chromium.org/2035733002/diff/20001/headless/lib/headless_w...
headless/lib/headless_web_contents_browsertest.cc:28: class
HeadlessAsyncDevTooledBrowserTest : public HeadlessBrowserTest,
On 2016/06/02 17:56:32, Eric Seckler wrote:
> I'm following the same pattern as the HeadlessDevToolsClientTest here. If this
> is the way to go, I think we might want to move this class (+ the define
above)
> to headless_browser_test.h/cc and eventually refactor the DevToolsClientTest
to
> use it as well.
> 
> I'm adding this here for now and will move it if you confirm that this is the
> correct general direction :)

Moved.

[Sami, 2016-06-03 11:46:25]

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:54: screenshot_file_stream_(nullptr) {}
No need to initialize a unique_ptr. The web_contents_ fix is a good find though.

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:222: OnScreenshotFileOpened(std::move(result),
file_name, open_result);
Does this work since you already moved |result| a few lines ago? Might need pass
in a nullptr instead.

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:233: << net::ErrorToString(open_result);
nit: you could early-out here to reduce the indentation in the rest of the
function.

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:237: net::IOBufferWithSize* buf =
Who owns |buf|?

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:244: OnScreenshotFileWritten(file_name,
open_result);
write_result?

https://codereview.chromium.org/2035733002/diff/40001/headless/test/headless_...
File headless/test/headless_browser_test.cc (right):

https://codereview.chromium.org/2035733002/diff/40001/headless/test/headless_...
headless/test/headless_browser_test.cc:184: void
HeadlessAsyncDevTooledBrowserTest::DevToolsTargetReady() {
Is this correctly indented? (git cl format should fix this for you).

https://codereview.chromium.org/2035733002/diff/40001/headless/test/headless_...
File headless/test/headless_browser_test.h (right):

https://codereview.chromium.org/2035733002/diff/40001/headless/test/headless_...
headless/test/headless_browser_test.h:75: class
HeadlessAsyncDevTooledBrowserTest : public HeadlessBrowserTest,
Could you add a TODO in HeadlessDevToolsClientTest to move it over to this
class?

[Eric Seckler, 2016-06-03 13:40:38]

Thanks Sami, PTAL :)

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
File headless/app/headless_shell.cc (right):

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:54: screenshot_file_stream_(nullptr) {}
On 2016/06/03 at 11:46:24, Sami wrote:
> No need to initialize a unique_ptr. The web_contents_ fix is a good find
though.

Ack, removed :)

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:222: OnScreenshotFileOpened(std::move(result),
file_name, open_result);
On 2016/06/03 at 11:46:24, Sami wrote:
> Does this work since you already moved |result| a few lines ago? Might need
pass in a nullptr instead.

Passing a nullptr now. From the comments in FileStream, it seems that Open() and
Close() can only execute asynchronously, so this should be fine. Interestingly,
Write() could execute synchronously (if we trust the comment). The FileStream
implementation seems to always return ERR_IO_PENDING anyway.

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:233: << net::ErrorToString(open_result);
On 2016/06/03 at 11:46:24, Sami wrote:
> nit: you could early-out here to reduce the indentation in the rest of the
function.

Done.

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:237: net::IOBufferWithSize* buf =
On 2016/06/03 at 11:46:24, Sami wrote:
> Who owns |buf|?

IOBuffers are refcounted, but passed as plain pointers to the functions (see
comment for IOBuffer class). So effectively we transfer ownership to the
FileStream. I've added a scoped_refptr to make this a bit clearer (that seems to
be the way that this is handled elsewhere in chromium).

https://codereview.chromium.org/2035733002/diff/40001/headless/app/headless_s...
headless/app/headless_shell.cc:244: OnScreenshotFileWritten(file_name,
open_result);
On 2016/06/03 at 11:46:24, Sami wrote:
> write_result?

Good catch, done.

https://codereview.chromium.org/2035733002/diff/40001/headless/test/headless_...
File headless/test/headless_browser_test.cc (right):

https://codereview.chromium.org/2035733002/diff/40001/headless/test/headless_...
headless/test/headless_browser_test.cc:184: void
HeadlessAsyncDevTooledBrowserTest::DevToolsTargetReady() {
On 2016/06/03 at 11:46:24, Sami wrote:
> Is this correctly indented? (git cl format should fix this for you).

Done.

https://codereview.chromium.org/2035733002/diff/40001/headless/test/headless_...
File headless/test/headless_browser_test.h (right):

https://codereview.chromium.org/2035733002/diff/40001/headless/test/headless_...
headless/test/headless_browser_test.h:75: class
HeadlessAsyncDevTooledBrowserTest : public HeadlessBrowserTest,
On 2016/06/03 at 11:46:24, Sami wrote:
> Could you add a TODO in HeadlessDevToolsClientTest to move it over to this
class?

Done.

[Sami, 2016-06-03 15:14:42]

Great, lgtm!

[Sami, 2016-06-03 15:15:22]

By the way, I think we had a bug opened for this. Could you link it with a
BUG=... line in the description?

[Eric Seckler, 2016-06-03 15:28:24]

Description was changed from

==========
headless: Implement screenshot capturing

(based on patch from skyostil@, also sets default window size to 800x600 to
enable basic snapshot support)

With the --screenshot option, headless shell will save a PNG screenshot
of the loaded page.

patch from issue 2000723002 at patchset 1 (http://crrev.com/2000723002#ps1)
==========

to

==========
headless: Implement screenshot capturing

(based on patch from skyostil@, also sets default window size to 800x600 to
enable basic snapshot support)

With the --screenshot option, headless shell will save a PNG screenshot
of the loaded page.

patch from issue 2000723002 at patchset 1 (http://crrev.com/2000723002#ps1)

BUG=546953
==========

[Eric Seckler, 2016-06-03 16:07:39]

The CQ bit was checked by eseckler@chromium.org

[commit-bot: I haz the power, 2016-06-03 16:08:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 16:08:09]

This CL has an open dependency (Issue 2000723002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Eric Seckler, 2016-06-03 16:32:36]

The CQ bit was checked by eseckler@chromium.org

[commit-bot: I haz the power, 2016-06-03 16:33:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2035733002/60001

[commit-bot: I haz the power, 2016-06-03 17:28:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-03 17:30:42]

Description was changed from

==========
headless: Implement screenshot capturing

(based on patch from skyostil@, also sets default window size to 800x600 to
enable basic snapshot support)

With the --screenshot option, headless shell will save a PNG screenshot
of the loaded page.

patch from issue 2000723002 at patchset 1 (http://crrev.com/2000723002#ps1)

BUG=546953
==========

to

==========
headless: Implement screenshot capturing

(based on patch from skyostil@, also sets default window size to 800x600 to
enable basic snapshot support)

With the --screenshot option, headless shell will save a PNG screenshot
of the loaded page.

patch from issue 2000723002 at patchset 1 (http://crrev.com/2000723002#ps1)

BUG=546953

Committed: https://crrev.com/9ef54766bc24bdeff0fec0a8df5540feb691cfc9
Cr-Commit-Position: refs/heads/master@{#397743}
==========

[commit-bot: I haz the power, 2016-06-03 17:30:44]

Patchset 4 (id:??) landed as
https://crrev.com/9ef54766bc24bdeff0fec0a8df5540feb691cfc9
Cr-Commit-Position: refs/heads/master@{#397743}