Bug 238375 - Add support for element.computedStyleMap()
Summary: Add support for element.computedStyleMap()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-25 08:12 PDT by Carlos Garcia Campos
Modified: 2022-04-01 01:42 PDT (History)
18 users (show)

See Also:


Attachments
Patch (114.98 KB, patch)
2022-03-25 08:24 PDT, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (114.97 KB, patch)
2022-03-25 09:05 PDT, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (119.68 KB, patch)
2022-03-29 05:32 PDT, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (120.44 KB, patch)
2022-03-30 01:04 PDT, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (119.70 KB, patch)
2022-03-30 04:18 PDT, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (154.24 KB, patch)
2022-03-31 00:55 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Carlos Garcia Campos 2022-03-25 08:24:27 PDT
Created attachment 455763 [details]
Patch

The xcode changes are missing, since I don't have access to a mac right now.
Comment 2 Adrian Perez 2022-03-25 08:55:38 PDT
Comment on attachment 455763 [details]
Patch

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

This looks fine to me, save for a couple of small comments; but I would
rather have someone more familiar with the CSS/WebIDL machinery to give
the final r+ :)

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

> Source/WebCore/dom/ElementRareData.h:151
> +        if (m_m_computedStyleMap)

Typo, double “m_” prefix: m_m_computedStyleMap → m_computedStyleMap

> Source/WebCore/dom/StyledElement.cpp:102
> +        // FIXME: implement.

Wouldn't it be better to call the notImplemented() helper function here instead
of using a comment?

> Source/WebCore/dom/ElementRareData.h:151
> +        if (m_m_computedStyleMap)

Typo, double “m_” prefix: m_m_computedStyleMap → m_computedStyleMap

> Source/WebCore/dom/StyledElement.cpp:102
> +        // FIXME: implement.

Wouldn't it be better to call the notImplemented() helper function here instead
of using a comment?
Comment 3 Carlos Garcia Campos 2022-03-25 09:05:14 PDT
Created attachment 455768 [details]
Patch
Comment 4 Carlos Garcia Campos 2022-03-26 06:59:10 PDT
Could someone help me with the xcode changes, please?
Comment 5 Carlos Garcia Campos 2022-03-29 05:32:29 PDT
Created attachment 456018 [details]
Patch
Comment 6 Carlos Garcia Campos 2022-03-29 07:03:31 PDT
download-built-product step failed in gtk-wk2, so I guess it used a previously downloaded build and tests were run with the wrong built product.
Comment 7 Carlos Garcia Campos 2022-03-30 00:50:38 PDT
hmm, I see typed om is only enabled by default in WebKit for GTK and WPE ports, I don't know why. I guess we should enable it in WTR globally instead of adding a header in wpt tests.
Comment 8 Carlos Garcia Campos 2022-03-30 01:04:29 PDT
Created attachment 456098 [details]
Patch
Comment 9 Carlos Garcia Campos 2022-03-30 04:16:41 PDT
Ah!, I see alex changed typed om api to be EnabledBySetting instead of EnabledAtRuntime in r291867. I'll update the patch again.
Comment 10 Carlos Garcia Campos 2022-03-30 04:18:11 PDT
Created attachment 456112 [details]
Patch
Comment 11 Carlos Garcia Campos 2022-03-31 00:55:25 PDT
Created attachment 456208 [details]
Patch
Comment 12 Alex Christensen 2022-03-31 17:08:50 PDT
Comment on attachment 456208 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.h:60
> +    enum class PropertyValueType { Resolved, Computed };

nit
: bool

> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:67
> +ExceptionOr<Vector<RefPtr<CSSStyleValue>>> ComputedStylePropertyMapReadOnly::getAll(const String& property) const

This is mostly duplicate code.  Could you factor the shared logic into a helper function?

> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:102
> +Vector<StylePropertyMapReadOnly::StylePropertyMapEntry> ComputedStylePropertyMapReadOnly::entries() const

Could you add a few spec links in this file?  There is some interesting logic.
Comment 13 Carlos Garcia Campos 2022-04-01 00:47:24 PDT
Comment on attachment 456208 [details]
Patch

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

>> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:67
>> +ExceptionOr<Vector<RefPtr<CSSStyleValue>>> ComputedStylePropertyMapReadOnly::getAll(const String& property) const
> 
> This is mostly duplicate code.  Could you factor the shared logic into a helper function?

I'm not sure how to move the common logic, because in the end they return a different value, I'm not sure it's worth using templates for this. I'll leave it as is for now, if you know a better way I'll submit a follow up patch.
Comment 14 Carlos Garcia Campos 2022-04-01 00:47:57 PDT
iOS failures are unrelated, I've seen the same tests failing in other bugs.
Comment 15 Carlos Garcia Campos 2022-04-01 01:30:07 PDT
Committed r292210 (249113@trunk): <https://commits.webkit.org/249113@trunk>
Comment 16 Radar WebKit Bug Importer 2022-04-01 01:31:18 PDT
<rdar://problem/91151388>