Bug 229702

Created attachment 436858 [details] Patch

Comment on attachment 436858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436858&action=review Looks like some test expectations need updating. Looks like a progression. > Source/WebCore/css/CSSVariableReferenceValue.h:64 > + // FIXME: is friend the best solution? > + friend class CSSStyleValue; It isn't. A better solution would probably be to expose public functions to do what CSSStyleValue needs to do. > Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:75 > + m_fallback->serialize(builder); nice! > Source/WebCore/css/typedom/CSSStyleImageValue.h:57 > + CSSStyleImageValue(Ref<CSSImageValue>&&, Document* = nullptr); It looks like the default value is unneeded because only one call site has nullptr. Also, it looks like the Document is used by the CanvasRenderingContext2DBase::drawImage which takes a CSSStyleImageValue&, and having a null Document would make this unable to be drawn. I'm not sure what the consequences of that would be, and I'm not sure how finished CanvasRenderingContext2DBase::drawImage is. It might be worth a comment saying that the CSSStyleImageValue created with no document currently can't be used for drawing. > Source/WebCore/css/typedom/CSSStyleValue.cpp:51 > +ExceptionOr<Vector<Ref<CSSStyleValue>>> CSSStyleValue::parseStyleValue(const String& cssProperty, const String& cssText, bool) Ah, I see the comment below. parseMultiple will be brought back. > Source/WebCore/css/typedom/CSSStyleValue.cpp:80 > + nit: unnecessary space addition. > Source/WebCore/css/typedom/CSSStyleValue.cpp:95 > + // FIXME: Add Reification control flow. Returns nullptr if failed We can't return nullptr here. An exception? > Source/WebCore/css/typedom/CSSStyleValue.cpp:119 > +CSSParserContext CSSStyleValue::getVariableReferenceParserContext() > +{ > + auto parserContext = CSSParserContext(HTMLStandardMode); Since this is static, do we need to make a new CSSParserContext every time it is called? Could we use NeverDestroyed and return a const CSSParserContext&? It doesn't look like the CSSParserContext stores state that we would need to reset or anything like that.

Created attachment 436978 [details] Patch

Created attachment 436979 [details] Patch

Created attachment 436997 [details] Patch

Comment on attachment 436997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436997&action=review > Source/WebCore/css/CSSVariableReferenceValue.h:49 > + CSSVariableData& data() const { return m_data.get(); } This should return a const CSSVariableData& > Source/WebCore/css/typedom/CSSStyleImageValue.h:57 > + CSSStyleImageValue(Ref<CSSImageValue>&&, Document* = nullptr); This default value of "= nullptr" should not be added. It is only called from create, which always has a value. > Source/WebCore/css/typedom/CSSStyleValue.cpp:96 > + // FIXME: Add Reification control flow. Returns default CSSStyleValue if failed. I don't think I understand why this FIXME comment is here. Is there something left to do after this change? > Source/WebCore/css/typedom/CSSStyleValue.cpp:128 > + parserContext->springTimingFunctionEnabled = true; These values are supposed to come from the Document. We shouldn't hard code them here. Is it possible to get a Document& to here using [CallWith=Document]? We would have to make this no longer a static function and not use a NeverDestroyed, but that would probably be a better design if it's possible. > Source/WebCore/css/typedom/CSSStyleValue.h:77 > + static const CSSParserContext& getVariableReferenceParserContext(); I don't think this name needs "get"

Created attachment 437123 [details] Patch

Comment on attachment 437123 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437123&action=review > Source/WebCore/css/typedom/CSSStyleValue.cpp:126 > + parserContext->springTimingFunctionEnabled = true; This should not be hard-coded. We should either get the values from a Document, or use the default value. Can't we just use strictCSSParserContext instead of all this?

Created attachment 437193 [details] Patch

