Pass False for fromSurface argument to Page.captureScreenshot.

Pass False for fromSurface argument to Page.captureScreenshot.

BUG=chromium:714058
Review-Url: https://codereview.chromium.org/2887023002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/dd9db5d3f658449a05fea103ae5f3c2bc329e6d5

[Ken Russell (switch to Gerrit), 2017-05-17 00:18:17]

kbr@chromium.org changed reviewers:
+ nednguyen@google.com

[Ken Russell (switch to Gerrit), 2017-05-17 00:18:18]

PTAL

I only lightly tested this.

[nednguyen, 2017-05-17 00:22:25]

On 2017/05/17 00:18:18, Ken Russell wrote:
> PTAL
> 
> I only lightly tested this.


What is the drawback of this? Would this fail to capture screenshot in some
case?

[Ken Russell (switch to Gerrit), 2017-05-17 07:01:53]

On 2017/05/17 00:22:25, nednguyen wrote:
> On 2017/05/17 00:18:18, Ken Russell wrote:
> > PTAL
> > 
> > I only lightly tested this.
> 
> 
> What is the drawback of this? Would this fail to capture screenshot in some
> case?

If for some reason someone launched Chrome via Telemetry with the --headless
option, this would break. But otherwise there's no drawback. It just ensures
that the current semantics of Page.captureScreenshot are preserved. The DevTools
team wants to change the default value of the fromSurface argument from false to
true; see http://crbug.com/714058 and
https://codereview.chromium.org/2871113002/ . The GPU tests must continue to
pass false for this argument in order to not lose test coverage.

[nednguyen, 2017-05-17 10:18:04]

On 2017/05/17 07:01:53, Ken Russell wrote:
> On 2017/05/17 00:22:25, nednguyen wrote:
> > On 2017/05/17 00:18:18, Ken Russell wrote:
> > > PTAL
> > > 
> > > I only lightly tested this.
> > 
> > 
> > What is the drawback of this? Would this fail to capture screenshot in some
> > case?
> 
> If for some reason someone launched Chrome via Telemetry with the --headless
> option, this would break. But otherwise there's no drawback. It just ensures
> that the current semantics of Page.captureScreenshot are preserved. The
DevTools
> team wants to change the default value of the fromSurface argument from false
to
> true; see http://crbug.com/714058 and
> https://codereview.chromium.org/2871113002/ . The GPU tests must continue to
> pass false for this argument in order to not lose test coverage.

Ravi is planning to use headless with Telemetry. I think this logic probably
should be change to:
If browser is "headless": 
   fromSurface = True
else: 
   fromSurface= False

[nednguyen, 2017-05-17 10:18:13]

Description was changed from

==========
Pass False for fromSurface argument to Page.captureScreenshot.

BUG=chromium:714058
==========

to

==========
Pass False for fromSurface argument to Page.captureScreenshot.

BUG=chromium:714058
==========

[nednguyen, 2017-05-17 10:18:13]

nednguyen@google.com changed reviewers:
+ rmistry@chromium.org

[Ken Russell (switch to Gerrit), 2017-05-17 17:40:22]

On 2017/05/17 10:18:04, nednguyen wrote:
> On 2017/05/17 07:01:53, Ken Russell wrote:
> > On 2017/05/17 00:22:25, nednguyen wrote:
> > > On 2017/05/17 00:18:18, Ken Russell wrote:
> > > > PTAL
> > > > 
> > > > I only lightly tested this.
> > > 
> > > 
> > > What is the drawback of this? Would this fail to capture screenshot in
some
> > > case?
> > 
> > If for some reason someone launched Chrome via Telemetry with the --headless
> > option, this would break. But otherwise there's no drawback. It just ensures
> > that the current semantics of Page.captureScreenshot are preserved. The
> DevTools
> > team wants to change the default value of the fromSurface argument from
false
> to
> > true; see http://crbug.com/714058 and
> > https://codereview.chromium.org/2871113002/ . The GPU tests must continue to
> > pass false for this argument in order to not lose test coverage.
> 
> Ravi is planning to use headless with Telemetry. I think this logic probably
> should be change to:
> If browser is "headless": 
>    fromSurface = True
> else: 
>    fromSurface= False

Sounds good; do you have a hint about how to get at the browser type at this
level? Or should I pass it in from the top level?

[nednguyen, 2017-05-17 17:55:46]

