Change from_surface default value to true

Change from_surface default value to true

BUG=714058
Review-Url: https://codereview.chromium.org/2871113002
Cr-Commit-Position: refs/heads/master@{#474207}
Committed: https://chromium.googlesource.com/chromium/src/+/b7b00695911984b3be8f9e7894ac84123136e38e

[dvallet, 2017-05-10 06:21:37]

The CQ bit was checked by dvallet@chromium.org to run a CQ dry run

[dvallet, 2017-05-10 06:22:08]

dvallet@chromium.org changed reviewers:
+ eseckler@chromium.org, skyostil@chromium.org

[commit-bot: I haz the power, 2017-05-10 06:22:17]

Dry run: CQ is trying da patch.

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

[Eric Seckler, 2017-05-10 07:00:57]

eseckler@chromium.org changed reviewers:
+ dgozman@chromium.org

[Eric Seckler, 2017-05-10 07:00:57]

Let's update the comment in browser_protocol.json, too. Otherwise lgtm :)

+dgozman for owners

[commit-bot: I haz the power, 2017-05-10 09:00:44]

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

[commit-bot: I haz the power, 2017-05-10 09:00:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2017-05-10 11:12:08]

lgtm with Eric's comment.

[dgozman, 2017-05-10 15:27:01]

The plan is to change it to true by default, and eventually remove it. We can
try that right now and see whether any bots fail.

[dvallet, 2017-05-10 23:39:04]

The CQ bit was checked by dvallet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 23:40:15]

Dry run: 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-11 01:30:47]

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

[commit-bot: I haz the power, 2017-05-11 01:30:47]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dvallet, 2017-05-11 02:36:37]

The CQ bit was checked by dvallet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 02:38:02]

Dry run: 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-11 04:41:28]

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

[commit-bot: I haz the power, 2017-05-11 04:41:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dvallet, 2017-05-11 04:46:09]

PTAL, webkit layout tests are failing, but I suspect it is because the webkit
tree is closed atm

[Eric Seckler, 2017-05-11 07:34:18]

lgtm % nit

https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (left):

https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name":
"fromSurface", "type": "boolean", "optional": true, "description": "Capture the
screenshot from the surface, rather than the view. Defaults to false.",
"experimental": true }
I'd keep "experimental": true, if we want to get rid of this soon.

[dgozman, 2017-05-11 20:48:05]

lgtm

https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (left):

https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name":
"fromSurface", "type": "boolean", "optional": true, "description": "Capture the
screenshot from the surface, rather than the view. Defaults to false.",
"experimental": true }
On 2017/05/11 07:34:18, Eric Seckler wrote:
> I'd keep "experimental": true, if we want to get rid of this soon.

+1

[dgozman, 2017-05-11 20:48:19]

On 2017/05/11 20:48:05, dgozman wrote:
> lgtm
> 
>
https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/inspector/browser_protocol.json (left):
> 
>
https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name":
> "fromSurface", "type": "boolean", "optional": true, "description": "Capture
the
> screenshot from the surface, rather than the view. Defaults to false.",
> "experimental": true }
> On 2017/05/11 07:34:18, Eric Seckler wrote:
> > I'd keep "experimental": true, if we want to get rid of this soon.
> 
> +1

Please also update the description.

[dvallet, 2017-05-12 00:29:25]

Description was changed from

==========
Change from_surface default when running in headless mode to true

BUG=714058
==========

to

==========
Change from_surface default value to true

BUG=714058
==========

[dvallet, 2017-05-12 00:32:57]

The CQ bit was checked by dvallet@chromium.org

[dvallet, 2017-05-12 00:32:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, dgozman@chromium.org, eseckler@chromium.org
Link to the patchset: https://codereview.chromium.org/2871113002/#ps60001
(title: "added experimental flag")

[commit-bot: I haz the power, 2017-05-12 00:34:47]

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-12 02:42:36]

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

[commit-bot: I haz the power, 2017-05-12 02:42:36]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dvallet, 2017-05-12 02:53:07]

