Implement cc::FilterOperation StructTraits

Implement cc::FilterOperation StructTraits

FilterOperation is used by RenderPassDrawQuad whcih is used by CompostiorFrame.
This CL allows FilterOperation to be serialized over mojo.

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

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

[Fady Samuel, 2016-06-08 00:29:13]

Description was changed from

==========
Implement cc::FilterOperation StructTraits

FilterOperation is used by RenderPassDrawQuad whcih is used by CompostiorFrame.
This CL allows FilterOperation to be serialized over mojo.

BUG=611802
==========

to

==========
Implement cc::FilterOperation StructTraits

FilterOperation is used by RenderPassDrawQuad whcih is used by CompostiorFrame.
This CL allows FilterOperation to be serialized over mojo.

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

[Fady Samuel, 2016-06-08 00:31:53]

fsamuel@chromium.org changed reviewers:
+ enne@chromium.org, tsepez@chromium.org, yzshen@chromium.org

[Fady Samuel, 2016-06-08 00:31:54]

This CL is diff'ed against : https://codereview.chromium.org/2046563005/

tsepez@ for ipc
enne@ for cc OWNER
yzshen1@ for StructTrait feedback

[Fady Samuel, 2016-06-08 15:13:43]

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

[commit-bot: I haz the power, 2016-06-08 15:14:33]

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

[yzshen1, 2016-06-08 16:25:16]

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
File cc/ipc/filter_operation.mojom (right):

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
cc/ipc/filter_operation.mojom:33: gfx.mojom.Point? drop_shadow_offset;
the custom type gfx::Point doesn't support null state (i.e., the StructTraits
doesn't have IsNull/SetToNull), therefore, you need to use non-nullable type
(without the question mark) here. Otherwise, when bindings of a different
language sends us a null |drop_shadow_offset|, the deserialization will fail.

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
cc/ipc/filter_operation.mojom:35: skia.mojom.ImageFilter? image_filter;
Ditto.

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/struct_traits_un...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/struct_traits_un...
cc/ipc/struct_traits_unittest.cc:216: continue;
It seems more common to use "break;" here.

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

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

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

Dry run: Exceeded global retry quota

[Fady Samuel, 2016-06-08 17:04:19]

PTAL Yuzhu

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
File cc/ipc/filter_operation.mojom (right):

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
cc/ipc/filter_operation.mojom:33: gfx.mojom.Point? drop_shadow_offset;
On 2016/06/08 16:25:16, yzshen1 wrote:
> the custom type gfx::Point doesn't support null state (i.e., the StructTraits
> doesn't have IsNull/SetToNull), therefore, you need to use non-nullable type
> (without the question mark) here. Otherwise, when bindings of a different
> language sends us a null |drop_shadow_offset|, the deserialization will fail.

I've made it non-optional for now, but really it should be optional I think
because we don't always need to send it. I can investigate this optimization
separately.

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
cc/ipc/filter_operation.mojom:35: skia.mojom.ImageFilter? image_filter;
On 2016/06/08 16:25:16, yzshen1 wrote:
> Ditto.

Done.

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/struct_traits_un...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/struct_traits_un...
cc/ipc/struct_traits_unittest.cc:216: continue;
On 2016/06/08 16:25:16, yzshen1 wrote:
> It seems more common to use "break;" here.

Done.

[yzshen1, 2016-06-08 17:09:52]

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
File cc/ipc/filter_operation.mojom (right):

https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
cc/ipc/filter_operation.mojom:33: gfx.mojom.Point? drop_shadow_offset;
On 2016/06/08 17:04:19, Fady Samuel wrote:
> On 2016/06/08 16:25:16, yzshen1 wrote:
> > the custom type gfx::Point doesn't support null state (i.e., the
StructTraits
> > doesn't have IsNull/SetToNull), therefore, you need to use non-nullable type
> > (without the question mark) here. Otherwise, when bindings of a different
> > language sends us a null |drop_shadow_offset|, the deserialization will
fail.
> 
> I've made it non-optional for now, but really it should be optional I think
> because we don't always need to send it. I can investigate this optimization
> separately.

One way is to make the custom type support null state.

Another possible solution is to map nullable and non-nullable types differently:
mojom.Point -> gfx::Piont mojom.Point? -> base::Optional<gfx::Point>. But that
is just an idea that we haven't determined to add support for.

[yzshen1, 2016-06-08 17:09:59]

