Add pickling traits for the WebFilterOperations class.

Add pickling traits for the WebFilterOperations class.

This class will be part of the WebCompositorFrame class for transport
under ubercompositor.

R=piman,jam@chromium.org
BUG=146080
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158687

[danakj, 2012-09-21 23:38:26]

Hi jam,

PTAL at this change. I'm running into issues with header file namespaces as
WebRect pickling is defined in content/ but now needed also in content/public.

Thanks.

https://codereview.chromium.org/10966050/diff/3001/content/public/common/webk...
File content/public/common/webkit_param_traits.cc (right):

https://codereview.chromium.org/10966050/diff/3001/content/public/common/webk...
content/public/common/webkit_param_traits.cc:46: IPC_STRUCT_TRAITS_END()
I strongly suspect this is the wrong thing to do, but I don't know what is the
right thing to allow us to pickle WebFilterOperation which has these as members.

[danakj, 2012-09-21 23:56:34]

Ok, I've moved the WebRect and WebPoint definitions from view_messages.h to
webkit_param_traits.h as per jam@'s advice (thanks!).

PTAL

[jamesr, 2012-09-22 00:12:15]

It's weird that these traits go in content/public/, since at least in this case
both the sender and receiver of the IPCs using these parameters will be inside
content/

[piman, 2012-09-22 01:01:36]

https://codereview.chromium.org/10966050/diff/6001/content/public/common/webk...
File content/public/common/webkit_param_traits.cc (right):