The CQ bit was checked by dvallet@chromium.org

[commit-bot: I haz the power, 2017-05-12 02:53:43]

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-12 05:10:31]

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

[commit-bot: I haz the power, 2017-05-12 05:10:32]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dvallet, 2017-05-12 05:13:39]

The CQ bit was checked by dvallet@chromium.org

[commit-bot: I haz the power, 2017-05-12 05:13:51]

CQ is trying da patch.

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

[dvallet, 2017-05-12 05:45:21]

So changing the default brakes 
gpu_tests.pixel_integration_test.PixelIntegrationTest.Pixel_Video_VP9
on Mac. 
It looks like it taking a screenshot from this code:
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...

I'm not sure how to change this (I think it might be related to a selenium
python library) So changing the default behavior might actually brake a bunch of
stuff.

My recommendation is going back to having from_surface == true for headless mode
and leave as is for non-headless

WDYT?

https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (left):

https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name":
"fromSurface", "type": "boolean", "optional": true, "description": "Capture the
screenshot from the surface, rather than the view. Defaults to false.",
"experimental": true }
On 2017/05/11 at 20:48:05, dgozman wrote:
> On 2017/05/11 07:34:18, Eric Seckler wrote:
> > I'd keep "experimental": true, if we want to get rid of this soon.
> 
> +1

Done

[commit-bot: I haz the power, 2017-05-12 07:27:36]

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

[commit-bot: I haz the power, 2017-05-12 07:27:38]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgozman, 2017-05-12 18:07:49]

On 2017/05/12 05:45:21, dvallet wrote:
> So changing the default brakes 
> gpu_tests.pixel_integration_test.PixelIntegrationTest.Pixel_Video_VP9
> on Mac. 
> It looks like it taking a screenshot from this code:
>
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
I looked at the test failures - perhaps we could just rebaseline? I'll add
someone from gpu in the loop.

> 
> I'm not sure how to change this (I think it might be related to a selenium
> python library) So changing the default behavior might actually brake a bunch
of
> stuff.
> 
> My recommendation is going back to having from_surface == true for headless
mode
> and leave as is for non-headless
I don't really like checking for headless in page_handler.cc. Can you make
headless always send true as a parameter?

> 
> WDYT?
> 
>
https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/inspector/browser_protocol.json (left):
> 
>
https://codereview.chromium.org/2871113002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/inspector/browser_protocol.json:423: { "name":
> "fromSurface", "type": "boolean", "optional": true, "description": "Capture
the
> screenshot from the surface, rather than the view. Defaults to false.",
> "experimental": true }
> On 2017/05/11 at 20:48:05, dgozman wrote:
> > On 2017/05/11 07:34:18, Eric Seckler wrote:
> > > I'd keep "experimental": true, if we want to get rid of this soon.
> > 
> > +1
> 
> Done

[dgozman, 2017-05-12 18:13:49]

dgozman@chromium.org changed reviewers:
+ kbr@chromium.org

[dgozman, 2017-05-12 18:13:49]

Ken, could you please take a look at gpu test failures?

For more context: it would be really nice to switch screenshots to be taken from
surface rather than doing readback (as I understand it). Both DevTools and
headless already use this mode, with only telemetry left on the old path. This
will allow us to clean up a lot of code and reduce maintenance, but I'm not sure
how comfortable you are with this, since it's a semantic change. WDYT?

Tests log here:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...
Sample diff here:
http://chromium-browser-gpu-tests.commondatastorage.googleapis.com/view_test_...

[Ken Russell (switch to Gerrit), 2017-05-16 16:49:21]

Description was changed from

==========
Change from_surface default value to true

BUG=714058
==========

to

==========
Change from_surface default value to true

BUG=714058
==========

[Ken Russell (switch to Gerrit), 2017-05-16 17:09:06]

