Bug 83726

Summary: Prepare functions in LengthFunctions.h for LayoutUnits
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: eae, eric, jchaffraix, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Levi Weintraub
Reported 2012-04-11 16:00:41 PDT
This also entails some static_casts in the few integer-based rendering types. Details in the changelog.
Attachments
Patch (7.88 KB, patch)
2012-04-11 16:24 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (7.83 KB, patch)
2012-04-11 17:16 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (10.92 KB, patch)
2012-04-12 10:52 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (12.46 KB, patch)
2012-04-12 11:42 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Levi Weintraub
Comment 1 2012-04-11 16:24:43 PDT
Created attachment 136781 [details] Patch
Eric Seidel (no email)
Comment 2 2012-04-11 16:28:03 PDT
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 3 2012-04-11 16:30:22 PDT
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
Comment 4 2012-04-11 17:16:23 PDT
Created attachment 136793 [details] Patch
Levi Weintraub
Comment 5 2012-04-12 10:19:04 PDT
Ping
Julien Chaffraix
Comment 6 2012-04-12 10:33:05 PDT
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
Comment 7 2012-04-12 10:52:20 PDT
Created attachment 136932 [details] Patch
Eric Seidel (no email)
Comment 8 2012-04-12 11:01:09 PDT
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 9 2012-04-12 11:03:57 PDT
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
Comment 10 2012-04-12 11:07:08 PDT
(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
Comment 11 2012-04-12 11:42:12 PDT
Created attachment 136944 [details] Patch
Levi Weintraub
Comment 12 2012-04-12 13:47:26 PDT
(In reply to comment #11) > Created an attachment (id=136944) [details] > Patch I think I finally got this covered :)
Eric Seidel (no email)
Comment 13 2012-04-12 20:33:47 PDT
Comment on attachment 136944 [details] Patch LGTM.
WebKit Review Bot
Comment 14 2012-04-12 21:19:58 PDT
Comment on attachment 136944 [details] Patch Clearing flags on attachment: 136944 Committed r114079: <http://trac.webkit.org/changeset/114079>
WebKit Review Bot
Comment 15 2012-04-12 21:20:03 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.