https://codereview.chromium.org/10966050/diff/6001/content/public/common/webk...
content/public/common/webkit_param_traits.cc:220: if (p.isEmpty()) {
nit: do we need to special case this? Isn't the below code going to have exactly
the same effect?

https://codereview.chromium.org/10966050/diff/6001/content/public/common/webk...
content/public/common/webkit_param_traits.cc:338: break;
Add a default case that returns false.
This is important for security, since the data is untrusted. By the time you
return from this, you want to be able to trust the data.

[danakj, 2012-09-22 14:23:29]

On Fri, Sep 21, 2012 at 8:12 PM,  <jamesr@chromium.org> wrote:
> It's weird that these traits go in content/public/, since at least in this
> case
> both the sender and receiver of the IPCs using these parameters will be
> inside content/

Where do you think is the right place for this? I could make a
content/common/cc/cc_param_traits.h|cc similar to indexed_db? I'm not
really sure what's appropriate here.

[danakj, 2012-09-22 14:31:46]

https://codereview.chromium.org/10966050/diff/6001/content/public/common/webk...
File content/public/common/webkit_param_traits.cc (right):

https://codereview.chromium.org/10966050/diff/6001/content/public/common/webk...
content/public/common/webkit_param_traits.cc:220: if (p.isEmpty()) {
On 2012/09/22 01:01:36, piman wrote:
> nit: do we need to special case this? Isn't the below code going to have
exactly
> the same effect?

Ya  true, I inherited this from the WebData one. Thanks.

https://codereview.chromium.org/10966050/diff/6001/content/public/common/webk...
content/public/common/webkit_param_traits.cc:338: break;
On 2012/09/22 01:01:36, piman wrote:
> Add a default case that returns false.
> This is important for security, since the data is untrusted. By the time you
> return from this, you want to be able to trust the data.

Oh, okay! I did this with a success bool instead of a default: case so we can
still get compile warnings about missing cases.

[piman, 2012-09-22 17:19:37]

On 2012/09/22 14:23:29, danakj wrote:
> On Fri, Sep 21, 2012 at 8:12 PM,  <mailto:jamesr@chromium.org> wrote:
> > It's weird that these traits go in content/public/, since at least in this
> > case
> > both the sender and receiver of the IPCs using these parameters will be
> > inside content/
> 
> Where do you think is the right place for this? I could make a
> content/common/cc/cc_param_traits.h|cc similar to indexed_db? I'm not
> really sure what's appropriate here.

Putting them in content/common/cc would definitely be appropriate.
I personally don't have a strong opinion, slight bias against public, however
since there's already a bunch there, it doesn't bother me so much. If others
have a stronger opinion please speak up.

[piman, 2012-09-22 17:19:45]

lgtm

[danakj, 2012-09-24 18:05:28]

On 2012/09/22 17:19:37, piman wrote:
> Putting them in content/common/cc would definitely be appropriate.
> I personally don't have a strong opinion, slight bias against public, however
> since there's already a bunch there, it doesn't bother me so much. If others
> have a stronger opinion please speak up.

In https://codereview.chromium.org/10915298/ you added param traits in
content/common/gpu/client/cc_param_traits.h for WebCompositorFrame. Since these
things are going to be part of a WCFrame, I think I should just move them into
that file?

[piman, 2012-09-24 18:14:49]

On Mon, Sep 24, 2012 at 11:05 AM, <danakj@chromium.org> wrote:

> On 2012/09/22 17:19:37, piman wrote:
>
>> Putting them in content/common/cc would definitely be appropriate.
>> I personally don't have a strong opinion, slight bias against public,
>> however
>> since there's already a bunch there, it doesn't bother me so much. If
>> others
>> have a stronger opinion please speak up.
>>
>
> In
https://codereview.chromium.**org/10915298/<https://codereview.chromium.org/1...
added param traits in
> content/common/gpu/client/cc_**param_traits.h for WebCompositorFrame.
> Since these
> things are going to be part of a WCFrame, I think I should just move them
> into
> that file?
>

I think just content/common/cc is fine - I'd move the WCFrame traits there.
Since that CL was experimental I was just lazy and din't want to add a
directory :)



>
>
https://codereview.chromium.**org/10966050/<https://codereview.chromium.org/1...
>
> --
>
>
>

[jam, 2012-09-24 18:28:33]

On 2012/09/24 18:05:28, danakj wrote:
> On 2012/09/22 17:19:37, piman wrote:
> > Putting them in content/common/cc would definitely be appropriate.
> > I personally don't have a strong opinion, slight bias against public,
however
> > since there's already a bunch there, it doesn't bother me so much. If others
> > have a stronger opinion please speak up.
> 
> In https://codereview.chromium.org/10915298/ you added param traits in
> content/common/gpu/client/cc_param_traits.h for WebCompositorFrame. Since
these
> things are going to be part of a WCFrame, I think I should just move them into
> that file?

this sounds good to me (I commented on that file too).

for WebRect and WebPoint, it would be good to have automatic trait creation for
them (i.e. using IPC_STRUCT_TRAITS_BEGIN  as they are now, instead of writing
custom serialization traits). that can be done by having webkit_param_traits.h
have two sections, one that's protected by ifdef headers (i.e.  everything
that's there now before your change), and another section that is included
multiple times by content/common/content_message_generator.h (that section would
include IPC_STRUCT_TRAITS_BEGIN macros)

[danakj, 2012-09-24 20:36:13]

PTAL

I've added content/common/cc/cc_messages.h and
content/common/cc/cc_param_traits.h|cc files for these things.

I moved the ones added recently in other places that are meant for cc into this
directory.

[aelias_OOO_until_Jul13, 2012-09-24 20:39:08]

Are you sure WebData traits will be usable by the IPC pickler, given that you're
moving it out of a public/ directory?

My preference would be to keep WebData and WebTransformationMatrix in
webkit_param_traits given that there's nothing CC-specific about them, anyway.

[piman, 2012-09-24 20:50:06]

On Mon, Sep 24, 2012 at 1:39 PM, <aelias@chromium.org> wrote:

> Are you sure WebData traits will be usable by the IPC pickler, given that
> you're
> moving it out of a public/ directory?
>

It sure can be used, the problem of where it lives is whether or not
chrome/ needs it (in which case it needs to be in public/) or not (in which
case it doesn't matter as long as it's accessible by content/)

My preference would be to keep WebData and WebTransformationMatrix in
> webkit_param_traits given that there's nothing CC-specific about them,
> anyway.
>

See discussion above? ;)



>
>
https://codereview.chromium.**org/10966050/<https://codereview.chromium.org/1...
>

[jam, 2012-09-24 20:56:40]

I wrote a reply that seems to have been lost!