On 2017/05/12 18:13:49, dgozman wrote:
> Ken, could you please take a look at gpu test failures?
> 
> For more context: it would be really nice to switch screenshots to be taken
from
> surface rather than doing readback (as I understand it). Both DevTools and
> headless already use this mode, with only telemetry left on the old path. This
> will allow us to clean up a lot of code and reduce maintenance, but I'm not
sure
> how comfortable you are with this, since it's a semantic change. WDYT?
> 
> Tests log here:
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...
> Sample diff here:
>
http://chromium-browser-gpu-tests.commondatastorage.googleapis.com/view_test_...

The pixel differences seem fine but the semantic change is concerning. The main
reason the GPU tests try to read back the on-screen view is that there is a
significant amount of platform-specific code which presents to the screen, and
it's possible to break this code so that all the browser's tests pass but the
browser window is actually rendering black.

I think we need to preserve the ability to screenshot the on-screen view, and
expand it. jbauman@ found for example that on Windows with Aura it's not
actually reading the on-screen view, and he's been working to get that code path
to work with DirectComposition.

Could we please discuss this more offline?

[dvallet, 2017-05-16 23:33:52]

Looks like we might take a while to decide. 
Can I submit Patch #2 (keeping the experimental flag) to fix the bugs for
headless for now?

[Ken Russell (switch to Gerrit), 2017-05-16 23:46:55]

On 2017/05/16 23:33:52, dvallet wrote:
> Looks like we might take a while to decide. 
> Can I submit Patch #2 (keeping the experimental flag) to fix the bugs for
> headless for now?

Patch set #2 breaks the Telemetry based tests on the commit queue, so no, it
can't be submitted as is.

I suggest you change the implementation of this method to ignore the fromSurface
argument and treat it as always true if running in headless mode, which you
should be able to easily detect from inside Chrome's code. This will avoid
changes to the DevTools API.

[dgozman, 2017-05-16 23:57:31]

On 2017/05/16 23:46:55, Ken Russell wrote:
> On 2017/05/16 23:33:52, dvallet wrote:
> > Looks like we might take a while to decide. 
> > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs for
> > headless for now?
> 
> Patch set #2 breaks the Telemetry based tests on the commit queue, so no, it
> can't be submitted as is.
> 
> I suggest you change the implementation of this method to ignore the
fromSurface
> argument and treat it as always true if running in headless mode, which you
> should be able to easily detect from inside Chrome's code. This will avoid
> changes to the DevTools API.

I'd prefer to send |fromSurface=false| from telemetry, and make it true by
default. Special-casing headless is not the right way to move forward.

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

On 2017/05/16 23:57:31, dgozman wrote:
> On 2017/05/16 23:46:55, Ken Russell wrote:
> > On 2017/05/16 23:33:52, dvallet wrote:
> > > Looks like we might take a while to decide. 
> > > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs for
> > > headless for now?
> > 
> > Patch set #2 breaks the Telemetry based tests on the commit queue, so no, it
> > can't be submitted as is.
> > 
> > I suggest you change the implementation of this method to ignore the
> fromSurface
> > argument and treat it as always true if running in headless mode, which you
> > should be able to easily detect from inside Chrome's code. This will avoid
> > changes to the DevTools API.
> 
> I'd prefer to send |fromSurface=false| from telemetry, and make it true by
> default. Special-casing headless is not the right way to move forward.

OK. https://codereview.chromium.org/2887023002/ uploaded. I'm not sure it's
correct.

[dvallet, 2017-05-17 07:03:50]

On 2017/05/17 at 00:18:45, kbr wrote:
> On 2017/05/16 23:57:31, dgozman wrote:
> > On 2017/05/16 23:46:55, Ken Russell wrote:
> > > On 2017/05/16 23:33:52, dvallet wrote:
> > > > Looks like we might take a while to decide. 
> > > > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs
for
> > > > headless for now?
> > > 
> > > Patch set #2 breaks the Telemetry based tests on the commit queue, so no,
it
> > > can't be submitted as is.
> > > 
> > > I suggest you change the implementation of this method to ignore the
> > fromSurface
> > > argument and treat it as always true if running in headless mode, which
you
> > > should be able to easily detect from inside Chrome's code. This will avoid
> > > changes to the DevTools API.
> > 
> > I'd prefer to send |fromSurface=false| from telemetry, and make it true by
> > default. Special-casing headless is not the right way to move forward.
> 
> OK. https://codereview.chromium.org/2887023002/ uploaded. I'm not sure it's
correct.

