Bug 229699 – Addition of CSSNumericFactory in CSS Typed OM

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Qiaosong Zhou]

Addition of CSSNumericFactory in CSS Typed OM

[Qiaosong Zhou]

Created attachment 436849 [details]
Patch

[Qiaosong Zhou]

Created attachment 436850 [details]
Patch

[Qiaosong Zhou]

Created attachment 436851 [details]
Patch

[Qiaosong Zhou]

Created attachment 436854 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

(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.

[Qiaosong Zhou]

(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.

[Qiaosong Zhou]

Created attachment 436972 [details]
Patch

[Qiaosong Zhou]

Created attachment 436999 [details]
Patch

[Qiaosong Zhou]

Created attachment 437006 [details]
Patch

[Qiaosong Zhou]

Created attachment 437121 [details]
Patch

[Qiaosong Zhou]

Created attachment 437129 [details]
Patch

[Qiaosong Zhou]

Created attachment 437130 [details]
Patch

[Qiaosong Zhou]

Created attachment 437131 [details]
Patch

[Alex Christensen]

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.

[Qiaosong Zhou]

Created attachment 437314 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/82746215>