Bug 93037

Summary: Remove dependency on LayoutTypes.h from transformation code
Product: WebKit Reporter: Emil A Eklund <eae>
Component: PlatformAssignee: Emil A Eklund <eae>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jamesr, leviw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 93050    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Emil A Eklund 2012-08-02 15:50:30 PDT
TransformationMatrix includes LayoutTypes.h which is in rendering, this is a layering violation and makes using the it outside of rendering complicated.
Comment 1 Emil A Eklund 2012-08-02 15:53:49 PDT
Created attachment 156185 [details]
Patch
Comment 2 Emil A Eklund 2012-08-02 15:55:49 PDT
Created attachment 156187 [details]
Patch
Comment 3 Levi Weintraub 2012-08-02 15:56:55 PDT
Comment on attachment 156187 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        TransformationMatrix includes LayoutTypes.h which is in rendering,
> +        this is a layering violation. Remove this dependency as it is no longer

Grammar Levi says: This is a run on. Period or conjunction needed :p
Comment 4 Emil A Eklund 2012-08-02 16:00:23 PDT
We'll also have to change it not to use MAX_LAYOUT_UNIT...
Comment 5 Emil A Eklund 2012-08-02 16:29:55 PDT
Created attachment 156198 [details]
Patch
Comment 6 Levi Weintraub 2012-08-02 16:38:42 PDT
Comment on attachment 156198 [details]
Patch

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

> Source/WebCore/platform/FractionalLayoutUnit.h:188
> +    static FractionalLayoutUnit clamp(double value)
> +    {
> +        return clampTo<FractionalLayoutUnit>(value, FractionalLayoutUnit::min(), FractionalLayoutUnit::max());
> +    }

I don't like the duplication of this code here and in LayoutTypes.h. Seems like if this is where this needs to live to prevent layering problems, we should eliminate the version in LayoutTypes.
Comment 7 Emil A Eklund 2012-08-02 17:03:07 PDT
Created attachment 156211 [details]
Patch
Comment 8 Levi Weintraub 2012-08-02 17:07:55 PDT
Comment on attachment 156211 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        * rendering/LayoutTypes.h:
> +        Remove clampToLayoutUnit
> +        

We don't usually put whitespace here, and this could just be one line.

> Source/WebCore/ChangeLog:21
> +        Add FractionalLayoutUnit::clamp method that clamps a double to a
> +        FractionalLayoutUnit.
> +        

Ditto.

> Source/WebCore/ChangeLog:27
> +        Use clamp/max/min from FractionalLayoutUnit instead of going through
> +        LayoutUnit abstraction.
> +        

Ditto.
Comment 9 Emil A Eklund 2012-08-03 11:54:28 PDT
Created attachment 156427 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-08-03 15:00:24 PDT
Comment on attachment 156427 [details]
Patch for landing

Clearing flags on attachment: 156427

Committed r124658: <http://trac.webkit.org/changeset/124658>
Comment 11 WebKit Review Bot 2012-08-03 15:00:28 PDT
All reviewed patches have been landed.  Closing bug.