Bug 93037 - Remove dependency on LayoutTypes.h from transformation code
Summary: Remove dependency on LayoutTypes.h from transformation code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks: 93050
  Show dependency treegraph
 
Reported: 2012-08-02 15:50 PDT by Emil A Eklund
Modified: 2012-08-03 15:00 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.32 KB, patch)
2012-08-02 15:53 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (3.34 KB, patch)
2012-08-02 15:55 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (6.57 KB, patch)
2012-08-02 16:29 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (7.35 KB, patch)
2012-08-02 17:03 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (7.30 KB, patch)
2012-08-03 11:54 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.