On 2016/06/08 17:09:52, yzshen1 wrote:
>
https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
> File cc/ipc/filter_operation.mojom (right):
> 
>
https://codereview.chromium.org/2042413003/diff/20001/cc/ipc/filter_operation...
> cc/ipc/filter_operation.mojom:33: gfx.mojom.Point? drop_shadow_offset;
> On 2016/06/08 17:04:19, Fady Samuel wrote:
> > On 2016/06/08 16:25:16, yzshen1 wrote:
> > > the custom type gfx::Point doesn't support null state (i.e., the
> StructTraits
> > > doesn't have IsNull/SetToNull), therefore, you need to use non-nullable
type
> > > (without the question mark) here. Otherwise, when bindings of a different
> > > language sends us a null |drop_shadow_offset|, the deserialization will
> fail.
> > 
> > I've made it non-optional for now, but really it should be optional I think
> > because we don't always need to send it. I can investigate this optimization
> > separately.
> 
> One way is to make the custom type support null state.
> 
> Another possible solution is to map nullable and non-nullable types
differently:
> mojom.Point -> gfx::Piont mojom.Point? -> base::Optional<gfx::Point>. But that
> is just an idea that we haven't determined to add support for.

LGTM

[Tom Sepez, 2016-06-08 17:33:01]

lgtm

[Fady Samuel, 2016-06-08 17:56:48]

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

[Fady Samuel, 2016-06-08 17:56:49]

Not sure if enne@ is in => moving to TBR. Please let me know if you have any
objections

+ajuma@ for cc stamp

[Fady Samuel, 2016-06-08 17:57:02]

Description was changed from

==========
Implement cc::FilterOperation StructTraits

FilterOperation is used by RenderPassDrawQuad whcih is used by CompostiorFrame.
This CL allows FilterOperation to be serialized over mojo.

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

to

==========
Implement cc::FilterOperation StructTraits

FilterOperation is used by RenderPassDrawQuad whcih is used by CompostiorFrame.
This CL allows FilterOperation to be serialized over mojo.

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

[enne (OOO), 2016-06-08 18:06:52]

lgtm

https://codereview.chromium.org/2042413003/diff/1/cc/ipc/filter_operation.mojom
File cc/ipc/filter_operation.mojom (right):

https://codereview.chromium.org/2042413003/diff/1/cc/ipc/filter_operation.moj...
cc/ipc/filter_operation.mojom:24: SATURATING_BRIGHTNESS,  // Not used in
CSS/SVG.
I'm not sure it's worth perpetuating this comment.

[Fady Samuel, 2016-06-08 19:23:20]

CQ'ing

https://codereview.chromium.org/2042413003/diff/1/cc/ipc/filter_operation.mojom
File cc/ipc/filter_operation.mojom (right):

https://codereview.chromium.org/2042413003/diff/1/cc/ipc/filter_operation.moj...
cc/ipc/filter_operation.mojom:24: SATURATING_BRIGHTNESS,  // Not used in
CSS/SVG.
On 2016/06/08 18:06:52, enne wrote:
> I'm not sure it's worth perpetuating this comment.

Done.

[Fady Samuel, 2016-06-08 19:23:25]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-08 19:23:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
yzshen@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2042413003/#ps80001
(title: "Rebased + Addressed Enne's comment")

[commit-bot: I haz the power, 2016-06-08 19:23:50]

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

[commit-bot: I haz the power, 2016-06-08 21:53:58]

Description was changed from

==========
Implement cc::FilterOperation StructTraits

FilterOperation is used by RenderPassDrawQuad whcih is used by CompostiorFrame.
This CL allows FilterOperation to be serialized over mojo.

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

to

==========
Implement cc::FilterOperation StructTraits

FilterOperation is used by RenderPassDrawQuad whcih is used by CompostiorFrame.
This CL allows FilterOperation to be serialized over mojo.

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

[commit-bot: I haz the power, 2016-06-08 21:54:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-08 21:56:12]

Description was changed from

==========
Implement cc::FilterOperation StructTraits

FilterOperation is used by RenderPassDrawQuad whcih is used by CompostiorFrame.
This CL allows FilterOperation to be serialized over mojo.

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

to

==========
Implement cc::FilterOperation StructTraits

FilterOperation is used by RenderPassDrawQuad whcih is used by CompostiorFrame.
This CL allows FilterOperation to be serialized over mojo.

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

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

[commit-bot: I haz the power, 2016-06-08 21:56:14]

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