Bug 133040 - Migrate layout ascents and descents to LayoutUnits instead of ints
Summary: Migrate layout ascents and descents to LayoutUnits instead of ints
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-05-17 19:48 PDT by Myles C. Maxfield
Modified: 2021-08-01 21:56 PDT (History)
14 users (show)

See Also:


Attachments
Patch (32.05 KB, patch)
2014-05-17 20:17 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (11.23 KB, patch)
2021-04-03 01:47 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.31 KB, patch)
2021-04-05 12:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.37 KB, patch)
2021-04-06 00:37 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.64 KB, patch)
2021-04-06 08:27 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2021-07-29 07:32 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.71 KB, patch)
2021-07-29 09:50 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.49 KB, patch)
2021-07-31 07:04 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2021-07-31 09:55 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.60 KB, patch)
2021-08-01 02:27 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-05-17 19:48:28 PDT
Migrate layout ascents and descents to LayoutUnit instead of ints
Comment 1 Myles C. Maxfield 2014-05-17 20:17:29 PDT
Created attachment 231645 [details]
Patch
Comment 2 zalan 2014-05-17 21:53:15 PDT
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.
Comment 3 Myles C. Maxfield 2014-06-03 22:08:03 PDT
Split out into https://bugs.webkit.org/show_bug.cgi?id=133501
Comment 4 Rob Buis 2021-04-03 01:47:59 PDT
Created attachment 425084 [details]
Patch
Comment 5 Rob Buis 2021-04-05 12:40:32 PDT
Created attachment 425188 [details]
Patch
Comment 6 zalan 2021-04-05 12:53:44 PDT
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).
Comment 7 Rob Buis 2021-04-06 00:37:22 PDT
Created attachment 425252 [details]
Patch
Comment 8 EWS 2021-04-06 03:49:38 PDT
Committed r275502: <https://commits.webkit.org/r275502>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425252 [details].
Comment 9 Radar WebKit Bug Importer 2021-04-06 03:50:15 PDT
<rdar://problem/76261027>
Comment 10 Rob Buis 2021-04-06 07:41:37 PDT
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.
Comment 11 zalan 2021-04-06 07:53:44 PDT
(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!
Comment 12 Rob Buis 2021-04-06 08:27:18 PDT
Reopening to attach new patch.
Comment 13 Rob Buis 2021-04-06 08:27:21 PDT
Created attachment 425281 [details]
Patch
Comment 14 Darin Adler 2021-07-26 14:11:19 PDT
Comment on attachment 425281 [details]
Patch

What about 0_lu vs. LayoutUnit() ?
Comment 15 Rob Buis 2021-07-29 07:32:43 PDT
Created attachment 434520 [details]
Patch
Comment 16 Rob Buis 2021-07-29 09:50:57 PDT
Created attachment 434530 [details]
Patch
Comment 17 Darin Adler 2021-07-29 10:36:46 PDT
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 18 Said Abou-Hallawa 2021-07-29 11:08:39 PDT
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);
Comment 19 Myles C. Maxfield 2021-07-29 11:37:30 PDT
Very exciting!!!
Comment 20 Rob Buis 2021-07-31 07:04:04 PDT
Created attachment 434697 [details]
Patch
Comment 21 Rob Buis 2021-07-31 09:18:13 PDT
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.
Comment 22 Rob Buis 2021-07-31 09:55:02 PDT
Created attachment 434700 [details]
Patch
Comment 23 Rob Buis 2021-08-01 02:27:06 PDT
Created attachment 434715 [details]
Patch
Comment 24 Rob Buis 2021-08-01 13:45:52 PDT
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.
Comment 25 EWS 2021-08-01 21:56:07 PDT
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].