Summary: | Migrate layout ascents and descents to LayoutUnits instead of ints | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, changseok, commit-queue, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, rbuis, sabouhallawa, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2014-05-17 19:48:28 PDT
Created attachment 231645 [details]
Patch
Comment on attachment 231645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231645&action=review > Source/WebCore/rendering/InlineFlowBox.cpp:872 > + LayoutUnit strokeOverflow = static_cast<int>(ceilf(lineStyle.textStrokeWidth() / 2.0f)); missing FIXME here. In general, I transition to LayoutUntits from int/float when I change the functionality as well. int type is a straightforward indication that a particular function/class has not been converted yet to subpixel. Functions with integral LayoutUnits tricked me a few times. Split out into https://bugs.webkit.org/show_bug.cgi?id=133501 Created attachment 425084 [details]
Patch
Created attachment 425188 [details]
Patch
Comment on attachment 425188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425188&action=review > Source/WebCore/rendering/RootInlineBox.cpp:819 > + LayoutUnit usedFontAscent = fontMetrics.ascent(baselineType()); It's up to you but as I mentioned in the previous patch I'd prefer uniform initialization (and mention in the changelog that ascent/descent values are still integral). Created attachment 425252 [details]
Patch
Committed r275502: <https://commits.webkit.org/r275502> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425252 [details]. Comment on attachment 425188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425188&action=review >> Source/WebCore/rendering/RootInlineBox.cpp:819 >> + LayoutUnit usedFontAscent = fontMetrics.ascent(baselineType()); > > It's up to you but as I mentioned in the previous patch I'd prefer uniform initialization (and mention in the changelog that ascent/descent values are still integral). Sure, done. (In reply to Rob Buis from comment #10) > Comment on attachment 425188 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425188&action=review > > >> Source/WebCore/rendering/RootInlineBox.cpp:819 > >> + LayoutUnit usedFontAscent = fontMetrics.ascent(baselineType()); > > > > It's up to you but as I mentioned in the previous patch I'd prefer uniform initialization (and mention in the changelog that ascent/descent values are still integral). > > Sure, done. Thanks! Reopening to attach new patch. Created attachment 425281 [details]
Patch
Comment on attachment 425281 [details]
Patch
What about 0_lu vs. LayoutUnit() ?
Created attachment 434520 [details]
Patch
Created attachment 434530 [details]
Patch
Comment on attachment 434530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434530&action=review > Source/WebCore/ChangeLog:8 > + Migrate GlyphOverflow to LayoutUnits instead of ints. This is missing rationale. Why are we doing this if it has no detectable effect? Does it make some future change possible? Does it make something more efficient? Does it make code more elegant? If it does have a good effect, then why no tests? > Source/WebCore/rendering/LegacyInlineFlowBox.cpp:936 > + LayoutUnit topGlyphEdge = glyphOverflow ? (isFlippedLine ? glyphOverflow->bottom : glyphOverflow->top) : 0_lu; Is it critical to use LayoutUnit rather than auto in these places? I ask because "staying as a LayoutUnit" is one kind of thing. Converting to LayoutUnit is another. Maybe this is conversion to LayoutUnit? Comment on attachment 434530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434530&action=review > Source/WebCore/style/InlineTextBoxStyle.cpp:100 > -static inline void extendIntToFloat(int& extendMe, float extendTo) > +static inline void extendIntToFloat(LayoutUnit& extendMe, float extendTo) This name does not make sense now. We are extending 'LayoutUnit' not 'int'. But I think this function should be written as two methods of GlyphOverflow since it is only called for the 'top' and 'bottom' members of GlyphOverflow. struct GlyphOverflow { void extendTop(float extendTo) { top = ... } void extendBottom(float extendTo) { bottom = ... } }; And instead of calling: extendIntToFloat(overflowResult.top, -underlineOffset); We can say: overflowResult.extendTop(-underlineOffset); Very exciting!!! Created attachment 434697 [details]
Patch
Comment on attachment 434530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434530&action=review >> Source/WebCore/style/InlineTextBoxStyle.cpp:100 >> +static inline void extendIntToFloat(LayoutUnit& extendMe, float extendTo) > > This name does not make sense now. We are extending 'LayoutUnit' not 'int'. > > But I think this function should be written as two methods of GlyphOverflow since it is only called for the 'top' and 'bottom' members of GlyphOverflow. > > struct GlyphOverflow { > void extendTop(float extendTo) > { > top = ... > } > > void extendBottom(float extendTo) > { > bottom = ... > } > }; > > And instead of calling: > extendIntToFloat(overflowResult.top, -underlineOffset); > > We can say: > overflowResult.extendTop(-underlineOffset); I like it! Done. Created attachment 434700 [details]
Patch
Created attachment 434715 [details]
Patch
Comment on attachment 434530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434530&action=review >> Source/WebCore/ChangeLog:8 >> + Migrate GlyphOverflow to LayoutUnits instead of ints. > > This is missing rationale. > > Why are we doing this if it has no detectable effect? Does it make some future change possible? Does it make something more efficient? Does it make code more elegant? If it does have a good effect, then why no tests? I added a bit to the changelog. >> Source/WebCore/rendering/LegacyInlineFlowBox.cpp:936 >> + LayoutUnit topGlyphEdge = glyphOverflow ? (isFlippedLine ? glyphOverflow->bottom : glyphOverflow->top) : 0_lu; > > Is it critical to use LayoutUnit rather than auto in these places? I ask because "staying as a LayoutUnit" is one kind of thing. Converting to LayoutUnit is another. Maybe this is conversion to LayoutUnit? I think auto is fine indeed since most of these are not conversions, just assignments with expressions all strictly dealing with LayoutUnits. Fixed. Committed r280525 (240157@main): <https://commits.webkit.org/240157@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434715 [details]. |