Comment on attachment 437193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437193&action=review > Source/WebCore/css/typedom/CSSOMVariableReferenceValue.cpp:77 > + builder.append(")"); nit: builder.append(')'); > Source/WebCore/css/typedom/CSSStyleValue.cpp:52 > +ExceptionOr<Vector<Ref<CSSStyleValue>>> CSSStyleValue::parseStyleValue(const String& cssProperty, const String& cssText, bool) The parameter name is not obvious so I wouldn't omit it. You can use UNUSED_PARAM(parseMultiple); if necessary. > Source/WebCore/css/typedom/CSSStyleValue.cpp:102 > + if (is<CSSImageValue>(cssValue.get())) shouldn't this be `else if`? > Source/WebCore/css/typedom/CSSStyleValue.cpp:105 > + if (is<CSSVariableReferenceValue>(cssValue.get())) { shouldn't this be `else if`? > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:105 > + if (topIdentifier) { if (auto topIdentifier = identifiers.takeLast()) {

Created attachment 437207 [details] Patch

Created attachment 437236 [details] Patch

Created attachment 437243 [details] Patch

Created attachment 437255 [details] Patch

Created attachment 437322 [details] Patch

<rdar://problem/82812803>

Comment on attachment 437322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437322&action=review Great progress. Let's get this polished up and landed. > Source/WebCore/css/typedom/CSSStyleValue.cpp:75 > + auto parserContext = strictCSSParserContext(); This isn't used in this patch. Please remove it from this patch and add it when it is used. > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:66 > + if (currentToken.value() == "var") { Would it be equivalent to check .functionId() and see if it's CSSValueVar? That would probably be better than hard-coding a css keyword here. > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:79 > + // Create a new vector of CSSUnparsedSegment > + segmentStack.append({ }); I don't think this comment adds anything. > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:88 > + ASSERT_NOT_REACHED(); I feel like this assertion can be hit with invalid input. What happens when it is hit? What is the behavior of Chrome? Please add such a test if possible. > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:96 > + ASSERT(segmentStack.size() > 0); "> 0" is unnecessary > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:102 > + ASSERT(!identifiers.isEmpty()); > + if (identifiers.isEmpty()) > + return Exception { SyntaxError, "Cannot parse string as CSSUnparsedValue, parentheses do not match."_s }; If identifiers can be empty when parentheses don't match, the assertion should be removed and we should add a test that tries to parse a value with mismatching parentheses and verify that a SyntaxError exception is thrown. ASSERT should be used for things that can't be hit no matter how invalid the input, not for things that shouldn't happen with valid input.

Created attachment 438081 [details] Patch

(In reply to Alex Christensen from comment #17) > > > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:88 > > + ASSERT_NOT_REACHED(); > > I feel like this assertion can be hit with invalid input. What happens when > it is hit? What is the behavior of Chrome? Please add such a test if > possible. > > > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:96 > > + ASSERT(segmentStack.size() > 0); > > "> 0" is unnecessary > > > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:102 > > + ASSERT(!identifiers.isEmpty()); > > + if (identifiers.isEmpty()) > > + return Exception { SyntaxError, "Cannot parse string as CSSUnparsedValue, parentheses do not match."_s }; > > If identifiers can be empty when parentheses don't match, the assertion > should be removed and we should add a test that tries to parse a value with > mismatching parentheses and verify that a SyntaxError exception is thrown. > ASSERT should be used for things that can't be hit no matter how invalid the > input, not for things that shouldn't happen with valid input. According to my testing, syntax error would be found during the tokenizing stage. Presumably if the CSSPropertyParser is bug-free, then variable references syntax would be correct, and parentheses should match.

Created attachment 438313 [details] Patch

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Created attachment 438319 [details] Patch

Created attachment 438320 [details] Patch

Created attachment 438321 [details] Patch

Comment on attachment 438321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438321&action=review > Source/WebCore/css/typedom/CSSStyleValueFactory.cpp:2 > + * Copyright (C) 2019 Apple Inc. All rights reserved. 2021 > Source/WebCore/css/typedom/CSSStyleValueFactory.h:2 > + * Copyright (C) 2018 Apple Inc. All rights reserved. 2021 > Source/WebCore/css/typedom/CSSUnparsedValue.cpp:103 > + RELEASE_ASSERT(!variableReference.hasException()); I think a lot of these could just be ASSERTs. RELEASE_ASSERT is typically used for cases where we want to crash if this condition is somehow reached because otherwise something bad will happen like writing out of bounds. I don't think that's the case here. > Source/WebCore/dom/StyledElement.cpp:122 > + auto shorthand = shorthandForProperty(propertyID); > + for (auto longhand : shorthand) { > + if (auto cssValue = element.inlineStyle()->getPropertyCSSValue(longhand)) > + return StylePropertyMapReadOnly::reifyValue(cssValue.get(), element.document(), &element); > + } I don't understand what this change does, why it's necessary outside of typedom, and what else it could change. Could this change go in a separate patch? > LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/inline/get.html:32 > + new CSSUnparsedValue([remove_leading_spaces(' auto')])); Does this still pass in Chrome? The remove_leading_spaces might be in the wrong place. To do this change you could just replace ' auto' with 'auto'. The intent of this change was to make the test more permissive of unspecified behavior where Safari and Firefox have different behavior than Chrome.

(In reply to Alex Christensen from comment #25) > > Source/WebCore/dom/StyledElement.cpp:122 > > + auto shorthand = shorthandForProperty(propertyID); > > + for (auto longhand : shorthand) { > > + if (auto cssValue = element.inlineStyle()->getPropertyCSSValue(longhand)) > > + return StylePropertyMapReadOnly::reifyValue(cssValue.get(), element.document(), &element); > > + } > > I don't understand what this change does, why it's necessary outside of > typedom, and what else it could change. Could this change go in a separate > patch? It adds the correct behavior that parses shorthand CSSPropertyID to reify the correct CSSValue. I think it is relevant because it is part of the usage of CSS Typed OM. > > LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/inline/get.html:32 > > + new CSSUnparsedValue([remove_leading_spaces(' auto')])); > > Does this still pass in Chrome? The remove_leading_spaces might be in the > wrong place. To do this change you could just replace ' auto' with 'auto'. > The intent of this change was to make the test more permissive of > unspecified behavior where Safari and Firefox have different behavior than > Chrome. Ops. I'm changing it now.

Created attachment 438427 [details] Patch

Created attachment 438428 [details] Patch

Created attachment 438520 [details] Patch

Comment on attachment 438520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438520&action=review > Source/WebCore/css/typedom/CSSStyleValueFactory.cpp:65 > + extra spaces > Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:51 > + return (result.hasException() ? nullptr : RefPtr<CSSStyleValue> { result.releaseReturnValue() }); I wonder if we'll want to return ExceptionOr<RefPtr> here to pass the exact exception up the call stack. > LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/inline/get.html:35 > + // assert_style_value_equals(styleMap.get('--foo'), This should probably be removed.

Tools/Scripts/svn-apply failed to apply attachment 438520 [details] to trunk. Please resolve the conflicts and upload a new patch.

Created attachment 438688 [details] Patch

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.