Bug 97147 – [chromium] Add setters to WebFilterOperation for IPC pickling

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Add flatten() and unflatten() to WebFilterOperation for IPC pickling

[Dana Jansens]

Created attachment 164802 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

I think Stephen wanted to change the filter list into a complete graph (hopefully a DAG) of skia types - is that right?  If so we should serialize that and not WebKit API types.

[James Robinson]

Comment on attachment 164802 [details]
Patch

I'd rather not put pickling or seralization logic directly in WebKit API.  If this type is going to be transported as a WebFoo then expose enough getters/setters to serialize/deserialize the types from chromium land.

[Alexandre Elias]

(In reply to comment #4)
> (From update of attachment 164802 [details])
> I'd rather not put pickling or seralization logic directly in WebKit API.  If this type is going to be transported as a WebFoo then expose enough getters/setters to serialize/deserialize the types from chromium land.

I tried to do that earlier and the results were painful.  You need quite a lot of custom logic to cover all the variants and you can't use the "create" methods (since you're passed in a created but uninitialized logic).  And it would mean having to write a patch in both trees to do any change to WebFilterOperations (in practice the Chromium side might just be forgotten about).  I think the better options are either this "flatten" method approach or just turning this into a struct.

[James Robinson]

I think the real issue here is that we're overloading the WebKit API types for two things - transit types between WebCore stuff and the compositor and internal CC data types.  Sharing these concerns was expedient for GTFO, but I think we really need to migrate the internal CC needs to use different types and have CC-internal concerns handled on the chromium side.  Serialization falls into this category - it's not something you need for communicating between WebCore and the compositor implementation, it's something that CC needs internally.

We don't want to have to make a WebKit patch every time we want to change the serialization format for filters - that'd be ridiculous.

Of course in this particular case if we move to expressing filter changes are graphs of Skia types this is moot for this particular instance.

[Alexandre Elias]

This is actually the only WebKit type that needs special treatment.  So we can avoid the philosophical debate if it's going away to get replaced by entirely Chromium/Skia types.

Is the new format coming soon?  The remaining question is whether we should land some kind of stopgap.

[James Robinson]

That's a question for Stephen - there's a design doc from August and I know it's in the plans, but I don't know the timeline.

[Stephen White]

(In reply to comment #8)
> That's a question for Stephen - there's a design doc from August and I know it's in the plans, but I don't know the timeline.

I have it as a stretch goal for this quarter, but it's looking unlikely.  I think it'll more likely be next quarter.  I'm visiting the NC team right now hashing out some of the details.

[Dana Jansens]

Serializing these is one of the blockers for ubercomp, so what's the right approach to take for now?

1) We can turn this into a struct and drop the ASSERT() guards
2) We can build a bunch of logic into the pickler to know which fields to read for which types, and add setters for everything to the API.
3) We can flatten()/unflatten()
4) ?

[Stephen White]

Comment on attachment 164802 [details]
Patch

BTW I'm fine with this patch as-is as a stopgap.

[Dana Jansens]

@jamesr thoughts?

[James Robinson]

I think 1 or 2 make the most sense.

[Dana Jansens]

Created attachment 165016 [details]
Patch

[Dana Jansens]

Created attachment 165019 [details]
Patch

fix asserts

[Stephen White]

Comment on attachment 165019 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165019&action=review

I'm ok with this, but will leave for James.

> Source/Platform/chromium/public/WebFilterOperation.h:109
> +    static WebFilterOperation createEmptyFilter() { return WebFilterOperation(FilterTypeGrayscale, 0.0); }

We could create a FilterTypeUnknown or something for this purpose, which might allow for some sanity-checking.

[Dana Jansens]

Heh I actually made a FilterTypeUnknown at first but then switches in chromium caused errors and I figured this wasn't worth a 3 way patch..

[James Robinson]

Comment on attachment 165019 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 165019 [details]
Patch

Clearing flags on attachment: 165019

Committed r129243: <http://trac.webkit.org/changeset/129243>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

Reopening since the change was rolled out in http://trac.webkit.org/changeset/129337

[Dana Jansens]

Created attachment 165384 [details]
Patch for landing

[Dana Jansens]

Created attachment 165385 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 165385 [details]
Patch for landing

Clearing flags on attachment: 165385

Committed r129373: <http://trac.webkit.org/changeset/129373>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.