Re-enable IOSurface Canvas2D.

Re-enable IOSurface Canvas2D.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same. This CL
passes the texture size through WebExternalTextureMailbox.

This CL also adds a pixel test.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4e7ecf36fb2af3e3677c085de5b1ea6e7e75f3be
Cr-Commit-Position: refs/heads/master@{#381570}

[erikchen, 2016-03-15 21:06:24]

Description was changed from

==========
Pass texture size through WebExternalTextureMailbox.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same.

BUG=595063
==========

to

==========
Pass texture size through WebExternalTextureMailbox.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-03-15 21:08:34]

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

[erikchen, 2016-03-15 21:08:35]

kbr: Please review.

I will include the new pixel test with the CL that reenables IOSurface Canvas2D.

[erikchen, 2016-03-15 21:09:39]

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

[commit-bot: I haz the power, 2016-03-15 21:10:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1804243002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1804243002/1

[commit-bot: I haz the power, 2016-03-15 21:20:08]

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

[commit-bot: I haz the power, 2016-03-15 21:20:09]

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

[Ken Russell (switch to Gerrit), 2016-03-15 21:41:54]

Looks good in principle, but will wait to review until the tryjobs are green.

[erikchen, 2016-03-15 22:00:58]

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

[commit-bot: I haz the power, 2016-03-15 22:01:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1804243002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1804243002/20001

[commit-bot: I haz the power, 2016-03-16 00:41:34]

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

[commit-bot: I haz the power, 2016-03-16 00:41:35]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-03-16 01:02:20]

erikchen@chromium.org changed reviewers:
+ aelias@chromium.org, danakj@chromium.org

[erikchen, 2016-03-16 01:02:20]

danakj: Please review cc/
aelias: Please review third_party/WebKit/public/

[aelias_OOO_until_Jul13, 2016-03-16 01:18:46]

third_party/WebKit/public/ lgtm

[danakj, 2016-03-16 01:21:20]

https://codereview.chromium.org/1804243002/diff/20001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (left):

https://codereview.chromium.org/1804243002/diff/20001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:90: if (client_mailbox.allowOverlay)
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/textu...

Can you explain this change vs that comment?

[Ken Russell (switch to Gerrit), 2016-03-16 01:27:52]

LGTM

Not sure whether the other CL https://codereview.chromium.org/1805673003/ covers
this, but a test should be added for this.

[erikchen, 2016-03-16 01:33:58]

https://codereview.chromium.org/1804243002/diff/20001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (left):

https://codereview.chromium.org/1804243002/diff/20001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:90: if (client_mailbox.allowOverlay)
On 2016/03/16 01:21:19, danakj wrote:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/textu...
> 
> Can you explain this change vs that comment?

For other onlookers, the comment says:
"""
// This is valid if allow_overlau() or IsSharedMemory() is true.
gfx::Size size_in_pixels() const { return size_in_pixels_; }
"""

The comment is accurate, in the sense that if either of those conditions is
true, size_in_pixels is guaranteed to be valid. This change does not contradict
the comment.

I see no reason to prematurely drop information, just because the information is
not currently needed.

[erikchen, 2016-03-16 01:35:21]

On 2016/03/16 01:27:52, Ken Russell wrote:
> LGTM
> 
> Not sure whether the other CL https://codereview.chromium.org/1805673003/
covers
> this, but a test should be added for this.

The new pixel test in the CL you linked tests the functionality in this CL. It
draws the correct output with this CL, and draws incorrect output without this
CL.

[Ken Russell (switch to Gerrit), 2016-03-16 01:39:32]

On 2016/03/16 01:35:21, erikchen wrote:
> On 2016/03/16 01:27:52, Ken Russell wrote:
> > LGTM
> > 
> > Not sure whether the other CL https://codereview.chromium.org/1805673003/
> covers
> > this, but a test should be added for this.
> 
> The new pixel test in the CL you linked tests the functionality in this CL. It
> draws the correct output with this CL, and draws incorrect output without this
> CL.

OK. I'm not used to the dependent patchset workflow in Rietveld but in general I
would ask that the test be added in the same CL that fixes the bug. That will
make the dependency more obvious when looking back at the code later.

[danakj, 2016-03-16 01:48:25]

https://codereview.chromium.org/1804243002/diff/20001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (left):

https://codereview.chromium.org/1804243002/diff/20001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:90: if (client_mailbox.allowOverlay)
On 2016/03/16 01:33:58, erikchen wrote:
> On 2016/03/16 01:21:19, danakj wrote:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/textu...
> > 
> > Can you explain this change vs that comment?
> 
> For other onlookers, the comment says:
> """
> // This is valid if allow_overlau() or IsSharedMemory() is true.
> gfx::Size size_in_pixels() const { return size_in_pixels_; }
> """
> 
> The comment is accurate, in the sense that if either of those conditions is
> true, size_in_pixels is guaranteed to be valid. This change does not
contradict
> the comment.
> 
> I see no reason to prematurely drop information, just because the information
is
> not currently needed. 

Well, but what's the point of passing in something valid when nothing can depend
on it? I don't understand why you'd need to change things if nothing is
depending on it incorrectly.

[erikchen, 2016-03-16 01:55:31]

https://codereview.chromium.org/1804243002/diff/20001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (left):

https://codereview.chromium.org/1804243002/diff/20001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:90: if (client_mailbox.allowOverlay)
On 2016/03/16 01:48:25, danakj wrote:
> On 2016/03/16 01:33:58, erikchen wrote:
> > On 2016/03/16 01:21:19, danakj wrote:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/textu...
> > > 
> > > Can you explain this change vs that comment?
> > 
> > For other onlookers, the comment says:
> > """
> > // This is valid if allow_overlau() or IsSharedMemory() is true.
> > gfx::Size size_in_pixels() const { return size_in_pixels_; }
> > """
> > 
> > The comment is accurate, in the sense that if either of those conditions is
> > true, size_in_pixels is guaranteed to be valid. This change does not
> contradict
> > the comment.
> > 
> > I see no reason to prematurely drop information, just because the
information
> is
> > not currently needed. 
> 
> Well, but what's the point of passing in something valid when nothing can
depend
> on it? I don't understand why you'd need to change things if nothing is
> depending on it incorrectly.

I re-added the conditional """if (client_mailbox.allowOverlay)""".

[danakj, 2016-03-16 01:57:32]

cc LGTM

[erikchen, 2016-03-16 18:02:59]

On 2016/03/16 01:57:32, danakj wrote:
> cc LGTM

Note that patch set 4 patches in https://codereview.chromium.org/1805673003/,
which was separately reviewed and lgtm-ed by kbr.

[erikchen, 2016-03-16 18:03:52]

Description was changed from

==========
Pass texture size through WebExternalTextureMailbox.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Re-enable IOSurface Canvas2D.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same. This CL
passes the texture size through WebExternalTextureMailbox.

This CL also adds a pixel test.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-03-16 18:04:13]