danakj: does this compile? WebPoint/WebRect shouldn't be the cc_param_traits
since they're used by other files as well. i.e. something like
content/common/internal_webkit_param_traits.h

aelias: the traits don't have to be in the public directory

everyone: it looks like webkit_param_traits.h doesn't really need to be in
content/public. i'm looking at this more now because of this thread.

[danakj, 2012-09-24 21:23:34]

On 2012/09/24 20:56:40, John Abd-El-Malek wrote:
> I wrote a reply that seems to have been lost!
> 
> danakj: does this compile? WebPoint/WebRect shouldn't be the cc_param_traits
> since they're used by other files as well. i.e. something like
> content/common/internal_webkit_param_traits.h

It does compile, since the file is included from content_message_generator.h I
guess.

I was trying to move the WebFloat* stuff added in
https://codereview.chromium.org/10948044/diff/5001/content/common/view_messag...

But maybe I don't need to move either, I had some linker errors before that
resolved themselves by not including other messages files, I'll try again.

Web* structs are spread out across so many files, maybe it would make sense to
move them all to some central webkit traits file, but shouldn't be needed for
now.

> aelias: the traits don't have to be in the public directory
> 
> everyone: it looks like webkit_param_traits.h doesn't really need to be in
> content/public. i'm looking at this more now because of this thread.

[jam, 2012-09-24 21:27:52]

On 2012/09/24 21:23:34, danakj wrote:
> On 2012/09/24 20:56:40, John Abd-El-Malek wrote:
> > I wrote a reply that seems to have been lost!
> > 
> > danakj: does this compile? WebPoint/WebRect shouldn't be the cc_param_traits
> > since they're used by other files as well. i.e. something like
> > content/common/internal_webkit_param_traits.h
> 
> It does compile, since the file is included from content_message_generator.h I
> guess.

does it link? there are now two definitions of the traits for WebPoint/WebRect

> 
> I was trying to move the WebFloat* stuff added in
>
https://codereview.chromium.org/10948044/diff/5001/content/common/view_messag...
> 
> But maybe I don't need to move either, I had some linker errors before that
> resolved themselves by not including other messages files, I'll try again.
> 
> Web* structs are spread out across so many files, maybe it would make sense to
> move them all to some central webkit traits file, but shouldn't be needed for
> now.

it's a bit confusing because of hacks needed to keep chrome_frame.dll from
linking in webkit.

> 
> > aelias: the traits don't have to be in the public directory
> > 
> > everyone: it looks like webkit_param_traits.h doesn't really need to be in
> > content/public. i'm looking at this more now because of this thread.

[danakj, 2012-09-24 21:29:56]

On Mon, Sep 24, 2012 at 5:27 PM,  <jam@chromium.org> wrote:
> On 2012/09/24 21:23:34, danakj wrote:
>>
>> On 2012/09/24 20:56:40, John Abd-El-Malek wrote:
>> > I wrote a reply that seems to have been lost!
>> >
>> > danakj: does this compile? WebPoint/WebRect shouldn't be the
>> > cc_param_traits
>> > since they're used by other files as well. i.e. something like
>> > content/common/internal_webkit_param_traits.h
>
>
>> It does compile, since the file is included from
>> content_message_generator.h I
>> guess.
>
>
> does it link? there are now two definitions of the traits for
> WebPoint/WebRect

yes, it did locally, but i'll try move them back and get it to link that way.

[jam, 2012-09-24 22:17:03]

btw I forgot to ask: why is content/common/cc directory added? where will the
code that will include these headers live? shouldn't this just be
content/common/gpu?

[piman, 2012-09-24 22:22:34]

On Mon, Sep 24, 2012 at 3:17 PM, <jam@chromium.org> wrote:

> btw I forgot to ask: why is content/common/cc directory added? where will
> the
> code that will include these headers live? shouldn't this just be
> content/common/gpu?
>

cc now works without a GPU, so putting it into gpu/ is a bit of a stretch.
But I don't feel strongly.


>
https://codereview.chromium.**org/10966050/<https://codereview.chromium.org/1...
>

[jam, 2012-09-24 23:14:53]

