Bug 230031 – Addition of CSSNumericValue and subclasses. (CSS Typed OM)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Qiaosong Zhou]

Addition of CSSNumericValue and subclasses. (CSS Typed OM)

[Qiaosong Zhou]

Created attachment 437577 [details]
Patch

[Qiaosong Zhou]

Created attachment 437794 [details]
Patch

[EWS Watchlist]

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

[Qiaosong Zhou]

Created attachment 437813 [details]
Patch

[Qiaosong Zhou]

Created attachment 437815 [details]
Patch

[Alex Christensen]

Comment on attachment 437815 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437815&action=review

> Source/WebCore/css/typedom/CSSNumericValue.cpp:50
> +        auto numericValue = CSSNumericValue::rectifyNumberish(WTFMove(value));

Either use this or remove it.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:58
> +    UNUSED_PARAM(values);

You can also just omit "values" above.
sub(Vector<CSSNumberish>&&)

> Source/WebCore/css/typedom/CSSNumericValue.cpp:75
> +}

inconsistent spacing.

> Source/WebCore/css/typedom/CSSNumericValue.cpp:91
> +//    if (WTF::holds_alternative<RefPtr<CSSNumericValue>>(numberish))

remove

> Source/WebCore/css/typedom/CSSNumericValue.cpp:108
> +    // FIXME: add impl.

This is probably also good to have above.

> Source/WebCore/css/typedom/CSSNumericValue.h:47
> +    Ref<CSSNumericValue> add(Vector<CSSNumberish>&& values);

"values" is probably unnecessary here.  It doesn't add any meaning.

> Source/WebCore/css/typedom/numeric/CSSMathMax.cpp:50
> +const CSSNumericArray& CSSMathMax::values()

const after values()
and in header.

> Source/WebCore/css/typedom/numeric/CSSMathOperator.h:32
> +enum class CSSMathOperator: uint8_t {

nit: space before colon

> Source/WebCore/css/typedom/numeric/CSSMathSum.cpp:50
> +const CSSNumericArray& CSSMathSum::values()

const after values()

> Source/WebCore/css/typedom/numeric/CSSMathValue.h:36
> +enum class CSSMathValueType: uint8_t {

space before colon.

> Source/WebCore/css/typedom/numeric/CSSNumericBaseType.cpp:28
> +#if ENABLE(CSS_TYPED_OM)

You should probably just omit this file, unless you have plans for it.
If you do need it, you need to include config.h and CSSNumericBaseType.h outside the ENABLE macro.

> Source/WebCore/css/typedom/numeric/CSSNumericBaseType.h:32
> +enum class CSSNumericBaseType {

: uint8_t

> Source/WebCore/css/typedom/numeric/CSSNumericType.cpp:31
> +#if ENABLE(CSS_TYPED_OM)
> +
> +#include "config.h"
> +#include "CSSNumericType.h"

include outside the ENABLE macros.

[Qiaosong Zhou]

Created attachment 437920 [details]
Patch

[Alex Christensen]

Comment on attachment 437920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437920&action=review

> Source/WebCore/css/typedom/CSSNumericValue.cpp:140
> +    return Exception { TypeError, "Not implemented Error" };

NotSupportedError

> Source/WebCore/css/typedom/numeric/CSSMathInvert.idl:2
> +* Copyright (C) 2018 Apple Inc.  All rights reserved.

2021

> Source/WebCore/css/typedom/numeric/CSSMathMax.idl:2
> +* Copyright (C) 2018 Apple Inc.  All rights reserved.

2021
Several more times below.  Check all the files.

> Source/WebCore/css/typedom/numeric/CSSMathOperator.cpp:35
> +namespace WebCore {
> +
> +} // namespace WebCore

Omit this file.

> Source/WebCore/css/typedom/numeric/CSSNumericArray.h:44
> +    Ref<CSSNumericValue> item(size_t index) { return m_array[index].copyRef(); }

This really needs bounds checks.  I'm not sure what to return if it's out of bounds.

> Source/WebCore/css/typedom/numeric/CSSNumericType.cpp:34
> +namespace WebCore {
> +} // namespace WebCore

Omit this file.

[Qiaosong Zhou]

Created attachment 437924 [details]
Patch

[Qiaosong Zhou]

Created attachment 437927 [details]
Patch

[Qiaosong Zhou]

Created attachment 437937 [details]
Patch

[Qiaosong Zhou]

Created attachment 437939 [details]
Patch

[Alex Christensen]

Comment on attachment 437939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437939&action=review

> Source/WebCore/css/typedom/CSSNumericValue.cpp:69
> +

extra space

> Source/WebCore/css/typedom/numeric/CSSNumericArray.cpp:45
> +    Vector<Ref<CSSNumericValue>> values;
> +    for (auto&& numberish : numberishes)
> +        values.append(CSSNumericValue::rectifyNumberish(WTFMove(numberish)));

This pattern can be replaced by one of two things to become more efficient:
1. WTF::map
2. Something like this:
Vector<Ref> values;
values.reserveInitialCapacity(numberishes.size());
for (auto&& numbers : numberishes)
    values.uncheckedAppend(...)

Currently, each append needs to check the capacity of the Vector which adds unnecessary branches, and if there is a large number of numberishes then we will reallocate and copy several times.  Using these patterns makes it so there is 1 allocation and 0 capacity checks.

> Source/WebCore/css/typedom/numeric/CSSNumericType.cpp:34
> +namespace WebCore {
> +} // namespace WebCore

Please remove this file.

[Qiaosong Zhou]

Created attachment 437982 [details]
Patch

[Qiaosong Zhou]

Created attachment 437984 [details]
Patch

[Qiaosong Zhou]

Created attachment 437985 [details]
Patch

[Qiaosong Zhou]

Created attachment 437986 [details]
Patch

[Qiaosong Zhou]

Created attachment 438000 [details]
Patch

[EWS]

Committed r282356 (241619@main): <https://commits.webkit.org/241619@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438000 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83071294>