Bug 203848

Created attachment 382818 [details] Patch

Created attachment 382898 [details] Patch

Comment on attachment 382898 [details] 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?

(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 382898 [details] > 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? That is quite common in CSSPropertyParser. Some functions, like consumeFunction, increase the token iterator of the range while parsing the range. The copy allows parsing the range and, if parsing of the sub-range fails, the same range can be consumed by another function changing the range. If successful, the original range gets replaced by the copy. > > > 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; This is from the same FIXME as for the shape-outside property. Sadly there is not much to share between the two. I think the idea is to use CSSValue, CSSValueList, CSSPrimitiveValue as much as possible and not to create custom CSSValues (which CSSBasicShape is). > > 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 Ok. But that will mean changing the runtime flag to cssMotionPath (instead of motion) as well since code gets generates. > > > Source/WebCore/rendering/style/RenderStyle.h:648 > > + const Length& offsetDistance() const { return m_rareNonInheritedData->offsetDistance; } > > You can return a Length by value. Length is special like a few other value types and used the same way in more than 70 cases. > > > 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. See above. > > > 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? The offset-* properties do not affect layout: The offset-path property defines possible definitions of an element along a given path. The offset-distance property defines a position on this path. The offset-rotate property defines the rotation of the object on the path at the specified position via offset-distance. The idea is the following: A user animates the offset-distance property and the element will transition along the path specified for offset-path. In a way, offset is just a different kind of transform. As a matter of fact, I intend to implement it as a transform initially: This is platform independent and will provide a way to performance test the offset implementation.

Created attachment 383124 [details] Patch

> > > > > 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? > > The offset-* properties do not affect layout: I mean the in-memory layout of StyleRareNonInheritedData Try ./Tools/Scripts/dump-class-layout on a release build.

+303 < 1> [93m<PADDING: 1 byte>[0m +304 < 8> [94mWTF::RefPtr<WebCore::BasicShape, WTF::DumbPtrTraits<WebCore::BasicShape> >[0m offsetPath +304 < 8> WTF::DumbPtrTraits<WebCore::BasicShape>::StorageType m_ptr +312 < 8> [94mWebCore::Length[0m offsetDistance +312 < 4> WebCore::Length::(anonymous union) None +316 < 1> bool m_hasQuirk +317 < 1> unsigned char m_type +318 < 1> bool m_isFloat +319 < 1> [93m<PADDING: 1 byte>[0m ... Total byte size: 496 Total pad bytes: 55 Padding percentage: 11.09 %

Ping

I think a first step before considering this patch would be to import the css/motion WPT tests with baseline results before any experimental support for the new CSS properties related to Motion Path. If the parsing WPT tests don't support the full breadth of testing added in the patch, we should make sure the WPT tests are updated to include them. As for this patch, I think the run-time flag should be made a setting, which as I understand it is the recommended way to add an experimental flag that doesn't require being checked across processes. I added one recently for the support of individual transform operations, see CSSIndividualTransformPropertiesEnabled in WebPreferencesExperimental.yaml.

Work is being done via bugs 232760, 231801, 231491.