Bug 192671 – CSS Typed OM should expose attributeStyleMap

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Justin Michaud]

CSS Typed OM should expose attributeStyleMap

[Justin Michaud]

Created attachment 357241 [details]
Patch

[Justin Michaud]

Created attachment 357243 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 357243 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

You should add a description as to what you're implementing.

> Source/WebCore/css/ElementCSSInlineStyle.idl:32
> +    [SameObject, EnabledAtRuntime=CSSTypedOM, Conditional=CSS_TYPED_OM] readonly attribute StylePropertyMap attributeStyleMap;

SameObject isn't supported hence the FIXME right above. So that the same thing here.

> Source/WebCore/css/typedom/StylePropertyMap.idl:32
> +] interface StylePropertyMap : StylePropertyMapReadOnly {

You need GenerateIsReachable=ImplOwnerNodeRoot and make StylePropertyMapReadOnly reachable from GC
when the element is alive to get the same effect as SameObject.

[Ryosuke Niwa]

Right now, your code has a bug that if you call element.attributeStyleMap, let GC run, and then access element.attributeStyleMap again, then we'd create a new JS wrapper.

That is, if you had inserted element.attributeStyleMap into a WeakMap/WeakSet and/or you added a property onto it, then we'd lose that entry / property when GC runs.

[Ryosuke Niwa]

Comment on attachment 357243 [details]
Patch

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

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:46
> +    struct StylePropertyMapImpl {
> +        virtual ~StylePropertyMapImpl() = default;
> +        virtual RefPtr<TypedOMCSSStyleValue> get(const String& property) const = 0;
> +    };

Why do we need a separate object for this? Why can't StylePropertyMapReadOnly simply provide some overridable virtual functions instead?
Is the intent that multiple StylePropertyMapReadOnly would share a simple implementation?
When would that happen?

> Source/WebCore/dom/StyledElement.cpp:124
> +    if (!ensureElementRareData().attributeStyleMap())
> +        elementRareData()->setAttributeStyleMap(StylePropertyMap::create(makeUniqueRef<StyledElementInlineStylePropertyMapImpl>(*this)));
> +    return *elementRareData()->attributeStyleMap();

Calling ensureElementRareData() three times is efficient.
Store a reference to it the first time you call it.

[Ryosuke Niwa]

Comment on attachment 357243 [details]
Patch

r- because of the above issues and build failures.

[Justin Michaud]

Created attachment 357272 [details]
Patch

[EWS Watchlist]

Comment on attachment 357272 [details]
Patch

Attachment 357272 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10390020

New failing tests:
css-typedom/sameobject.html
css-typedom/attributeStyleMap.html

[EWS Watchlist]

Created attachment 357280 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357272 [details]
Patch

Attachment 357272 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10390423

New failing tests:
imported/w3c/web-platform-tests/css/css-properties-values-api/typedom.tentative.html
js/dom/dom-static-property-for-in-iteration.html
imported/w3c/web-platform-tests/css/css-properties-values-api/unit-cycles.html

[EWS Watchlist]

Created attachment 357282 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357272 [details]
Patch

Attachment 357272 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10390266

New failing tests:
css-typedom/sameobject.html
css-typedom/attributeStyleMap.html

[EWS Watchlist]

Created attachment 357286 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357272 [details]
Patch

Attachment 357272 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10390282

New failing tests:
imported/w3c/web-platform-tests/css/css-properties-values-api/typedom.tentative.html
imported/w3c/web-platform-tests/css/css-properties-values-api/unit-cycles.html

[EWS Watchlist]

Created attachment 357287 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Justin Michaud]

