Bug 230031 - Addition of CSSNumericValue and subclasses. (CSS Typed OM)
Summary: Addition of CSSNumericValue and subclasses. (CSS Typed OM)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Qiaosong Zhou
URL:
Keywords: InRadar
Depends on:
Blocks: 175733
  Show dependency treegraph
 
Reported: 2021-09-07 19:00 PDT by Qiaosong Zhou
Modified: 2021-09-13 13:59 PDT (History)
17 users (show)

See Also:


Attachments
Patch (88.07 KB, patch)
2021-09-07 19:00 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (473.10 KB, patch)
2021-09-09 15:44 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (473.13 KB, patch)
2021-09-09 17:44 PDT, Qiaosong Zhou
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (473.08 KB, patch)
2021-09-09 17:58 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (470.67 KB, patch)
2021-09-10 16:13 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (468.89 KB, patch)
2021-09-10 16:51 PDT, Qiaosong Zhou
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (468.96 KB, patch)
2021-09-10 17:01 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (468.97 KB, patch)
2021-09-10 20:10 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (468.97 KB, patch)
2021-09-10 21:55 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (466.96 KB, patch)
2021-09-12 02:29 PDT, Qiaosong Zhou
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (466.96 KB, patch)
2021-09-12 02:53 PDT, Qiaosong Zhou
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (466.98 KB, patch)
2021-09-12 03:02 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (466.98 KB, patch)
2021-09-12 03:16 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff
Patch (466.98 KB, patch)
2021-09-12 18:30 PDT, Qiaosong Zhou
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Qiaosong Zhou 2021-09-07 19:00:31 PDT
Addition of CSSNumericValue and subclasses. (CSS Typed OM)
Comment 1 Qiaosong Zhou 2021-09-07 19:00:56 PDT
Created attachment 437577 [details]
Patch
Comment 2 Qiaosong Zhou 2021-09-09 15:44:52 PDT
Created attachment 437794 [details]
Patch
Comment 3 EWS Watchlist 2021-09-09 15:45:54 PDT
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
Comment 4 Qiaosong Zhou 2021-09-09 17:44:59 PDT
Created attachment 437813 [details]
Patch
Comment 5 Qiaosong Zhou 2021-09-09 17:58:26 PDT
Created attachment 437815 [details]
Patch
Comment 6 Alex Christensen 2021-09-09 20:09:47 PDT
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.
Comment 7 Qiaosong Zhou 2021-09-10 16:13:49 PDT
Created attachment 437920 [details]
Patch
Comment 8 Alex Christensen 2021-09-10 16:28:43 PDT
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.
Comment 9 Qiaosong Zhou 2021-09-10 16:51:42 PDT
Created attachment 437924 [details]
Patch
Comment 10 Qiaosong Zhou 2021-09-10 17:01:38 PDT
Created attachment 437927 [details]
Patch
Comment 11 Qiaosong Zhou 2021-09-10 20:10:05 PDT
Created attachment 437937 [details]
Patch
Comment 12 Qiaosong Zhou 2021-09-10 21:55:15 PDT
Created attachment 437939 [details]
Patch
Comment 13 Alex Christensen 2021-09-10 22:43:03 PDT
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.
Comment 14 Qiaosong Zhou 2021-09-12 02:29:16 PDT
Created attachment 437982 [details]
Patch
Comment 15 Qiaosong Zhou 2021-09-12 02:53:07 PDT
Created attachment 437984 [details]
Patch
Comment 16 Qiaosong Zhou 2021-09-12 03:02:18 PDT
Created attachment 437985 [details]
Patch
Comment 17 Qiaosong Zhou 2021-09-12 03:16:22 PDT
Created attachment 437986 [details]
Patch
Comment 18 Qiaosong Zhou 2021-09-12 18:30:02 PDT
Created attachment 438000 [details]
Patch
Comment 19 EWS 2021-09-13 13:58:35 PDT
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].
Comment 20 Radar WebKit Bug Importer 2021-09-13 13:59:40 PDT
<rdar://problem/83071294>