Bug 133040 – Migrate layout ascents and descents to LayoutUnits instead of ints

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Myles C. Maxfield]

Migrate layout ascents and descents to LayoutUnit instead of ints

[Myles C. Maxfield]

Created attachment 231645 [details]
Patch

[zalan]

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.

[Myles C. Maxfield]

Split out into https://bugs.webkit.org/show_bug.cgi?id=133501

[Rob Buis]

Created attachment 425084 [details]
Patch

[Rob Buis]

Created attachment 425188 [details]
Patch

[zalan]

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).

[Rob Buis]

Created attachment 425252 [details]
Patch

[EWS]

Committed r275502: <https://commits.webkit.org/r275502>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425252 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76261027>

[Rob Buis]

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.

[zalan]

(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!

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 425281 [details]
Patch

[Darin Adler]

Comment on attachment 425281 [details]
Patch

What about 0_lu vs. LayoutUnit() ?

[Rob Buis]

Created attachment 434520 [details]
Patch

[Rob Buis]

Created attachment 434530 [details]
Patch

[Darin Adler]

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?

[Said Abou-Hallawa]

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);

[Myles C. Maxfield]

Very exciting!!!

[Rob Buis]

Created attachment 434697 [details]
Patch

[Rob Buis]

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.

[Rob Buis]

Created attachment 434700 [details]
Patch

[Rob Buis]

Created attachment 434715 [details]
Patch

[Rob Buis]

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.

[EWS]

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].