(In reply to Ryosuke Niwa from comment #5)
> 
> Why do we need a separate object for this? Why can't
> StylePropertyMapReadOnly simply provide some overridable virtual functions
> instead?
> Is the intent that multiple StylePropertyMapReadOnly would share a simple
> implementation?
> When would that happen?

This is more for bindings. StylePropertyMap inherits from StylePropertyMapReadOnly, and I didn't want to have to disable bindings hardening or write my own toJS function. I think (?) that I would need to do at least one of those things if I tried to pass a subclass of StylePropertyMap to toJS.

[Justin Michaud]

Created attachment 357289 [details]
Patch

[Ryosuke Niwa]

(In reply to Justin Michaud from comment #16)
> (In reply to Ryosuke Niwa from comment #5)
> > 
> > Why do we need a separate object for this? Why can't
> > StylePropertyMapReadOnly simply provide some overridable virtual functions
> > instead?
> > Is the intent that multiple StylePropertyMapReadOnly would share a simple
> > implementation?
> > When would that happen?
> 
> This is more for bindings. StylePropertyMap inherits from
> StylePropertyMapReadOnly, and I didn't want to have to disable bindings
> hardening or write my own toJS function. I think (?) that I would need to do
> at least one of those things if I tried to pass a subclass of
> StylePropertyMap to toJS.

Just remove ImplementationLacksVTable from IDL then you should be able to use inheritance. There is no need to avoid using virtual function table for this.

[EWS Watchlist]

Comment on attachment 357289 [details]
Patch

Attachment 357289 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10392035

New failing tests:
js/dom/dom-static-property-for-in-iteration.html
css-typedom/sameobject.html
css-typedom/attributeStyleMap.html

[EWS Watchlist]

Created attachment 357296 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 357289 [details]
Patch

Attachment 357289 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10392140

New failing tests:
js/dom/dom-static-property-for-in-iteration.html
css-typedom/sameobject.html
css-typedom/attributeStyleMap.html

[EWS Watchlist]

Created attachment 357298 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Justin Michaud]

(In reply to Ryosuke Niwa from comment #18)
> (In reply to Justin Michaud from comment #16)
> > (In reply to Ryosuke Niwa from comment #5)
> > > 
> > > Why do we need a separate object for this? Why can't
> > > StylePropertyMapReadOnly simply provide some overridable virtual functions
> > > instead?
> > > Is the intent that multiple StylePropertyMapReadOnly would share a simple
> > > implementation?
> > > When would that happen?
> > 
> > This is more for bindings. StylePropertyMap inherits from
> > StylePropertyMapReadOnly, and I didn't want to have to disable bindings
> > hardening or write my own toJS function. I think (?) that I would need to do
> > at least one of those things if I tried to pass a subclass of
> > StylePropertyMap to toJS.
> 
> Just remove ImplementationLacksVTable from IDL then you should be able to
> use inheritance. There is no need to avoid using virtual function table for
> this.

I did need to disable binding hardening, although since there are no IDL subclasses, thankfully I don't need a custom toJS function like in TypedOMCSSStyleValue.

[Justin Michaud]

Created attachment 357343 [details]
Patch

[Justin Michaud]

Once the bots pass, I am going to roll the bindings generator changes into a separate patch.

[Justin Michaud]

Created attachment 357345 [details]
Patch

[Justin Michaud]

Created attachment 357347 [details]
Patch

[EWS Watchlist]

Comment on attachment 357347 [details]
Patch

Attachment 357347 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10404832

New failing tests:
css-typedom/sameobject.html
css-typedom/attributeStyleMap.html

[EWS Watchlist]

Created attachment 357368 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ryosuke Niwa]

Huh, css-typedom/attributeStyleMap.html is failing on Windows. Do we know why what's happening here?

[Justin Michaud]

(In reply to Ryosuke Niwa from comment #30)
> Huh, css-typedom/attributeStyleMap.html is failing on Windows. Do we know
> why what's happening here?

The feature flag only works with WK2, but I forgot to change the windows test expectations.

[Ryosuke Niwa]

Comment on attachment 357347 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        * css/ElementCSSInlineStyle.idl:

You're missing an entry for CodeGeneratorJS.pm. Please add it with a description as to what you're changing.

> Source/WebCore/css/typedom/StylePropertyMap.h:38
> +public:

I don't think we need to have this access specifier.

> Source/WebCore/dom/StyledElement.cpp:89
> +                auto* registered = element.document().getCSSRegisteredCustomPropertySet().get(name);

Nit: we should probably address this in a separate patch but our coding guideline mandates
that this function, which doesn't have an out argument, to not have "get" prefix:
https://webkit.org/code-style-guidelines/#names-out-argument

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:62
> +static RefPtr<TypedOMCSSStyleValue> extractComputedProperty(const String& name, Element& element)

Can we share this code with the one in StyledElement.cpp e.g. using a template?
It's not great to duplicate this much code in two places.

[Justin Michaud]

Created attachment 357538 [details]
Patch

[Justin Michaud]

Created attachment 357539 [details]
Patch

[Justin Michaud]

Created attachment 357574 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 357574 [details]
Patch

Clearing flags on attachment: 357574

Committed r239341: <https://trac.webkit.org/changeset/239341>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46815119>