Sorry, I meant Patch #1. But yea happy to wait until crrev/2887023002 is
submitted

[Ken Russell (switch to Gerrit), 2017-05-18 16:39:04]

On 2017/05/17 07:03:50, dvallet wrote:
> On 2017/05/17 at 00:18:45, kbr wrote:
> > On 2017/05/16 23:57:31, dgozman wrote:
> > > On 2017/05/16 23:46:55, Ken Russell wrote:
> > > > On 2017/05/16 23:33:52, dvallet wrote:
> > > > > Looks like we might take a while to decide. 
> > > > > Can I submit Patch #2 (keeping the experimental flag) to fix the bugs
> for
> > > > > headless for now?
> > > > 
> > > > Patch set #2 breaks the Telemetry based tests on the commit queue, so
no,
> it
> > > > can't be submitted as is.
> > > > 
> > > > I suggest you change the implementation of this method to ignore the
> > > fromSurface
> > > > argument and treat it as always true if running in headless mode, which
> you
> > > > should be able to easily detect from inside Chrome's code. This will
avoid
> > > > changes to the DevTools API.
> > > 
> > > I'd prefer to send |fromSurface=false| from telemetry, and make it true by
> > > default. Special-casing headless is not the right way to move forward.
> > 
> > OK. https://codereview.chromium.org/2887023002/ uploaded. I'm not sure it's
> correct.
> 
> Sorry, I meant Patch #1. But yea happy to wait until crrev/2887023002 is
> submitted

https://codereview.chromium.org/2887023002/ has rolled in in
https://chromium-review.googlesource.com/c/508260 . OK to proceed with this CL
(whichever patch set you had in mind). Thanks for waiting.

[dvallet, 2017-05-18 23:15:19]

On 2017/05/18 at 16:39:04, kbr wrote:
> On 2017/05/17 07:03:50, dvallet wrote:
> > On 2017/05/17 at 00:18:45, kbr wrote:
> > > On 2017/05/16 23:57:31, dgozman wrote:
> > > > On 2017/05/16 23:46:55, Ken Russell wrote:
> > > > > On 2017/05/16 23:33:52, dvallet wrote:
> > > > > > Looks like we might take a while to decide. 
> > > > > > Can I submit Patch #2 (keeping the experimental flag) to fix the
bugs
> > for
> > > > > > headless for now?
> > > > > 
> > > > > Patch set #2 breaks the Telemetry based tests on the commit queue, so
no,
> > it
> > > > > can't be submitted as is.
> > > > > 
> > > > > I suggest you change the implementation of this method to ignore the
> > > > fromSurface
> > > > > argument and treat it as always true if running in headless mode,
which
> > you
> > > > > should be able to easily detect from inside Chrome's code. This will
avoid
> > > > > changes to the DevTools API.
> > > > 
> > > > I'd prefer to send |fromSurface=false| from telemetry, and make it true
by
> > > > default. Special-casing headless is not the right way to move forward.
> > > 
> > > OK. https://codereview.chromium.org/2887023002/ uploaded. I'm not sure
it's
> > correct.
> > 
> > Sorry, I meant Patch #1. But yea happy to wait until crrev/2887023002 is
> > submitted
> 
> https://codereview.chromium.org/2887023002/ has rolled in in
https://chromium-review.googlesource.com/c/508260 . OK to proceed with this CL
(whichever patch set you had in mind). Thanks for waiting.

Thanks!, I'll try to commit the patch

[dvallet, 2017-05-18 23:15:22]

The CQ bit was checked by dvallet@chromium.org

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

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-19 02:04:33]

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

[commit-bot: I haz the power, 2017-05-19 02:04:35]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dvallet, 2017-05-19 02:28:18]

The CQ bit was checked by dvallet@chromium.org

[commit-bot: I haz the power, 2017-05-19 02:29:27]

