Bug 190206 – [CSSTypedOM] Add StylePropertyMap IDL and stubs

• Status: ASSIGNED
• Component: CSS

[Don Olmstead]

Start out by adding in the StylePropertyMap IDL files.

[Don Olmstead]

Created attachment 351403 [details]
WIP Patch

Its my first time trying to add a platform feature so I'm sure there's a bunch of problems with the implementation out of the gate.

[EWS Watchlist]

Attachment 351403 [details] did not pass style-queue:


ERROR: Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/typedom/StylePropertyMap.h:31:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/typedom/StylePropertyMap.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/features.json:0:  The feature "Pointer Events" has a specification field but no specification of that name exists.  [json/syntax] [5]
ERROR: Source/WebCore/css/typedom/CSSStyleValue.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/css/typedom/CSSStyleValue.cpp:37:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:29:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:38:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:58:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 12 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 351408 [details]
WIP Patch

[EWS Watchlist]

Attachment 351408 [details] did not pass style-queue:


ERROR: Source/WebCore/css/typedom/StylePropertyMap.h:31:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/features.json:0:  The feature "Pointer Events" has a specification field but no specification of that name exists.  [json/syntax] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 351408 [details]
WIP Patch

Some nits below.

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

> Source/WebCore/CMakeLists.txt:556
> +    css/typedom/CSSStyleValue.idl

Should it be typeddom?
You might need to add this folder to the include path.

> Source/WebCore/ChangeLog:8
> +        No new tests. Covered by WPT.

Might be good to add a link to the spec.
I guess some WPT tests will need to bee rebased?

> Source/WebCore/Sources.txt:690
> +css/typedom/StylePropertyMapReadOnly.cpp

Might need JSXX files as well.

> Source/WebCore/css/CSSStyleRule.cpp:136
> +    return nullptr;

notImplemented()

> Source/WebCore/css/CSSStyleRule.h:31
> +class StylePropertyMap;

Move it before StyleRuleCSSStyleDeclaration

> Source/WebCore/css/CSSStyleRule.idl:28
> +    [EnabledAtRuntime=CSSTypedOM,SameObject] readonly attribute StylePropertyMap styleMap;

space after comma?

> Source/WebCore/css/typedom/CSSStyleValue.cpp:33
> +    return nullptr;

notImplemented() here and below?
Should probably be String&& here and below and you will need to use UNUSED_PARAM

> Source/WebCore/css/typedom/StylePropertyMap.cpp:31
> +void StylePropertyMap::set(String property, Vector<Variant<String, RefPtr<CSSStyleValue>>> values)

r-values here and below as well.

[Don Olmstead]

Created attachment 351416 [details]
WIP Patch

[EWS Watchlist]

Attachment 351416 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/features.json:0:  The feature "Pointer Events" has a specification field but no specification of that name exists.  [json/syntax] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 351499 [details]
WIP Patch

[EWS Watchlist]

Attachment 351499 [details] did not pass style-queue:


ERROR: Source/WebCore/features.json:0:  The feature "Pointer Events" has a specification field but no specification of that name exists.  [json/syntax] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 351501 [details]
WIP Patch

[EWS Watchlist]

Attachment 351501 [details] did not pass style-queue:


ERROR: Source/WebCore/features.json:0:  The feature "Pointer Events" has a specification field but no specification of that name exists.  [json/syntax] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 351504 [details]
WIP Patch

[EWS Watchlist]

Attachment 351504 [details] did not pass style-queue:


ERROR: Source/WebCore/features.json:0:  The feature "Pointer Events" has a specification field but no specification of that name exists.  [json/syntax] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Comment on attachment 351504 [details]
WIP Patch

Simon am I in the ballpark with this? First time working on a legit feature so feel free to let me know if I'm doing anything totally naive. I was mostly looking at what people have done in the past to add features.

Will get the XCode part in once I know I'm doing it right.

[Simon Fraser (smfr)]

I'll let Antti weigh on on whether this is the right direction.

[Antti Koivisto]

Comment on attachment 351504 [details]
WIP Patch

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

> Source/WebCore/ChangeLog:8
> +
> +        [CSSTypedOM] Add StylePropertyMap IDL and stubs
> +        https://bugs.webkit.org/show_bug.cgi?id=190206
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests. Covered by existing WPT tests.

You should link to the spec.

[Antti Koivisto]

Comment on attachment 351504 [details]
WIP Patch

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

> Source/WebCore/css/typedom/StylePropertyMap.h:32
> +namespace WebCore {
> +
> +class StylePropertyMap final : public StylePropertyMapReadOnly {

It might make sense to put these types into a namespace of their own (like 'TypedOM') so it is clear these are API implementations. Currently these names get easily mixed with the existing internal ones (StyleProperties etc).

[Ahmad Saleem]

I think this commit:

https://github.com/WebKit/WebKit/commit/9940f5c6cd901c3cce4efb372ebd94d808218d80

Added these bits as well?


Is it needed anymore since patch is r+ but might need rebase, if needed. I will leave other to decide and mark / tag this bug accordingly. Thanks!