[webkit-reviews] review granted: [Bug 204575] Enable full CSS color parsing within a worker for OffscreenCanvas : [Attachment 408810] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 204575: Enable full CSS color parsing within a worker for OffscreenCanvas
https://bugs.webkit.org/show_bug.cgi?id=204575

Attachment 408810: Patch

https://bugs.webkit.org/attachment.cgi?id=408810&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 408810
  --> https://bugs.webkit.org/attachment.cgi?id=408810
Patch

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

Approach looks fine, but there is too much code duplication and we could
improve the style of the new code, especially with how it uses return value.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:131
> +    bool consumeDoubleRaw(double& result)

The naming here is inconsistent with that the naming above. The functions above
name what they consume, not the type they convert to. Maybe consumeValueRaw?

Also, I suggest using Optional<double> instead of an out argument (even though
consumeNumberRaw doesn’t do that). The consumeNumberRaw function was written a
long time ago and is using an archaic style.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:309
> +bool consumePercentRaw(CSSParserTokenRange& range, double& result)

Optional<double> return value.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:330
>  RefPtr<CSSPrimitiveValue> consumePercent(CSSParserTokenRange& range,
ValueRange valueRange)

Can this call the new consumePercentRaw?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:383
> +CSSUnitType consumeAngleRaw(CSSParserTokenRange& range, CSSParserMode
cssParserMode, UnitlessQuirk unitless, double& result)

Return a structure that contains both the result and its units, do not use an
out argument for one and a return value for the other.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:418
>  RefPtr<CSSPrimitiveValue> consumeAngle(CSSParserTokenRange& range,
CSSParserMode cssParserMode, UnitlessQuirk unitless)

I’m concerned that we are repeating too much code; I’d like this function to
simply call the other.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:600
> +    bool isPercentage = false;
> +    double colorParameter;

We should consider putting these in a structure so we can make helper functions
and don’t need so many if statements and extra arguments.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:692
> +    double angleInDegrees;
> +    switch (unitType) {
> +    case CSSUnitType::CSS_DEG:
> +	   angleInDegrees = value;
> +	   break;
> +    case CSSUnitType::CSS_RAD:
> +	   angleInDegrees = rad2deg(value);
> +	   break;
> +    case CSSUnitType::CSS_GRAD:
> +	   angleInDegrees = grad2deg(value);
> +	   break;
> +    case CSSUnitType::CSS_TURN:
> +	   angleInDegrees = turn2deg(value);
> +	   break;
> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   return Color();
> +    }

Need to find a way to share this code with computeDegrees function instead of
having it twice.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:836
> +bool consumeColorWorkerSafe(CSSParserTokenRange& range, CSSParserMode
cssParserMode, Color& result)

Should return Color and use "invalid" as the failure value as we would use
nullptr or nullopt elsewhere.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:840
> +	   // FIXME: System colours are not worker-safe

I think you mean something more like this:

    // FIXME: Need a worker-safe way to compute the system colors. For now, we
detect the system color, but then intentionally fail parsing.