CSS Typed OM should expose attributeStyleMap
Created attachment 357241 [details] Patch
Created attachment 357243 [details] Patch
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.
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.
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.
Comment on attachment 357243 [details] Patch r- because of the above issues and build failures.
Created attachment 357272 [details] Patch
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
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
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
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
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
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
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
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
(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.
Created attachment 357289 [details] Patch
(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.
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
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
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
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
(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.
Created attachment 357343 [details] Patch
Once the bots pass, I am going to roll the bindings generator changes into a separate patch.
Created attachment 357345 [details] Patch
Created attachment 357347 [details] Patch
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
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
Huh, css-typedom/attributeStyleMap.html is failing on Windows. Do we know why what's happening here?
(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.
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.
Created attachment 357538 [details] Patch
Created attachment 357539 [details] Patch
Created attachment 357574 [details] Patch
Comment on attachment 357574 [details] Patch Clearing flags on attachment: 357574 Committed r239341: <https://trac.webkit.org/changeset/239341>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46815119>