Description was changed from

==========
Re-enable IOSurface Canvas2D.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same. This CL
passes the texture size through WebExternalTextureMailbox.

This CL also adds a pixel test.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Re-enable IOSurface Canvas2D.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same. This CL
passes the texture size through WebExternalTextureMailbox.

This CL also adds a pixel test.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-03-16 18:04:21]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-03-16 18:04:21]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
aelias@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1804243002/#ps60001
(title: "Patch in https://codereview.chromium.org/1805673003/.")

[commit-bot: I haz the power, 2016-03-16 18:04:56]

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

[commit-bot: I haz the power, 2016-03-16 18:24:26]

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

[commit-bot: I haz the power, 2016-03-16 18:24:28]

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

[erikchen, 2016-03-16 18:33:26]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-03-16 18:33:27]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
kbr@chromium.org, aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/1804243002/#ps100001
(title: "Use the right conditional.")

[commit-bot: I haz the power, 2016-03-16 18:33:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1804243002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1804243002/100001

[commit-bot: I haz the power, 2016-03-16 18:50:31]

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

[commit-bot: I haz the power, 2016-03-16 18:50:33]

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

[erikchen, 2016-03-16 21:00:17]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-03-16 21:00:18]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
kbr@chromium.org, aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/1804243002/#ps120001
(title: "Compile errors from rebase, still...")

[commit-bot: I haz the power, 2016-03-16 21:00:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1804243002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1804243002/120001

[commit-bot: I haz the power, 2016-03-16 22:32:35]

Description was changed from

==========
Re-enable IOSurface Canvas2D.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same. This CL
passes the texture size through WebExternalTextureMailbox.

This CL also adds a pixel test.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Re-enable IOSurface Canvas2D.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same. This CL
passes the texture size through WebExternalTextureMailbox.

This CL also adds a pixel test.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-03-16 22:32:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-16 22:35:34]

Description was changed from

==========
Re-enable IOSurface Canvas2D.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same. This CL
passes the texture size through WebExternalTextureMailbox.

This CL also adds a pixel test.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Re-enable IOSurface Canvas2D.

Previously, WebExternalTextureLayerImpl inferred the size of the texture from
the bounds of the WebLayer, but the two are not necessarily the same. This CL
passes the texture size through WebExternalTextureMailbox.

This CL also adds a pixel test.

BUG=595063
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4e7ecf36fb2af3e3677c085de5b1ea6e7e75f3be
Cr-Commit-Position: refs/heads/master@{#381570}
==========

[commit-bot: I haz the power, 2016-03-16 22:35:37]

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