Bug 64301 - Switch transform operations to FloatSize
Summary: Switch transform operations to FloatSize
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on: 64405
Blocks: 63567
  Show dependency treegraph
 
Reported: 2011-07-11 11:37 PDT by Levi Weintraub
Modified: 2011-07-27 16:18 PDT (History)
5 users (show)

See Also:


Attachments
Patch (13.54 KB, patch)
2011-07-11 11:48 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (12.10 KB, patch)
2011-07-11 12:25 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (11.01 KB, patch)
2011-07-26 14:08 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2011-07-11 11:37:33 PDT
Currently they all act on IntSize.
Comment 1 Levi Weintraub 2011-07-11 11:48:22 PDT
Created attachment 100338 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-07-11 12:05:51 PDT
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?
Comment 3 Levi Weintraub 2011-07-11 12:11:28 PDT
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.
Comment 4 Eric Seidel (no email) 2011-07-11 12:12:15 PDT
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?
Comment 5 Levi Weintraub 2011-07-11 12:20:59 PDT
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.
Comment 6 Levi Weintraub 2011-07-11 12:25:19 PDT
Created attachment 100348 [details]
Patch
Comment 7 Levi Weintraub 2011-07-12 10:29:47 PDT
(In reply to comment #6)
> Created an attachment (id=100348) [details]
> Patch

Ping for re-review :)
Comment 8 Eric Seidel (no email) 2011-07-12 11:49:43 PDT
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.
Comment 9 Darin Adler 2011-07-12 11:53:33 PDT
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.
Comment 10 Simon Fraser (smfr) 2011-07-12 11:55:20 PDT
Comment on attachment 100348 [details]
Patch

Agree with Darin.
Comment 11 Levi Weintraub 2011-07-26 14:08:36 PDT
Created attachment 102050 [details]
Patch
Comment 12 Levi Weintraub 2011-07-27 11:46:07 PDT
Can I get a review? This just works :)
Comment 13 Levi Weintraub 2011-07-27 12:28:32 PDT
Comment on attachment 102050 [details]
Patch

Thanks for the review, Simon.
Comment 14 Levi Weintraub 2011-07-27 16:18:37 PDT
Comment on attachment 102050 [details]
Patch

Clearing flags on attachment: 102050

Committed r91880: <http://trac.webkit.org/changeset/91880>
Comment 15 Levi Weintraub 2011-07-27 16:18:43 PDT
All reviewed patches have been landed.  Closing bug.