[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