Bug 222665 - Allow CSS font-styling for canvas without RenderStyle
Summary: Allow CSS font-styling for canvas without RenderStyle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on:
Blocks: 202793
  Show dependency treegraph
 
Reported: 2021-03-03 10:46 PST by Chris Lord
Modified: 2021-03-05 07:06 PST (History)
11 users (show)

See Also:


Attachments
Patch (30.15 KB, patch)
2021-03-03 10:49 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (30.33 KB, patch)
2021-03-03 11:37 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (30.59 KB, patch)
2021-03-04 02:32 PST, Chris Lord
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (32.20 KB, patch)
2021-03-04 06:40 PST, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (32.24 KB, patch)
2021-03-05 01:38 PST, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2021-03-03 10:46:26 PST
Currently, resolveForFontRaw requires RenderStyle, not for any good reason beyond the functions it needs to use expect it. It would be good to refactor and remove that dependency to make it easier for Workers in the future. (I suppose it's also a potential memory/perf improvement)
Comment 1 Chris Lord 2021-03-03 10:49:49 PST
Created attachment 422118 [details]
Patch
Comment 2 Chris Lord 2021-03-03 11:37:21 PST
Created attachment 422128 [details]
Patch
Comment 3 Chris Lord 2021-03-04 02:32:06 PST
Created attachment 422208 [details]
Patch
Comment 4 Chris Lord 2021-03-04 06:40:11 PST
Created attachment 422222 [details]
Patch
Comment 5 Darin Adler 2021-03-04 13:39:32 PST
Comment on attachment 422222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422222&action=review

> Source/WebCore/css/CSSPrimitiveValue.h:194
> +    static double computeUnzoomedNonCalcLengthDouble(CSSUnitType, double value, CSSPropertyID, const FontMetrics* = nullptr, const FontCascadeDescription* = nullptr, const FontCascadeDescription* rootFontDescription = nullptr, const RenderView* = nullptr);

I worry about the long list of arguments. Is there any way for us to cut that complexity down to size? This is why we end up creating objects to group the context for calls like this.

Also wonder if there’s a more terse way to name these variations.

> Source/WebCore/css/CSSToLengthConversionData.h:78
> +    CSSPropertyID propertyToCompute() const { return m_propertyToCompute ? *m_propertyToCompute : CSSPropertyInvalid; }

Can use valueOr for this sort of thing.

> Source/WebCore/style/StyleFontSizeFunctions.h:28
> +#include "Settings.h"

Really annoying that Settings::Values creates these additional header dependencies. I think we should consider defining SettingsValues at the namespace level to avoid the need for all these additional header includes.

> Source/WebCore/style/StyleFontSizeFunctions.h:41
> +enum class MinimumFontSizeRule {
> +    None,
> +    Absolute,
> +    AbsoluteAndRelative
> +};

Comment on "just moved" code: I may be the only person who feels this way, but I like the one-line versions of this kind of small enum rather than the vertical version.

    enum class MinimumFontSizeRule : uint8_t { None, Absolute, AbsoluteAndRelative };

It’s also a tiny bit helpful, I think, to mark the underlying type uint8_t rather than letting it default to signed 32-bit integer.

> Source/WebCore/style/StyleResolveForFontRaw.cpp:202
> +    // Line height is non-existent on FontCascade, so is left unhandled.

Is there a slightly more precise way to say what we mean by "left unhandled"?
Comment 6 Chris Lord 2021-03-05 01:38:41 PST
Created attachment 422350 [details]
Patch
Comment 7 Chris Lord 2021-03-05 01:44:51 PST
(In reply to Darin Adler from comment #5)
> > Source/WebCore/css/CSSPrimitiveValue.h:194
> > +    static double computeUnzoomedNonCalcLengthDouble(CSSUnitType, double value, CSSPropertyID, const FontMetrics* = nullptr, const FontCascadeDescription* = nullptr, const FontCascadeDescription* rootFontDescription = nullptr, const RenderView* = nullptr);
> 
> I worry about the long list of arguments. Is there any way for us to cut
> that complexity down to size? This is why we end up creating objects to
> group the context for calls like this.
> 
> Also wonder if there’s a more terse way to name these variations.

I didn't address this - I think ideally, CSSToLengthConversionData would have another constructor that would take these arguments and accessors for them that could use them or defer to the style members... But that's a bigger change than I'd like to make at this stage and I think doing that nicely takes a bit more thought than a quick change on this patch.

> > Source/WebCore/css/CSSToLengthConversionData.h:78
> > +    CSSPropertyID propertyToCompute() const { return m_propertyToCompute ? *m_propertyToCompute : CSSPropertyInvalid; }
> 
> Can use valueOr for this sort of thing.

Thanks, fixed.

> > Source/WebCore/style/StyleFontSizeFunctions.h:28
> > +#include "Settings.h"
> 
> Really annoying that Settings::Values creates these additional header
> dependencies. I think we should consider defining SettingsValues at the
> namespace level to avoid the need for all these additional header includes.

I agree, I've opened bug 222788.

> > Source/WebCore/style/StyleFontSizeFunctions.h:41
> > +enum class MinimumFontSizeRule {
> > +    None,
> > +    Absolute,
> > +    AbsoluteAndRelative
> > +};
> 
> Comment on "just moved" code: I may be the only person who feels this way,
> but I like the one-line versions of this kind of small enum rather than the
> vertical version.
> 
>     enum class MinimumFontSizeRule : uint8_t { None, Absolute,
> AbsoluteAndRelative };
> 
> It’s also a tiny bit helpful, I think, to mark the underlying type uint8_t
> rather than letting it default to signed 32-bit integer.

Agreed on both counts and so I made the change. I'll wait for some green before cq+ in case there was some dubious casting going on that this breaks.

> > Source/WebCore/style/StyleResolveForFontRaw.cpp:202
> > +    // Line height is non-existent on FontCascade, so is left unhandled.
> 
> Is there a slightly more precise way to say what we mean by "left unhandled"?

I've updated this comment, "left unhandled" is indeed too vague. This now reads:

    // As there is no line-height on FontCascade, there's no need to resolve
    // it, even though there is line-height information on FontRaw.
Comment 8 EWS 2021-03-05 04:20:01 PST
Found 1 new test failure: platform/mac/media/encrypted-media/fps-encrypted-event.html
Comment 9 Chris Lord 2021-03-05 06:40:30 PST
Comment on attachment 422350 [details]
Patch

I don't think the failure has anything to do with this patch, trying again.
Comment 10 EWS 2021-03-05 07:05:12 PST
Committed r273964: <https://commits.webkit.org/r273964>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422350 [details].
Comment 11 Radar WebKit Bug Importer 2021-03-05 07:06:23 PST
<rdar://problem/75093446>