Prepare functions in LengthFunctions.h for LayoutUnits

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

This also entails some static_casts in the few integer-based rendering types. Details in the changelog.

[Levi Weintraub]

Created attachment 136781 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 136781 [details]
Patch

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

> Source/WebCore/rendering/RenderFrameSet.cpp:280
>                  remainingLen -= gridLayout[i];

is this a length?  Should this use .intValue() instead?

[Levi Weintraub]

Comment on attachment 136781 [details]
Patch

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

>> Source/WebCore/rendering/RenderFrameSet.cpp:280
>>                  remainingLen -= gridLayout[i];
> 
> is this a length?  Should this use .intValue() instead?

You are so right :)

[Levi Weintraub]

Created attachment 136793 [details]
Patch

[Levi Weintraub]

Ping

[Julien Chaffraix]

Comment on attachment 136793 [details]
Patch

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

The change is fine, one question.

> Source/WebCore/css/LengthFunctions.h:36
> +LayoutUnit minimumValueForLength(const Length&, LayoutUnit maximumValue, RenderView* = 0, bool roundPercentages = false);
> +LayoutUnit valueForLength(const Length&, LayoutUnit maximumValue, RenderView* = 0, bool roundPercentages = false);
> +float floatValueForLength(const Length&, LayoutUnit maximumValue, RenderView* = 0);

Why don't you also update the implementation of those functions as part of this change?

[Levi Weintraub]

Created attachment 136932 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 136932 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:1855
> +    marginTop = style->marginTop().isAuto() ? marginTop : static_cast<int>(valueForLength(style->marginTop(), width, view));
> +    marginRight = style->marginRight().isAuto() ? marginRight : static_cast<int>(valueForLength(style->marginRight(), width, view));
> +    marginBottom = style->marginBottom().isAuto() ? marginBottom : static_cast<int>(valueForLength(style->marginBottom(), width, view));
> +    marginLeft = style->marginLeft().isAuto() ? marginLeft : static_cast<int>(valueForLength(style->marginLeft(), width, view));

Seems like you want intValidForLength now?  Which at least does this wrapping/casting for you?

[Julien Chaffraix]

Comment on attachment 136932 [details]
Patch

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

> Source/WebCore/css/LengthFunctions.cpp:47
>          return 0;

There should be no 0 but ZERO_LAYOUT_UNIT for all the functions touched and that now returns a LayoutUnit.

[Levi Weintraub]

(In reply to comment #8)
> (From update of attachment 136932 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136932&action=review
> 
> > Source/WebCore/dom/Document.cpp:1855
> > +    marginTop = style->marginTop().isAuto() ? marginTop : static_cast<int>(valueForLength(style->marginTop(), width, view));
> > +    marginRight = style->marginRight().isAuto() ? marginRight : static_cast<int>(valueForLength(style->marginRight(), width, view));
> > +    marginBottom = style->marginBottom().isAuto() ? marginBottom : static_cast<int>(valueForLength(style->marginBottom(), width, view));
> > +    marginLeft = style->marginLeft().isAuto() ? marginLeft : static_cast<int>(valueForLength(style->marginLeft(), width, view));
> 
> Seems like you want intValidForLength now?  Which at least does this wrapping/casting for you?

I like that suggestion. I'll get on that momentarily

[Levi Weintraub]

Created attachment 136944 [details]
Patch

[Levi Weintraub]

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

I think I finally got this covered :)

[Eric Seidel (no email)]

Comment on attachment 136944 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 136944 [details]
Patch

Clearing flags on attachment: 136944

Committed r114079: <http://trac.webkit.org/changeset/114079>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.