On 2012/09/24 22:22:34, piman wrote:
> On Mon, Sep 24, 2012 at 3:17 PM, <mailto:jam@chromium.org> wrote:
> 
> > btw I forgot to ask: why is content/common/cc directory added? where will
> > the
> > code that will include these headers live? shouldn't this just be
> > content/common/gpu?
> >
> 
> cc now works without a GPU, so putting it into gpu/ is a bit of a stretch.
> But I don't feel strongly.
> 
> 
> >
>
https://codereview.chromium.**org/10966050/%3Chttps://codereview.chromium.org...>
> >

where will the code that will include these headers live?

it's sometimes hard to reason about these small changes when they don't have
context.

[piman, 2012-09-24 23:19:35]

On Mon, Sep 24, 2012 at 4:14 PM, <jam@chromium.org> wrote:

> On 2012/09/24 22:22:34, piman wrote:
>
>  On Mon, Sep 24, 2012 at 3:17 PM, <mailto:jam@chromium.org> wrote:
>>
>
>  > btw I forgot to ask: why is content/common/cc directory added? where
>> will
>> > the
>> > code that will include these headers live? shouldn't this just be
>> > content/common/gpu?
>> >
>>
>
>  cc now works without a GPU, so putting it into gpu/ is a bit of a stretch.
>> But I don't feel strongly.
>>
>
>
>  >
>>
>
> https://codereview.chromium.****org/10966050/%3Chttps://codere**
> view.chromium.org/10966050/ <http://codereview.chromium.org/10966050/>>
>
>  >
>>
>
> where will the code that will include these headers live?
>

In content/common for the message definitions, content/renderer and
content/browser/renderer_host for the message senders and receivers
(RWH/RWHV*/CompositorOutputSurface).


>
> it's sometimes hard to reason about these small changes when they don't
> have
> context.
>

You feel like reviewing 5k bombs? ;)


>
https://codereview.chromium.**org/10966050/<https://codereview.chromium.org/1...
>

[jam, 2012-09-24 23:33:15]

On 2012/09/24 23:19:35, piman wrote:
> On Mon, Sep 24, 2012 at 4:14 PM, <mailto:jam@chromium.org> wrote:
> 
> > On 2012/09/24 22:22:34, piman wrote:
> >
> >  On Mon, Sep 24, 2012 at 3:17 PM, <mailto:jam@chromium.org> wrote:
> >>
> >
> >  > btw I forgot to ask: why is content/common/cc directory added? where
> >> will
> >> > the
> >> > code that will include these headers live? shouldn't this just be
> >> > content/common/gpu?
> >> >
> >>
> >
> >  cc now works without a GPU, so putting it into gpu/ is a bit of a stretch.
> >> But I don't feel strongly.
> >>
> >
> >
> >  >
> >>
> >
> > https://codereview.chromium.****org/10966050/%253Chttps://codere**
> > view.chromium.org/10966050/ <http://codereview.chromium.org/10966050/>>
> >
> >  >
> >>
> >
> > where will the code that will include these headers live?
> >
> 
> In content/common for the message definitions, content/renderer and
> content/browser/renderer_host for the message senders and receivers
> (RWH/RWHV*/CompositorOutputSurface).

I see. In that case, it seems that the messages files belong in content/common
without any subdirectories (whoever changes them will need owner approval for
RWH/RHWV anyways)

> 
> 
> >
> > it's sometimes hard to reason about these small changes when they don't
> > have
> > context.
> >
> 
> You feel like reviewing 5k bombs? ;)

no, not 5k :) I guess it's a tradeoff, but with a change this small, it's hard
for me to reason about it.

> 
> 
> >
>
https://codereview.chromium.**org/10966050/%3Chttps://codereview.chromium.org...>
> >

[danakj, 2012-09-25 17:07:35]

PTAL. Added the traits to the new cc_messages. Thanks for cleaning this stuff up
jam.

[jam, 2012-09-25 21:18:52]

lgtm

[commit-bot: I haz the power, 2012-09-25 21:21:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/10966050/16012

[commit-bot: I haz the power, 2012-09-25 23:46:27]

Change committed as 158687