Bug 188195

Created attachment 346167 [details]
Patch

Created attachment 346168 [details]
Patch

Created attachment 346169 [details]
Patch

Comment on attachment 346169 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsLayer.cpp:295
> +static const TransformationMatrix identityTransform { };

The braces here are OK, but not needed.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:119
> +    TransformationMatrix(const TransformationMatrix& t) { *this = t; }

I know this is just showing up in the patch because of changes the other other nearby functions, but why is this needed? Isn’t this what the compiler will generate if we don’t write a copy constructor explicitly? Same question about the assignment operator below. Or maybe we do need to define these explicitly but can use "= default".

Comment on attachment 346169 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsLayer.cpp:295
>> +static const TransformationMatrix identityTransform { };
> 
> The braces here are OK, but not needed.

Personally, I would like to have this here since it makes explicit that this is a definition :).

>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:119
>> +    TransformationMatrix(const TransformationMatrix& t) { *this = t; }
> 
> I know this is just showing up in the patch because of changes the other other nearby functions, but why is this needed? Isn’t this what the compiler will generate if we don’t write a copy constructor explicitly? Same question about the assignment operator below. Or maybe we do need to define these explicitly but can use "= default".

In this patch, I keep these things. TransformationMatrix::operator= is different from the default operator: it first checks `*this != other`, and memcpy if the above condition is true.
I'm not sure this check before copying is important... I think it is rather costly. But in the meantime, I would like to keep this patch simple by not having operator=/copy constructor changes.
I'll open a bug to remove operator=/copy constructor separately.

Committed r234427: <https://trac.webkit.org/changeset/234427>

<rdar://problem/42774823>

Comment on attachment 346169 [details]
Patch

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

>>> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:119
>>> +    TransformationMatrix(const TransformationMatrix& t) { *this = t; }
>> 
>> I know this is just showing up in the patch because of changes the other other nearby functions, but why is this needed? Isn’t this what the compiler will generate if we don’t write a copy constructor explicitly? Same question about the assignment operator below. Or maybe we do need to define these explicitly but can use "= default".
> 
> In this patch, I keep these things. TransformationMatrix::operator= is different from the default operator: it first checks `*this != other`, and memcpy if the above condition is true.
> I'm not sure this check before copying is important... I think it is rather costly. But in the meantime, I would like to keep this patch simple by not having operator=/copy constructor changes.
> I'll open a bug to remove operator=/copy constructor separately.

Ah, no. setMatrix's `if (m && m != m_matrix)` is pointer check since m is a C array. default copy constructor just works as expected.

Comment on attachment 346169 [details]
Patch

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

>>> Source/WebCore/platform/graphics/GraphicsLayer.cpp:295
>>> +static const TransformationMatrix identityTransform { };
>> 
>> The braces here are OK, but not needed.
> 
> Personally, I would like to have this here since it makes explicit that this is a definition :).

Can TransformationMatrix.h just export a static const TransformationMatrix identityTransform?

Comment on attachment 346169 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/GraphicsLayer.cpp:295
>>>> +static const TransformationMatrix identityTransform { };
>>> 
>>> The braces here are OK, but not needed.
>> 
>> Personally, I would like to have this here since it makes explicit that this is a definition :).
> 
> Can TransformationMatrix.h just export a static const TransformationMatrix identityTransform?

That sounds nice. I'll try this.