Bug 238375 – Add support for element.computedStyleMap()

• Component: CSS
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

See https://drafts.css-houdini.org/css-typed-om-1/#dom-element-computedstylemap.

[Carlos Garcia Campos]

Created attachment 455763 [details]
Patch

The xcode changes are missing, since I don't have access to a mac right now.

[Adrian Perez]

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?

[Carlos Garcia Campos]

Created attachment 455768 [details]
Patch

[Carlos Garcia Campos]

Could someone help me with the xcode changes, please?

[Carlos Garcia Campos]

Created attachment 456018 [details]
Patch

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 456098 [details]
Patch

[Carlos Garcia Campos]

Ah!, I see alex changed typed om api to be EnabledBySetting instead of EnabledAtRuntime in r291867. I'll update the patch again.

[Carlos Garcia Campos]

Created attachment 456112 [details]
Patch

[Carlos Garcia Campos]

Created attachment 456208 [details]
Patch

[Alex Christensen]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

iOS failures are unrelated, I've seen the same tests failing in other bugs.

[Carlos Garcia Campos]

Committed r292210 (249113@trunk): <https://commits.webkit.org/249113@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/91151388>