Bug 192671

Summary: CSS Typed OM should expose attributeStyleMap
Product: WebKit Reporter: Justin Michaud <justin_michaud>
Component: Layout and RenderingAssignee: Justin Michaud <justin_michaud>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, dino, ews-watchlist, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 192721    
Bug Blocks: 175733    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews116 for mac-sierra
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews112 for mac-sierra
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Patch
none
Patch
none
Patch none

Description Justin Michaud 2018-12-13 11:45:56 PST
CSS Typed OM should expose attributeStyleMap
Comment 1 Justin Michaud 2018-12-13 11:52:03 PST
Created attachment 357241 [details]
Patch
Comment 2 Justin Michaud 2018-12-13 11:56:19 PST
Created attachment 357243 [details]
Patch
Comment 3 Ryosuke Niwa 2018-12-13 13:28:47 PST
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.
Comment 4 Ryosuke Niwa 2018-12-13 13:30:29 PST
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 5 Ryosuke Niwa 2018-12-13 13:34:00 PST
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 6 Ryosuke Niwa 2018-12-13 13:34:27 PST
Comment on attachment 357243 [details]
Patch

r- because of the above issues and build failures.
Comment 7 Justin Michaud 2018-12-13 17:00:37 PST
Created attachment 357272 [details]
Patch
Comment 8 EWS Watchlist 2018-12-13 18:07:18 PST Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-12-13 18:07:19 PST Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-12-13 18:41:53 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-12-13 18:41:55 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-12-13 19:02:50 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 2018-12-13 19:02:52 PST Comment hidden (obsolete)
Comment 14 EWS Watchlist 2018-12-13 19:03:26 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-12-13 19:03:28 PST Comment hidden (obsolete)
Comment 16 Justin Michaud 2018-12-13 19:22:11 PST
(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.
Comment 17 Justin Michaud 2018-12-13 19:37:43 PST
Created attachment 357289 [details]
Patch
Comment 18 Ryosuke Niwa 2018-12-13 19:41:44 PST
(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 19 EWS Watchlist 2018-12-13 20:51:09 PST Comment hidden (obsolete)
Comment 20 EWS Watchlist 2018-12-13 20:51:11 PST Comment hidden (obsolete)
Comment 21 EWS Watchlist 2018-12-13 21:32:06 PST Comment hidden (obsolete)
Comment 22 EWS Watchlist 2018-12-13 21:32:08 PST Comment hidden (obsolete)
Comment 23 Justin Michaud 2018-12-14 13:32:12 PST
(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.
Comment 24 Justin Michaud 2018-12-14 14:18:41 PST
Created attachment 357343 [details]
Patch
Comment 25 Justin Michaud 2018-12-14 14:21:00 PST
Once the bots pass, I am going to roll the bindings generator changes into a separate patch.
Comment 26 Justin Michaud 2018-12-14 15:10:46 PST
Created attachment 357345 [details]
Patch
Comment 27 Justin Michaud 2018-12-14 15:16:01 PST
Created attachment 357347 [details]
Patch
Comment 28 EWS Watchlist 2018-12-14 18:11:34 PST Comment hidden (obsolete)
Comment 29 EWS Watchlist 2018-12-14 18:11:45 PST Comment hidden (obsolete)
Comment 30 Ryosuke Niwa 2018-12-17 13:04:10 PST
Huh, css-typedom/attributeStyleMap.html is failing on Windows. Do we know why what's happening here?
Comment 31 Justin Michaud 2018-12-17 14:44:44 PST
(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 32 Ryosuke Niwa 2018-12-17 14:54:25 PST
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.
Comment 33 Justin Michaud 2018-12-17 22:41:07 PST
Created attachment 357538 [details]
Patch
Comment 34 Justin Michaud 2018-12-17 22:49:08 PST
Created attachment 357539 [details]
Patch
Comment 35 Justin Michaud 2018-12-18 10:01:30 PST
Created attachment 357574 [details]
Patch
Comment 36 WebKit Commit Bot 2018-12-18 10:51:14 PST
Comment on attachment 357574 [details]
Patch

Clearing flags on attachment: 357574

Committed r239341: <https://trac.webkit.org/changeset/239341>
Comment 37 WebKit Commit Bot 2018-12-18 10:51:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Radar WebKit Bug Importer 2018-12-18 10:52:47 PST
<rdar://problem/46815119>