CQ is trying da patch.

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

[Ken Russell (switch to Gerrit), 2017-05-19 02:39:59]

On 2017/05/19 02:04:35, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

This is http://crbug.com/722246 , but there's even worse flakiness on this
tryserver now filed as http://crbug.com/724350 .

[commit-bot: I haz the power, 2017-05-19 05:48:35]

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

[commit-bot: I haz the power, 2017-05-19 05:48:37]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

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

On 2017/05/19 05:48:37, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences. Doesn't
this imply that Telemetry's fromSurface=false isn't being respected?

[dvallet, 2017-05-22 02:58:47]

On 2017/05/19 at 17:26:23, kbr wrote:
> On 2017/05/19 05:48:37, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences.
Doesn't this imply that Telemetry's fromSurface=false isn't being respected?

Are you certain that those tests make use of the code changed in
https://codereview.chromium.org/2887023002/?
My guess is that part of those tests are still not setting the fromSurface
command, and thus is defaulting to true.

[Ken Russell (switch to Gerrit), 2017-05-22 18:45:06]

On 2017/05/22 02:58:47, dvallet wrote:
> On 2017/05/19 at 17:26:23, kbr wrote:
> > On 2017/05/19 05:48:37, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
> > >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> > 
> > Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences.
> Doesn't this imply that Telemetry's fromSurface=false isn't being respected?
> 
> Are you certain that those tests make use of the code changed in
> https://codereview.chromium.org/2887023002/
> My guess is that part of those tests are still not setting the fromSurface
> command, and thus is defaulting to true.

Yes, I'm positive these tests are going through that code path in Telemetry.

Do you have a Mac and can you reproduce this behavior locally? You can undo all
your changes, build Release, delete the following directories:

src/content/test/data/gpu/gpu_reference/
src/content/test/data/gpu/generated/

and run:

./content/test/gpu/run_gpu_integration_test.py pixel --browser=release
--test-filter=Pixel_Video

Then apply your changes, rebuild, and re-run. The tests should hopefully fail
the second time. They will be using the files written into gpu_reference/ and
write the error images into generated/ .

[dvallet, 2017-05-23 05:03:31]

On 2017/05/22 at 18:45:06, kbr wrote:
> On 2017/05/22 02:58:47, dvallet wrote:
> > On 2017/05/19 at 17:26:23, kbr wrote:
> > > On 2017/05/19 05:48:37, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> > > 
> > > Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences.
> > Doesn't this imply that Telemetry's fromSurface=false isn't being respected?
> > 
> > Are you certain that those tests make use of the code changed in
> > https://codereview.chromium.org/2887023002/
> > My guess is that part of those tests are still not setting the fromSurface
> > command, and thus is defaulting to true.
> 
> Yes, I'm positive these tests are going through that code path in Telemetry.
> 
> Do you have a Mac and can you reproduce this behavior locally? You can undo
all your changes, build Release, delete the following directories:
> 
> src/content/test/data/gpu/gpu_reference/
> src/content/test/data/gpu/generated/
> 
> and run:
> 
> ./content/test/gpu/run_gpu_integration_test.py pixel --browser=release
--test-filter=Pixel_Video
> 
> Then apply your changes, rebuild, and re-run. The tests should hopefully fail
the second time. They will be using the files written into gpu_reference/ and
write the error images into generated/ .

OK, I debugged this locally and I found the problem: The change in
https://codereview.chromium.org/2887023002/ is incorrect, instead of

request = {
        'method': 'Page.captureScreenshot',
        'fromSurface': False
        }

It should be 

request = {
        'method': 'Page.captureScreenshot',
        'params': {
            'fromSurface': False
          }
        }

I'd sent a change myself but I don't really know how send cls to catapult

[dvallet, 2017-05-23 05:03:34]

