Addition of CSSNumericFactory in CSS Typed OM
Created attachment 436849 [details] Patch
Created attachment 436850 [details] Patch
Created attachment 436851 [details] Patch
Created attachment 436854 [details] Patch
Comment on attachment 436854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436854&action=review Looks like there's still one test failing. > Source/WebCore/css/typedom/CSSNumericFactory.cpp:2 > + * Copyright (C) 2018 Apple Inc. All rights reserved. 2021 > Source/WebCore/css/typedom/CSSNumericFactory.cpp:37 > +CSSNumericFactory* CSSNumericFactory::from(DOMCSSNamespace& css) This should probably return a CSSNumericFactory& because it makes a new 37CSSNumericFactory if there isn't one already. > Source/WebCore/css/typedom/CSSNumericFactory.h:41 > +class CSSNumericFactory final : public Supplement<DOMCSSNamespace> { I've never been a fan of the whole Supplementable/Supplement pattern which does a hash lookup just to access a member variable. I think just making a std::unique_ptr instead of a SupplementMap would be cleaner, but for some reason DOMCSSNamespace is already Supplementable so this is just following an existing pattern. Maybe we shouldn't change it in this patch. > Source/WebCore/css/typedom/CSSNumericFactory.h:46 > + static Ref<CSSUnitValue> number(double value) { return CSSUnitValue::create(value, "number"); } Can someone make custom units, or could we change CSSUnitValue::m_unit to be an enum class instead of a String? > Source/WebCore/css/typedom/CSSNumericFactory.idl:1 > +partial namespace CSS { This needs a copyright.
(In reply to Alex Christensen from comment #5) > Can someone make custom units, or could we change CSSUnitValue::m_unit to be > an enum class instead of a String? Maybe CSSUnitType, which already seems to exist.
(In reply to Alex Christensen from comment #5) > Looks like there's still one test failing. I'm confused as to why that test is failing. It seems related to my patch. > > > Source/WebCore/css/typedom/CSSNumericFactory.cpp:37 > > +CSSNumericFactory* CSSNumericFactory::from(DOMCSSNamespace& css) > > This should probably return a CSSNumericFactory& because it makes a new > 37CSSNumericFactory if there isn't one already. I think it might be the standard of using Supplemental to return a pointer for the from function. I copied this format from another DOMCSSNamespace supplement, and in the Supplementable.h example it also returns a pointer. Should I use a lvalue ref here instead? > > Source/WebCore/css/typedom/CSSNumericFactory.h:46 > > + static Ref<CSSUnitValue> number(double value) { return CSSUnitValue::create(value, "number"); } > > Can someone make custom units, or could we change CSSUnitValue::m_unit to be > an enum class instead of a String? > I don't think custom units are allowed. But the whole unit system is quite convoluted and needs another patch. I think I'll just add a FIXME here for now.
Created attachment 436972 [details] Patch
Created attachment 436999 [details] Patch
Created attachment 437006 [details] Patch
Created attachment 437121 [details] Patch
Created attachment 437129 [details] Patch
Created attachment 437130 [details] Patch
Created attachment 437131 [details] Patch
Comment on attachment 437131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437131&action=review > Source/WebCore/css/typedom/CSSUnitValue.h:-46 > - // FIXME: not correct. > - String toString() const final { return makeString((int) m_value, m_unit); } Removing this caused the change to LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-serialization/cssUnitValue.tentative-expected.txt and LayoutTests/fast/css-custom-paint/properties.html and LayoutTests/fast/css-custom-paint/properties-expected.html I suggest you remove this change and revert those two test expectations files and do those in a separate patch. As it stands now, this change is unrelated to the other changes in this patch and changes one incorrect result for another incorrect result.
Created attachment 437314 [details] Patch
Committed r282039 (241339@main): <https://commits.webkit.org/241339@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437314 [details].
<rdar://problem/82746215>