Bug 47728 – Add public functions to serialize TransformOperations

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

this is needed for WebKit2 on Qt (and maybe other platforms), to allow serialization of the GraphicsLayer animation data to the UI thread, which is going to control the actual animations.

[Noam Rosenthal]

I'm thinking of something along the lines of

TransformOperation {
...
    virtual void getSerializableData(StringBuffer&) const;
    virtual void putSerializableData(const StringBuffer&);
};

Any comments/objections?

[Simon Fraser (smfr)]

I think serialize() and deserialize() would be fine. Or make a constructor for deserialization.

[Noam Rosenthal]

Created attachment 70889 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 70889 [details]
Patch

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

> WebCore/ChangeLog:70505
> -        Bug 41340 - [GStreamer] Subtle race condition during seeks
> +        Bug 41340 - [GStreamer] Subtle race condition during seeks

Why this change?

[Noam Rosenthal]

(In reply to comment #4)
> (From update of attachment 70889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70889&action=review
> 
> > WebCore/ChangeLog:70505
> > -        Bug 41340 - [GStreamer] Subtle race condition during seeks
> > +        Bug 41340 - [GStreamer] Subtle race condition during seeks
> 
> Why this change?

over-motivated text editors.. will remove after the reset is reviewed.

[Noam Rosenthal]

s/reset/rest

[Simon Fraser (smfr)]

Comment on attachment 70889 [details]
Patch

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

I'm not convinced that it's OK to serialize everything to doubles. I guess there are tokens in the serialization stream that are not visible to these methods?

> WebCore/platform/graphics/transforms/RotateTransformOperation.cpp:103
> +void RotateTransformOperation::serialize(Vector<double> & data) const
> +{
> +    data.resize(4);
> +    data[0] = m_x;
> +    data[1] = m_y;
> +    data[2] = m_z;
> +    data[3] = m_angle;
> +}
> +

You need to serialize m_type here too.

> WebCore/platform/graphics/transforms/TransformOperation.cpp:43
> +        return ScaleTransformOperation::create(data[0], data[1], data[2], operationType);
> +        break;

No need to break after a return.

> WebCore/platform/graphics/transforms/TransformOperation.cpp:49
> +    case TRANSLATE_3D:
> +        {

Brace goes on previous line.

> WebCore/platform/graphics/transforms/TransformOperation.cpp:56
> +        }
> +        break;

Normally we put the } after the break. But the break can be removed because of the return.

[Noam Rosenthal]

(In reply to comment #7)
> (From update of attachment 70889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70889&action=review
> 
> I'm not convinced that it's OK to serialize everything to doubles. I guess there are tokens in the serialization stream that are not visible to these methods?

Originally I thought about using Vector<char>, but then I just end up casting it to doubles (in all the cases but Translate), which doesn't look right. This seems more robust since I can deal with the actual serialization in CoreIPC::ArgumentEncoder, and just pass doubles to the WebCore::TransformOperation classes. Maybe setRawData is more suitable then serialize then?


> 
> You need to serialize m_type here too.
> 
The idea is to serialize that separately in ArgumentCoder via getOperationType - since I need that information in order to know which class to construct, and 
I don't want to duplicate its serialization for each type.

[Simon Fraser (smfr)]

Then maybe the serialization lies outside this code. You should just expose the member vars you need to serialize.

[Noam Rosenthal]

ok, will do.

[Noam Rosenthal]

Created attachment 70895 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 70895 [details]
Patch

Clearing flags on attachment: 70895

Committed r69884: <http://trac.webkit.org/changeset/69884>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.