Implement RenderPass StructTraits

Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in
surfaces_type_converters that will go away in a subsequent
CL. bitmap_uploader has been updated to speak to cc
directly.

This CL also deletes the test executable mojo_surfaces_lib_unittests.

BUG=611802
TBR=ben@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ff18d775f07c5c2f8002dfe563f081427045a187
Cr-Commit-Position: refs/heads/master@{#401861}

[Fady Samuel, 2016-06-20 22:58:27]

Description was changed from

==========
WIP: RenderPass StructTraits

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

to

==========
WIP: RenderPass StructTraits

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

[Fady Samuel, 2016-06-21 21:44:57]

Description was changed from

==========
WIP: RenderPass StructTraits

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

to

==========
Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of 
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in 
surfaces_type_converters that will go away in a subsequent 
CL. bitmap_uploader has been updated to speak to cc 
directly.

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

[Fady Samuel, 2016-06-21 21:51:31]

fsamuel@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org, dpranke@chromium.org,
rjkroege@chromium.org, yzshen@chromium.org

[Fady Samuel, 2016-06-21 21:51:32]

rjkroege@: overall
danakj@ for cc
dcheng@ for ipc
dpranke@ for testing.

[Dirk Pranke, 2016-06-21 23:40:23]

//testing lgtm.

[Dirk Pranke, 2016-06-21 23:41:17]

https://codereview.chromium.org/2088603002/diff/120001/mojo/BUILD.gn
File mojo/BUILD.gn (left):

https://codereview.chromium.org/2088603002/diff/120001/mojo/BUILD.gn#oldcode32
mojo/BUILD.gn:32:
"//components/mus/public/cpp/surfaces/tests:mojo_surfaces_lib_unittests",
You should mention that you're deleting this test executable in the CL
description.

[yzshen1, 2016-06-21 23:51:21]

LGTM with one question.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass_str...
File cc/ipc/render_pass_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:64:
DCHECK_EQ(shared_quad_state_references[it.index()], sqs_iter.index());
(I am not familiar with this data structure...)
Are these DCHECKs never violated even if the message sender is malicious and
sends us invalid data?

[danakj, 2016-06-22 00:05:09]

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/quads_struct_tr...
File cc/ipc/quads_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/quads_struct_tr...
cc/ipc/quads_struct_traits.cc:267:
quad->resources.ids[cc::YUVVideoDrawQuad::kAPlaneResourceIdIndex] =
no count here? (a is optional, i think the other 3 aren't. pls double check me.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass.mojom
File cc/ipc/render_pass.mojom (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass.moj...
cc/ipc/render_pass.mojom:15: RenderPassId id;
Can you explain this to me?

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass_str...
File cc/ipc/render_pass_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:37: bool StructTraits<cc::mojom::RenderPass,
std::unique_ptr<cc::RenderPass>>::Read(
Where does the writer live?

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:64:
DCHECK_EQ(shared_quad_state_references[it.index()], sqs_iter.index());
1On 2016/06/21 23:51:21, yzshen1 wrote:
> (I am not familiar with this data structure...)
> Are these DCHECKs never violated even if the message sender is malicious and
> sends us invalid data?

The receiver needs to reject bad messages. The sender should DCHECK to find bugs
there.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:330: texture_draw_quad->material = material5;
You should be calling TextureDrawQuad::SetNew() or SetAll(), not setting fields
manually. Otherwise it's easy to miss fields now or when they get added.

The same goes for all the other quads here.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:392: shared_state->SetAll(gfx::Transform(),
gfx::Size(), gfx::Rect(), gfx::Rect(),
how about using some non-0/false stuff, its easy to pass with 0s when it
shouldn't

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:398: gfx::Rect(), SkColor(), false);
dittos

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:424: DrawQuad* out_draw_quad =
output->quad_list.ElementAt(0);
Can you test with multiple quads in 1 SQS, and multiple SQSs? I think this test
is a bit too trivial right now

https://codereview.chromium.org/2088603002/diff/120001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/quads/render_pass.c...
cc/quads/render_pass.cc:90: RenderPass& RenderPass::operator=(RenderPass&&
other) {
can you use =default?

https://codereview.chromium.org/2088603002/diff/120001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/quads/render_pass.h...
cc/quads/render_pass.h:62: RenderPass& operator=(RenderPass&& other);
make a constructor too then.

[danakj, 2016-06-22 00:06:21]

https://codereview.chromium.org/2088603002/diff/120001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:84:
std::unique_ptr<cc::RenderPass> pass = cc::RenderPass::Create();
include render_pass.h too then

https://codereview.chromium.org/2088603002/diff/120001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:85: pass->output_rect = bounds;
use SetNew/SetAll

[Fady Samuel, 2016-06-22 00:10:25]

Description was changed from

==========
Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of 
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in 
surfaces_type_converters that will go away in a subsequent 
CL. bitmap_uploader has been updated to speak to cc 
directly.

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

to

==========
Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of 
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in 
surfaces_type_converters that will go away in a subsequent 
CL. bitmap_uploader has been updated to speak to cc 
directly.

This CL also deletes the test executable mojo_surfaces_lib_unittests.

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

[Fady Samuel, 2016-06-22 20:08:13]

fsamuel@chromium.org changed reviewers:
+ meacer@chromium.org

[Fady Samuel, 2016-06-22 20:08:15]

PTAL Dana! + meacer@ as dcheng is OOO

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/quads_struct_tr...
File cc/ipc/quads_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/quads_struct_tr...
cc/ipc/quads_struct_traits.cc:267:
quad->resources.ids[cc::YUVVideoDrawQuad::kAPlaneResourceIdIndex] =
On 2016/06/22 00:05:09, danakj wrote:
> no count here? (a is optional, i think the other 3 aren't. pls double check
me.

  resources.count = resources.ids[kAPlaneResourceIdIndex] ? 4 : 3; basically.
Added.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass.mojom
File cc/ipc/render_pass.mojom (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass.moj...
cc/ipc/render_pass.mojom:15: RenderPassId id;
On 2016/06/22 00:05:09, danakj wrote:
> Can you explain this to me?

Explain what? We're in the cc.mojom module and so we don't need the cc.mojom
prefix.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass_str...
File cc/ipc/render_pass_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:37: bool StructTraits<cc::mojom::RenderPass,
std::unique_ptr<cc::RenderPass>>::Read(
On 2016/06/22 00:05:09, danakj wrote:
> Where does the writer live?

All the accessors in render_pass_struct_traits.h. A StructTraits tells mojo how
to read a C++ type to translate it into the wire format.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:64:
DCHECK_EQ(shared_quad_state_references[it.index()], sqs_iter.index());
On 2016/06/21 23:51:21, yzshen1 wrote:
> (I am not familiar with this data structure...)
> Are these DCHECKs never violated even if the message sender is malicious and
> sends us invalid data?

Good point. I changed them to return false if the conditions are violated.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:64:
DCHECK_EQ(shared_quad_state_references[it.index()], sqs_iter.index());
On 2016/06/22 00:05:09, danakj wrote:
> 1On 2016/06/21 23:51:21, yzshen1 wrote:
> > (I am not familiar with this data structure...)
> > Are these DCHECKs never violated even if the message sender is malicious and
> > sends us invalid data?
> 
> The receiver needs to reject bad messages. The sender should DCHECK to find
bugs
> there.

Done.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:330: texture_draw_quad->material = material5;
On 2016/06/22 00:05:09, danakj wrote:
> You should be calling TextureDrawQuad::SetNew() or SetAll(), not setting
fields
> manually. Otherwise it's easy to miss fields now or when they get added.
> 
> The same goes for all the other quads here.

We don't serialize SharedQuadState here, and so attaching it to a DrawQuad to
avoid a DCHECK.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:392: shared_state->SetAll(gfx::Transform(),
gfx::Size(), gfx::Rect(), gfx::Rect(),
On 2016/06/22 00:05:09, danakj wrote:
> how about using some non-0/false stuff, its easy to pass with 0s when it
> shouldn't

Done.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:398: gfx::Rect(), SkColor(), false);
On 2016/06/22 00:05:09, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:424: DrawQuad* out_draw_quad =
output->quad_list.ElementAt(0);
On 2016/06/22 00:05:09, danakj wrote:
> Can you test with multiple quads in 1 SQS, and multiple SQSs? I think this
test
> is a bit too trivial right now

Done.

https://codereview.chromium.org/2088603002/diff/120001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/quads/render_pass.c...
cc/quads/render_pass.cc:90: RenderPass& RenderPass::operator=(RenderPass&&
other) {
On 2016/06/22 00:05:09, danakj wrote:
> can you use =default?

This change is unnecessary. Removed.

https://codereview.chromium.org/2088603002/diff/120001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/quads/render_pass.h...
cc/quads/render_pass.h:62: RenderPass& operator=(RenderPass&& other);
On 2016/06/22 00:05:09, danakj wrote:
> make a constructor too then.

This is actually an unnecessary change since I made the typemap to a
std::unique_ptr<cc::RenderPass>..

https://codereview.chromium.org/2088603002/diff/120001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:84:
std::unique_ptr<cc::RenderPass> pass = cc::RenderPass::Create();
On 2016/06/22 00:06:20, danakj wrote:
> include render_pass.h too then

Done.

https://codereview.chromium.org/2088603002/diff/120001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:85: pass->output_rect = bounds;
On 2016/06/22 00:06:21, danakj wrote:
> use SetNew/SetAll

Done.

https://codereview.chromium.org/2088603002/diff/120001/mojo/BUILD.gn
File mojo/BUILD.gn (left):

https://codereview.chromium.org/2088603002/diff/120001/mojo/BUILD.gn#oldcode32
mojo/BUILD.gn:32:
"//components/mus/public/cpp/surfaces/tests:mojo_surfaces_lib_unittests",
On 2016/06/21 23:41:17, Dirk Pranke wrote:
> You should mention that you're deleting this test executable in the CL
> description.

Done.

[danakj, 2016-06-22 20:51:35]

Thanks, cc LGTM

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/quads_struct_tr...
File cc/ipc/quads_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/quads_struct_tr...
cc/ipc/quads_struct_traits.cc:267:
quad->resources.ids[cc::YUVVideoDrawQuad::kAPlaneResourceIdIndex] =
On 2016/06/22 20:08:14, Fady Samuel wrote:
> On 2016/06/22 00:05:09, danakj wrote:
> > no count here? (a is optional, i think the other 3 aren't. pls double check
> me.
> 
>   resources.count = resources.ids[kAPlaneResourceIdIndex] ? 4 : 3; basically.
> Added.

https://cs.chromium.org/chromium/src/cc/layers/video_layer_impl.cc?rcl=146659...

There can be 2 resources, but then it repeats U twice, so I guess that is
correct ya.

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass.mojom
File cc/ipc/render_pass.mojom (right):

https://codereview.chromium.org/2088603002/diff/120001/cc/ipc/render_pass.moj...
cc/ipc/render_pass.mojom:15: RenderPassId id;
On 2016/06/22 20:08:14, Fady Samuel wrote:
> On 2016/06/22 00:05:09, danakj wrote:
> > Can you explain this to me?
> 
> Explain what? We're in the cc.mojom module and so we don't need the cc.mojom
> prefix. 

So it's equivalent. ok Thanks

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/quads_struct_tr...
File cc/ipc/quads_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/quads_struct_tr...
cc/ipc/quads_struct_traits.cc:269: quad->resources.count =
data.a_plane_resource_id() ? 4 : 3;
This depends on the A resource coming last in the array, since you insert them
based on the A index, not just in order that they are written. Maybe you can
static assert this here so we know it's the case in the future?

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/struct_traits_u...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:443: SolidColorDrawQuad* color_quad =
Can you add comments to explain the steps this test is doing and why? Such as
"This quad uses the first shared quad state". "The next 3 quads use the second
shared quad state". etc.. It really helps understand a test for readers to have
these types of comment, since they tend to be walls of code and the intent gets
lost.

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:446:
color_quad->SetNew(input->shared_quad_state_list.back(), color_quad_rect,
nit: shared_state_2 to mirror the use of shared_state_1 above?

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:452:
surface_quad->SetNew(input->shared_quad_state_list.back(), surface_quad_rect,
nitto?

https://codereview.chromium.org/2088603002/diff/160001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (right):

https://codereview.chromium.org/2088603002/diff/160001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:76: const gfx::Rect
bounds(window_->bounds().size());
is there a test for this behaviour change?

[Fady Samuel, 2016-06-22 21:41:19]

Thanks Dana! PTAL Rob, and Mustafa! Thanks!

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/quads_struct_tr...
File cc/ipc/quads_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/quads_struct_tr...
cc/ipc/quads_struct_traits.cc:269: quad->resources.count =
data.a_plane_resource_id() ? 4 : 3;
On 2016/06/22 20:51:35, danakj wrote:
> This depends on the A resource coming last in the array, since you insert them
> based on the A index, not just in order that they are written. Maybe you can
> static assert this here so we know it's the case in the future?

Done.

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/struct_traits_u...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:443: SolidColorDrawQuad* color_quad =
On 2016/06/22 20:51:35, danakj wrote:
> Can you add comments to explain the steps this test is doing and why? Such as
> "This quad uses the first shared quad state". "The next 3 quads use the second
> shared quad state". etc.. It really helps understand a test for readers to
have
> these types of comment, since they tend to be walls of code and the intent
gets
> lost.

Done.

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:446:
color_quad->SetNew(input->shared_quad_state_list.back(), color_quad_rect,
On 2016/06/22 20:51:35, danakj wrote:
> nit: shared_state_2 to mirror the use of shared_state_1 above?

Done.

https://codereview.chromium.org/2088603002/diff/160001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:452:
surface_quad->SetNew(input->shared_quad_state_list.back(), surface_quad_rect,
On 2016/06/22 20:51:35, danakj wrote:
> nitto?

Done.

https://codereview.chromium.org/2088603002/diff/160001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (right):

https://codereview.chromium.org/2088603002/diff/160001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:76: const gfx::Rect
bounds(window_->bounds().size());
On 2016/06/22 20:51:35, danakj wrote:
> is there a test for this behaviour change?

Unfortunately bitmap_uploader lacks tests. I'll write one in a separate CL. I
have tested Mus+Ash manually with this CL.

[Fady Samuel, 2016-06-22 21:51:50]

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

[commit-bot: I haz the power, 2016-06-22 21:53:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088603002/180001

[commit-bot: I haz the power, 2016-06-22 22:03:49]

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

[commit-bot: I haz the power, 2016-06-22 22:03:50]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[Fady Samuel, 2016-06-23 03:37:14]

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

[commit-bot: I haz the power, 2016-06-23 03:37:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088603002/200001

[Fady Samuel, 2016-06-23 04:32:21]

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

[commit-bot: I haz the power, 2016-06-23 04:32:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088603002/220001

[commit-bot: I haz the power, 2016-06-23 05:30:42]

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

[commit-bot: I haz the power, 2016-06-23 05:30:43]

Dry run: This issue passed the CQ dry run.

[rjkroege, 2016-06-23 14:09:59]

lgtm with some minor comments

https://codereview.chromium.org/2088603002/diff/220001/cc/ipc/struct_traits_u...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2088603002/diff/220001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:322:
background_filters.Append(FilterOperation::CreateSaturateFilter(4.f));
Should the created saturation filters be different to ensure that the test is
disambiguating them or are they intentionally the same? If the latter, it's
conceivable that you should say so.

https://codereview.chromium.org/2088603002/diff/220001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (right):

https://codereview.chromium.org/2088603002/diff/220001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:144: const bool needs_blending =
true;
We should set this to true only when the SkBitmap has alpha yes?

[Fady Samuel, 2016-06-23 15:30:17]

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

[commit-bot: I haz the power, 2016-06-23 15:30:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088603002/240001

[Fady Samuel, 2016-06-23 15:31:09]

PTAL!

https://codereview.chromium.org/2088603002/diff/220001/cc/ipc/struct_traits_u...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2088603002/diff/220001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:322:
background_filters.Append(FilterOperation::CreateSaturateFilter(4.f));
On 2016/06/23 14:09:59, rjkroege wrote:
> Should the created saturation filters be different to ensure that the test is
> disambiguating them or are they intentionally the same? If the latter, it's
> conceivable that you should say so.

Done.

https://codereview.chromium.org/2088603002/diff/220001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (right):

https://codereview.chromium.org/2088603002/diff/220001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:144: const bool needs_blending =
true;
On 2016/06/23 14:09:59, rjkroege wrote:
> We should set this to true only when the SkBitmap has alpha yes?

I'm not sure. I don't want to make behaviorial changes in this CL. Also,
bitmap_uploader is essentially deprecated in favor of accelerated compositing.

[commit-bot: I haz the power, 2016-06-23 16:41:09]

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

[commit-bot: I haz the power, 2016-06-23 16:41:10]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-06-24 09:12:20]

lgtm with nits

https://codereview.chromium.org/2088603002/diff/240001/cc/ipc/render_pass_str...
File cc/ipc/render_pass_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/240001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:16: mojo::Array<uint32_t>* sqs_references =
Nit: std::unique_ptr and release at the end.

https://codereview.chromium.org/2088603002/diff/240001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:25: DCHECK_EQ(*sqs_iter,
(*it)->shared_quad_state);
For clarity, it might help to DCHECK() that the input shared quad state isn't
null here.

[Fady Samuel, 2016-06-24 13:32:28]

Description was changed from

==========
Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of 
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in 
surfaces_type_converters that will go away in a subsequent 
CL. bitmap_uploader has been updated to speak to cc 
directly.

This CL also deletes the test executable mojo_surfaces_lib_unittests.

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

to

==========
Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of 
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in 
surfaces_type_converters that will go away in a subsequent 
CL. bitmap_uploader has been updated to speak to cc 
directly.

This CL also deletes the test executable mojo_surfaces_lib_unittests.

BUG=611802
TBR=ben@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-06-24 13:33:20]

fsamuel@chromium.org changed reviewers:
+ ben@chromium.org

[Fady Samuel, 2016-06-24 13:33:21]

Thanks Daniel! TBR'ing Ben for trivial changes to delete a couple of includes
in:

components/mus/ws/platform_display.cc
content/renderer/mus/render_widget_mus_connection.cc

https://codereview.chromium.org/2088603002/diff/240001/cc/ipc/render_pass_str...
File cc/ipc/render_pass_struct_traits.cc (right):

https://codereview.chromium.org/2088603002/diff/240001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:16: mojo::Array<uint32_t>* sqs_references =
On 2016/06/24 09:12:20, dcheng wrote:
> Nit: std::unique_ptr and release at the end.

Done.

https://codereview.chromium.org/2088603002/diff/240001/cc/ipc/render_pass_str...
cc/ipc/render_pass_struct_traits.cc:25: DCHECK_EQ(*sqs_iter,
(*it)->shared_quad_state);
On 2016/06/24 09:12:20, dcheng wrote:
> For clarity, it might help to DCHECK() that the input shared quad state isn't
> null here.

Done.

[Fady Samuel, 2016-06-24 13:33:31]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-24 13:33:31]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
dpranke@chromium.org, danakj@chromium.org, rjkroege@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2088603002/#ps260001
(title: "Fix bots")

[commit-bot: I haz the power, 2016-06-24 13:33:48]

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

[Fady Samuel, 2016-06-24 13:41:57]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-24 13:41:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, dpranke@chromium.org, dcheng@chromium.org,
danakj@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2088603002/#ps280001
(title: "Cleanup")

[commit-bot: I haz the power, 2016-06-24 13:42:07]

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

[commit-bot: I haz the power, 2016-06-24 14:44:41]

Description was changed from

==========
Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of 
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in 
surfaces_type_converters that will go away in a subsequent 
CL. bitmap_uploader has been updated to speak to cc 
directly.

This CL also deletes the test executable mojo_surfaces_lib_unittests.

BUG=611802
TBR=ben@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of 
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in 
surfaces_type_converters that will go away in a subsequent 
CL. bitmap_uploader has been updated to speak to cc 
directly.

This CL also deletes the test executable mojo_surfaces_lib_unittests.

BUG=611802
TBR=ben@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-24 14:44:43]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-06-24 14:45:50]

Description was changed from

==========
Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of 
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in 
surfaces_type_converters that will go away in a subsequent 
CL. bitmap_uploader has been updated to speak to cc 
directly.

This CL also deletes the test executable mojo_surfaces_lib_unittests.

BUG=611802
TBR=ben@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement RenderPass StructTraits

This CL implements RenderPass StructTraits. Most of
surfaces_type_converters goes away now. There's still extra
copies due to lack of context propagation and array traits
data views. There's also an extra copy in
surfaces_type_converters that will go away in a subsequent
CL. bitmap_uploader has been updated to speak to cc
directly.

This CL also deletes the test executable mojo_surfaces_lib_unittests.

BUG=611802
TBR=ben@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ff18d775f07c5c2f8002dfe563f081427045a187
Cr-Commit-Position: refs/heads/master@{#401861}
==========

[commit-bot: I haz the power, 2016-06-24 14:45:51]

Patchset 15 (id:??) landed as
https://crrev.com/ff18d775f07c5c2f8002dfe563f081427045a187
Cr-Commit-Position: refs/heads/master@{#401861}