On 2017/05/22 at 18:45:06, kbr wrote:
> On 2017/05/22 02:58:47, dvallet wrote:
> > On 2017/05/19 at 17:26:23, kbr wrote:
> > > On 2017/05/19 05:48:37, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
> > > >
> >
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> > > 
> > > Pixel_Video_MP4 and Pixel_Video_VP9 both failed with subtle differences.
> > Doesn't this imply that Telemetry's fromSurface=false isn't being respected?
> > 
> > Are you certain that those tests make use of the code changed in
> > https://codereview.chromium.org/2887023002/
> > My guess is that part of those tests are still not setting the fromSurface
> > command, and thus is defaulting to true.
> 
> Yes, I'm positive these tests are going through that code path in Telemetry.
> 
> Do you have a Mac and can you reproduce this behavior locally? You can undo
all your changes, build Release, delete the following directories:
> 
> src/content/test/data/gpu/gpu_reference/
> src/content/test/data/gpu/generated/
> 
> and run:
> 
> ./content/test/gpu/run_gpu_integration_test.py pixel --browser=release
--test-filter=Pixel_Video
> 
> Then apply your changes, rebuild, and re-run. The tests should hopefully fail
the second time. They will be using the files written into gpu_reference/ and
write the error images into generated/ .

OK, I debugged this locally and I found the problem: The change in
https://codereview.chromium.org/2887023002/ is incorrect, instead of

request = {
        'method': 'Page.captureScreenshot',
        'fromSurface': False
        }

It should be 

request = {
        'method': 'Page.captureScreenshot',
        'params': {
            'fromSurface': False
          }
        }

I'd sent a change myself but I don't really know how send cls to catapult

[Sami, 2017-05-23 16:43:14]

On 2017/05/23 05:03:34, dvallet wrote:
> I'd sent a change myself but I don't really know how send cls to catapult

It's pretty similar to Chrome actually -- instructions here:
https://github.com/catapult-project/catapult/blob/master/CONTRIBUTING.md

[Ken Russell (switch to Gerrit), 2017-05-23 22:31:06]

On 2017/05/23 16:43:14, Sami wrote:
> On 2017/05/23 05:03:34, dvallet wrote:
> > I'd sent a change myself but I don't really know how send cls to catapult
> 
> It's pretty similar to Chrome actually -- instructions here:
> https://github.com/catapult-project/catapult/blob/master/CONTRIBUTING.md

Oops. Thanks dvallet@ for finding this. Fixing in
https://codereview.chromium.org/2901153002/ .

[dvallet, 2017-05-24 00:12:44]

On 2017/05/23 at 22:31:06, kbr wrote:
> On 2017/05/23 16:43:14, Sami wrote:
> > On 2017/05/23 05:03:34, dvallet wrote:
> > > I'd sent a change myself but I don't really know how send cls to catapult
> > 
> > It's pretty similar to Chrome actually -- instructions here:
> > https://github.com/catapult-project/catapult/blob/master/CONTRIBUTING.md
> 
> Oops. Thanks dvallet@ for finding this. Fixing in
https://codereview.chromium.org/2901153002/ .

Thanks! Let me know when it's rolled in

[dvallet, 2017-05-24 05:10:55]

The CQ bit was checked by dvallet@chromium.org

[dvallet, 2017-05-24 05:10:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, eseckler@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2871113002/#ps100001
(title: "revert to defaulting to true")

[commit-bot: I haz the power, 2017-05-24 05:11:59]

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-24 08:04:46]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495602655877680,
"parent_rev": "e296b2b02fe6997bd09faca1b4efb86f3158c90e", "commit_rev":
"b7b00695911984b3be8f9e7894ac84123136e38e"}

[commit-bot: I haz the power, 2017-05-24 08:04:55]

Description was changed from

==========
Change from_surface default value to true

BUG=714058
==========

to

==========
Change from_surface default value to true

BUG=714058

Review-Url: https://codereview.chromium.org/2871113002
Cr-Commit-Position: refs/heads/master@{#474207}
Committed:
https://chromium.googlesource.com/chromium/src/+/b7b00695911984b3be8f9e7894ac...
==========

[commit-bot: I haz the power, 2017-05-24 08:04:56]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b7b00695911984b3be8f9e7894ac...