On 2017/05/17 17:40:22, Ken Russell wrote:
> On 2017/05/17 10:18:04, nednguyen wrote:
> > On 2017/05/17 07:01:53, Ken Russell wrote:
> > > On 2017/05/17 00:22:25, nednguyen wrote:
> > > > On 2017/05/17 00:18:18, Ken Russell wrote:
> > > > > PTAL
> > > > > 
> > > > > I only lightly tested this.
> > > > 
> > > > 
> > > > What is the drawback of this? Would this fail to capture screenshot in
> some
> > > > case?
> > > 
> > > If for some reason someone launched Chrome via Telemetry with the
--headless
> > > option, this would break. But otherwise there's no drawback. It just
ensures
> > > that the current semantics of Page.captureScreenshot are preserved. The
> > DevTools
> > > team wants to change the default value of the fromSurface argument from
> false
> > to
> > > true; see http://crbug.com/714058 and
> > > https://codereview.chromium.org/2871113002/ . The GPU tests must continue
to
> > > pass false for this argument in order to not lose test coverage.
> > 
> > Ravi is planning to use headless with Telemetry. I think this logic probably
> > should be change to:
> > If browser is "headless": 
> >    fromSurface = True
> > else: 
> >    fromSurface= False
> 
> Sounds good; do you have a hint about how to get at the browser type at this
> level? Or should I pass it in from the top level?

I think we need to plumb it in from the higher level :P

Though since it's not easy, I would be fine if we can leave a TODO here about
handling the case of "headless". Would be great if it throws exception rather
than fails silently when that happens, but I would be fine too if that's not
possible.

[Ken Russell (switch to Gerrit), 2017-05-17 23:35:47]

On 2017/05/17 17:55:46, nednguyen wrote:
> On 2017/05/17 17:40:22, Ken Russell wrote:
> > On 2017/05/17 10:18:04, nednguyen wrote:
> > > On 2017/05/17 07:01:53, Ken Russell wrote:
> > > > On 2017/05/17 00:22:25, nednguyen wrote:
> > > > > On 2017/05/17 00:18:18, Ken Russell wrote:
> > > > > > PTAL
> > > > > > 
> > > > > > I only lightly tested this.
> > > > > 
> > > > > 
> > > > > What is the drawback of this? Would this fail to capture screenshot in
> > some
> > > > > case?
> > > > 
> > > > If for some reason someone launched Chrome via Telemetry with the
> --headless
> > > > option, this would break. But otherwise there's no drawback. It just
> ensures
> > > > that the current semantics of Page.captureScreenshot are preserved. The
> > > DevTools
> > > > team wants to change the default value of the fromSurface argument from
> > false
> > > to
> > > > true; see http://crbug.com/714058 and
> > > > https://codereview.chromium.org/2871113002/ . The GPU tests must
continue
> to
> > > > pass false for this argument in order to not lose test coverage.
> > > 
> > > Ravi is planning to use headless with Telemetry. I think this logic
probably
> > > should be change to:
> > > If browser is "headless": 
> > >    fromSurface = True
> > > else: 
> > >    fromSurface= False
> > 
> > Sounds good; do you have a hint about how to get at the browser type at this
> > level? Or should I pass it in from the top level?
> 
> I think we need to plumb it in from the higher level :P
> 
> Though since it's not easy, I would be fine if we can leave a TODO here about
> handling the case of "headless". Would be great if it throws exception rather
> than fails silently when that happens, but I would be fine too if that's not
> possible.

After searching through the code base, there is currently no knowledge about
whether Chrome is running in headless mode. I'll be happy to plumb that
knowledge down once there is, but in the meantime I left a TODO for Ravi.

[Ken Russell (switch to Gerrit), 2017-05-17 23:35:56]

PTAL again.

[nednguyen, 2017-05-18 00:30:45]

lgtm

[Ken Russell (switch to Gerrit), 2017-05-18 00:54:25]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2017-05-18 00:54:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 01:19:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495068865494880,
"parent_rev": "0b7a82e228583819237fe4080cb5ab63d0ee064b", "commit_rev":
"dd9db5d3f658449a05fea103ae5f3c2bc329e6d5"}

[commit-bot: I haz the power, 2017-05-18 01:19:04]

Description was changed from

==========
Pass False for fromSurface argument to Page.captureScreenshot.

BUG=chromium:714058
==========

to

==========
Pass False for fromSurface argument to Page.captureScreenshot.

BUG=chromium:714058

Review-Url: https://codereview.chromium.org/2887023002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-05-18 01:19:05]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...