[webkit-reviews] review denied: [Bug 203848] Support parsing, computed style and property style for offset, offset-path, offset-distance : [Attachment 382898] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 7 13:02:31 PST 2019


Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 203848: Support parsing, computed style and property style for offset,
offset-path, offset-distance
https://bugs.webkit.org/show_bug.cgi?id=203848

Attachment 382898: Patch

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




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 382898
  --> https://bugs.webkit.org/attachment.cgi?id=382898
Patch

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

> Source/WebCore/ChangeLog:11
> +	   This patch implements the parsing, computed style and property style
> +	   for the
> +	   longhand properties: offset-path, offset-distance
> +	   shorthand properties: offset

Weird wrapping

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2542
> +    CSSParserTokenRange rangeCopy = range;
> +    CSSParserTokenRange args = consumeFunction(rangeCopy);

What's happening with rangeCopy?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2544
> +    // FIXME-NEWPARSER: CSSBasicShape should be a CSSValue, and shapes
should not be primitive values.

Why the FIXME-NEWPARSER in new code?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2550
> +    range = rangeCopy;

And here.

> Source/WebCore/page/RuntimeEnabledFeatures.h:184
> +    void setMotionEnabled(bool isEnabled) { m_motionEnabled = isEnabled; }
> +    bool motionEnabled() const { return m_motionEnabled; }
> +

Let's call this CSSMotionPath. so setCSSMotionPathEnabled etc

> Source/WebCore/rendering/style/RenderStyle.h:648
> +    const Length& offsetDistance() const { return
m_rareNonInheritedData->offsetDistance; }

You can return a Length by value.

> Source/WebCore/rendering/style/RenderStyle.h:1175
> +    void setOffsetDistance(Length&& offsetDistance) {
SET_VAR(m_rareNonInheritedData, offsetDistance, WTFMove(offsetDistance)); }

No point using r-value ref for a Length I think.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:148
> +    RefPtr<BasicShape> offsetPath;
> +    Length offsetDistance;

How does this affect padding in the class? Is there a more optimal layout?


More information about the webkit-reviews mailing list