Bug 192671 - CSS Typed OM should expose attributeStyleMap
Summary: CSS Typed OM should expose attributeStyleMap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on: 192721
Blocks: 175733
  Show dependency treegraph
 
Reported: 2018-12-13 11:45 PST by Justin Michaud
Modified: 2018-12-18 10:52 PST (History)
8 users (show)

See Also:


Attachments
Patch (36.17 KB, patch)
2018-12-13 11:52 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (35.37 KB, patch)
2018-12-13 11:56 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (38.88 KB, patch)
2018-12-13 17:00 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.80 MB, application/zip)
2018-12-13 18:07 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.52 MB, application/zip)
2018-12-13 18:41 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (2.00 MB, application/zip)
2018-12-13 19:02 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.41 MB, application/zip)
2018-12-13 19:03 PST, Build Bot
no flags Details
Patch (52.60 KB, patch)
2018-12-13 19:37 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.51 MB, application/zip)
2018-12-13 20:51 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (2.24 MB, application/zip)
2018-12-13 21:32 PST, Build Bot
no flags Details
Patch (74.46 KB, patch)
2018-12-14 14:18 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (73.79 KB, patch)
2018-12-14 15:10 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (73.86 KB, patch)
2018-12-14 15:16 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.83 MB, application/zip)
2018-12-14 18:11 PST, Build Bot
no flags Details
Patch (67.69 KB, patch)
2018-12-17 22:41 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (68.48 KB, patch)
2018-12-17 22:49 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (68.47 KB, patch)
2018-12-18 10:01 PST, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 2018-12-13 18:07:18 PST Comment hidden (obsolete)
Comment 9 Build Bot 2018-12-13 18:07:19 PST Comment hidden (obsolete)
Comment 10 Build Bot 2018-12-13 18:41:53 PST Comment hidden (obsolete)
Comment 11 Build Bot 2018-12-13 18:41:55 PST Comment hidden (obsolete)
Comment 12 Build Bot 2018-12-13 19:02:50 PST Comment hidden (obsolete)
Comment 13 Build Bot 2018-12-13 19:02:52 PST Comment hidden (obsolete)
Comment 14 Build Bot 2018-12-13 19:03:26 PST Comment hidden (obsolete)
Comment 15 Build Bot 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 Build Bot 2018-12-13 20:51:09 PST Comment hidden (obsolete)
Comment 20 Build Bot 2018-12-13 20:51:11 PST Comment hidden (obsolete)
Comment 21 Build Bot 2018-12-13 21:32:06 PST Comment hidden (obsolete)
Comment 22 Build Bot 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 Build Bot 2018-12-14 18:11:34 PST Comment hidden (obsolete)
Comment 29 Build Bot 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>