Bug 83491

Summary: Prepare html classes for sub-pixel LayoutUnits
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: eae, eric.carlson, eric, feature-media-reviews, jchaffraix, tkent, 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

Levi Weintraub
Reported 2012-04-09 11:40:24 PDT
This eliminates remaining diffs in html/ with the subpixellayout branch.
Attachments
Patch (7.42 KB, patch)
2012-04-09 11:59 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (4.37 KB, patch)
2012-04-09 13:34 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Patch (4.38 KB, patch)
2012-04-09 13:43 PDT, Levi Weintraub 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Levi Weintraub
Comment 1 2012-04-09 11:59:02 PDT
Created attachment 136279 [details] Patch
Eric Seidel (no email)
Comment 2 2012-04-09 13:14:18 PDT
Comment on attachment 136279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136279&action=review > Source/WebCore/html/ValidationMessage.cpp:123 > + bubble->setInlineStyleProperty(CSSPropertyTop, hostY + static_cast<double>(hostRect.height()), CSSPrimitiveValue::CSS_PX); Why the double casts?
Levi Weintraub
Comment 3 2012-04-09 13:21:29 PDT
Comment on attachment 136279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136279&action=review >> Source/WebCore/html/ValidationMessage.cpp:123 >> + bubble->setInlineStyleProperty(CSSPropertyTop, hostY + static_cast<double>(hostRect.height()), CSSPrimitiveValue::CSS_PX); > > Why the double casts? You're right, this can be removed. We were just missing a double flavor of operator+ in FractionalLayoutUnit. Will upload again.
Levi Weintraub
Comment 4 2012-04-09 13:34:46 PDT
Created attachment 136295 [details] Patch
Eric Seidel (no email)
Comment 5 2012-04-09 13:37:00 PDT
Comment on attachment 136295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136295&action=review > Source/WebCore/platform/Length.h:127 > + // FIXME: When we switch to sub-pixel layout, value will return float by default, and this will > + // no longer simply return value(). > + int intValue() const > + { > + return value(); > + } > + What will it return? static_cast<int>(value())? round(value())? Can we make it return whatever that is now? Or add such in a comment or something?
Levi Weintraub
Comment 6 2012-04-09 13:40:35 PDT
Comment on attachment 136295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136295&action=review >> Source/WebCore/platform/Length.h:127 >> + > > What will it return? static_cast<int>(value())? round(value())? Can we make it return whatever that is now? Or add such in a comment or something? Sorry, my comment was better in the changelog than the code. This will simply inherit the current logic of value().
Levi Weintraub
Comment 7 2012-04-09 13:43:50 PDT
Created attachment 136299 [details] Patch
Levi Weintraub
Comment 8 2012-04-09 14:44:29 PDT
I hope I've cleared up the issues you called out. I'd love another look :)
Eric Seidel (no email)
Comment 9 2012-04-09 14:51:35 PDT
Comment on attachment 136299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136299&action=review > Source/WebCore/platform/Length.h:122 > + // FIXME: When we switch to sub-pixel layout, value will return float by default, and this will inherit > + // the current implementation of value(). Were you going to change this?
Eric Seidel (no email)
Comment 10 2012-04-09 14:52:14 PDT
Comment on attachment 136299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136299&action=review >> Source/WebCore/platform/Length.h:122 >> + // the current implementation of value(). > > Were you going to change this? Oh, I guess you did. Anyway, the switch is very soon, so this is fine regardless.
Levi Weintraub
Comment 11 2012-04-09 15:10:25 PDT
Comment on attachment 136299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136299&action=review >>> Source/WebCore/platform/Length.h:122 >>> + // the current implementation of value(). >> >> Were you going to change this? > > Oh, I guess you did. Anyway, the switch is very soon, so this is fine regardless. Thanks for the review!
WebKit Review Bot
Comment 12 2012-04-09 17:03:19 PDT
Comment on attachment 136299 [details] Patch Clearing flags on attachment: 136299 Committed r113645: <http://trac.webkit.org/changeset/113645>
WebKit Review Bot
Comment 13 2012-04-09 17:03:24 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.