Bug 64301 – Switch transform operations to FloatSize

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

Currently they all act on IntSize.

[Levi Weintraub]

Created attachment 100338 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 100338 [details]
Patch

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

Otherwise looks fine.

> Source/WebCore/platform/Length.h:158
> +        default:
> +            return static_cast<float>(undefinedLength);

Normally we don't use default: cases for switch(enum), but maybe it makes sense here?

> Source/WebCore/platform/Length.h:162
> +    float calcFloatValue(float maxValue) const

Did you copy paste this same function twice?

[Levi Weintraub]

Comment on attachment 100338 [details]
Patch

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

>> Source/WebCore/platform/Length.h:158
>> +            return static_cast<float>(undefinedLength);
> 
> Normally we don't use default: cases for switch(enum), but maybe it makes sense here?

This function remains unchanged, I just fixed the indenting for webkit-style.

>> Source/WebCore/platform/Length.h:162
>> +    float calcFloatValue(float maxValue) const
> 
> Did you copy paste this same function twice?

I'm adding a version that takes a float maxValue.

[Eric Seidel (no email)]

Comment on attachment 100338 [details]
Patch

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

>>> Source/WebCore/platform/Length.h:162
>>> +    float calcFloatValue(float maxValue) const
>> 
>> Did you copy paste this same function twice?
> 
> I'm adding a version that takes a float maxValue.

Shouldn't we just templatize this sucker?

[Levi Weintraub]

Comment on attachment 100338 [details]
Patch

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

>>>> Source/WebCore/platform/Length.h:162
>>>> +    float calcFloatValue(float maxValue) const
>>> 
>>> Did you copy paste this same function twice?
>> 
>> I'm adding a version that takes a float maxValue.
> 
> Shouldn't we just templatize this sucker?

Ultimately I believe lengths should stop being ints at all, but I don't know that templates will help us here since we generally create lengths at parse-time. Anyways I think I'll revisit this when we're a bit closer to switching over. This whole class could use some cleanup at that point.

[Levi Weintraub]

Created attachment 100348 [details]
Patch

[Levi Weintraub]

(In reply to comment #6)
> Created an attachment (id=100348) [details]
> Patch

Ping for re-review :)

[Eric Seidel (no email)]

Comment on attachment 100348 [details]
Patch

This changes some platform things to know about Layout*.  Darin may feel this is also a layering violation.  You should check.

[Darin Adler]

Comment on attachment 100348 [details]
Patch

This looks like a layering violation to me. The transforms themselves are part of the graphics framework, not the rendering system, so they should not be using layout types. Instead they could be overloaded to work on both int and float, or changed to always use float.

[Simon Fraser (smfr)]

Comment on attachment 100348 [details]
Patch

Agree with Darin.

[Levi Weintraub]

Created attachment 102050 [details]
Patch

[Levi Weintraub]

Can I get a review? This just works :)

[Levi Weintraub]

Comment on attachment 102050 [details]
Patch

Thanks for the review, Simon.

[Levi Weintraub]

Comment on attachment 102050 [details]
Patch

Clearing flags on attachment: 102050

Committed r91880: <http://trac.webkit.org/changeset/91880>

[Levi Weintraub]

All reviewed patches have been landed.  Closing bug.