Bug 191309 – CSS Painting API should pass size, arguments and input properties to paint callback

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

[Justin Michaud]

CSS Painting API should pass size, arguments and input properties to paint callback

[Justin Michaud]

Created attachment 353973 [details]
Patch

[Justin Michaud]

Created attachment 354026 [details]
Patch

[Justin Michaud]

Created attachment 354031 [details]
Patch

[EWS Watchlist]

Comment on attachment 354031 [details]
Patch

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

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

[EWS Watchlist]

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

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

[Justin Michaud]

Created attachment 354046 [details]
Patch

[Chris Dumez]

Comment on attachment 354046 [details]
Patch

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

> Source/WebCore/css/typedom/CSSNumericValue.h:31
> +#include <wtf/RefCounted.h>

Not needed.

> Source/WebCore/css/typedom/CSSNumericValue.h:32
> +#include <wtf/text/WTFString.h>

Not needed.

> Source/WebCore/css/typedom/CSSStyleValue.idl:34
> +    SkipVTableValidation,

Why?

> Source/WebCore/css/typedom/CSSUnitValue.h:44
> +    String toString() final { return makeString(String::number((int) m_value), m_unit); }

Do not call String::number(). makeString() is able to deal with numbers if you include StringConcatenateNumbers.h

> Source/WebCore/css/typedom/CSSUnitValue.h:45
> +    bool isUnitValue() final { return true; }

Should be private.

> Source/WebCore/css/typedom/CSSUnitValue.h:49
> +    String unit() const { return m_unit; }

can return a const String&.

> Source/WebCore/css/typedom/CSSUnitValue.h:50
> +    void setUnit(String unit) { m_unit = unit; }

Should take in a const String&.

> Source/WebCore/css/typedom/CSSUnparsedValue.h:44
> +    bool isUnparsedValue() final { return true; }

Should be private.

> Source/WebCore/css/typedom/CSSUnparsedValue.h:47
> +    CSSUnparsedValue(const String& serializedValue)

explicit

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:39
> +    static Ref<StylePropertyMapReadOnly> create(const HashMap<String, RefPtr<CSSStyleValue>>& map)

Why isn't the value a Ref<> ? Also, can't this take a HashMap&& instead of a const HashMap& for performance?

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:44
> +    RefPtr<CSSStyleValue> get(String property) { return m_map.get(property); }

Seems like this could return a CSSStyleValue* since you are not transferring ownership to the caller. Also, the method can probably be const.

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:47
> +    StylePropertyMapReadOnly(const HashMap<String, RefPtr<CSSStyleValue>>& map)

explicit.

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:77
> +    HashMap<String, RefPtr<CSSStyleValue>> propertyValues;

Why isn't the value a Ref<>?

[Justin Michaud]

Comment on attachment 354046 [details]
Patch

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

>> Source/WebCore/css/typedom/CSSStyleValue.idl:34
>> +    SkipVTableValidation,
> 
> Why?

We pass subclasses to toJS, so it crashes otherwise.

>> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:39
>> +    static Ref<StylePropertyMapReadOnly> create(const HashMap<String, RefPtr<CSSStyleValue>>& map)
> 
> Why isn't the value a Ref<> ? Also, can't this take a HashMap&& instead of a const HashMap& for performance?

I originally used a Ref, but our implementation of map requires operator= to work for some reason.

>> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:44
>> +    RefPtr<CSSStyleValue> get(String property) { return m_map.get(property); }
> 
> Seems like this could return a CSSStyleValue* since you are not transferring ownership to the caller. Also, the method can probably be const.

NullableType for an idl interface is a RefPtr. Is there anything I could do in the idl to make that work? Also, the type in the idl should technically be (undefined or CSSStyleValue), not CSSStyleValue?. Is there a way to do that in our IDL? The spec leaves it as any, with a link to a spec bug. https://drafts.css-houdini.org/css-typed-om/#stylepropertymapreadonly

[Justin Michaud]

Created attachment 354140 [details]
Patch

[Chris Dumez]

(In reply to Justin Michaud from comment #8)
> Comment on attachment 354046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354046&action=review
> 
> >> Source/WebCore/css/typedom/CSSStyleValue.idl:34
> >> +    SkipVTableValidation,
> > 
> > Why?
> 
> We pass subclasses to toJS, so it crashes otherwise.
> 
> >> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:39
> >> +    static Ref<StylePropertyMapReadOnly> create(const HashMap<String, RefPtr<CSSStyleValue>>& map)
> > 
> > Why isn't the value a Ref<> ? Also, can't this take a HashMap&& instead of a const HashMap& for performance?
> 
> I originally used a Ref, but our implementation of map requires operator= to
> work for some reason.

It should work fine if used correctly. I would suggest using Ref<> and trying to figure out why it does not build instead of using RefPtr.

> 
> >> Source/WebCore/css/typedom/StylePropertyMapReadOnly.h:44
> >> +    RefPtr<CSSStyleValue> get(String property) { return m_map.get(property); }
> > 
> > Seems like this could return a CSSStyleValue* since you are not transferring ownership to the caller. Also, the method can probably be const.
> 
> NullableType for an idl interface is a RefPtr. Is there anything I could do
> in the idl to make that work? 

Are you telling me that it does not build if you use CSSStyleValue*? That would surprise me.

> Also, the type in the idl should technically
> be (undefined or CSSStyleValue), not CSSStyleValue?. Is there a way to do
> that in our IDL? The spec leaves it as any, with a link to a spec bug.
> https://drafts.css-houdini.org/css-typed-om/#stylepropertymapreadonly

I would use "any" and CallWith=ExecState in the IDL, and have this method return a JSValue.

[Justin Michaud]

Created attachment 354155 [details]
Patch

[Chris Dumez]

Comment on attachment 354155 [details]
Patch

r=me if the bots are happy.

[Justin Michaud]

Thanks for the review! I have a windows linker error that I think might be related to unified sources that I am trying to debug with Per. After that, hopefully the bots should be happy.

(In reply to Chris Dumez from comment #10)
> It should work fine if used correctly. I would suggest using Ref<> and
> trying to figure out why it does not build instead of using RefPtr.
You are right. I was using a forward declaration before, and I guess that is why it didn't work. Now it's fixed!

> > Also, the type in the idl should technically
> > be (undefined or CSSStyleValue), not CSSStyleValue?. Is there a way to do
> > that in our IDL? The spec leaves it as any, with a link to a spec bug.
> > https://drafts.css-houdini.org/css-typed-om/#stylepropertymapreadonly
> 
> I would use "any" and CallWith=ExecState in the IDL, and have this method
> return a JSValue.

If it's OK with you, I'd like to leave it as-is. By the time StylePropertyMapReadOnly actually gets implemented according to the spec, I suspect that the spec issue about creating a type for (undefined or X) will be resolved.

Thanks!

[Chris Dumez]

(In reply to Justin Michaud from comment #13)
> Thanks for the review! I have a windows linker error that I think might be
> related to unified sources that I am trying to debug with Per. After that,
> hopefully the bots should be happy.
> 
> (In reply to Chris Dumez from comment #10)
> > It should work fine if used correctly. I would suggest using Ref<> and
> > trying to figure out why it does not build instead of using RefPtr.
> You are right. I was using a forward declaration before, and I guess that is
> why it didn't work. Now it's fixed!
> 
> > > Also, the type in the idl should technically
> > > be (undefined or CSSStyleValue), not CSSStyleValue?. Is there a way to do
> > > that in our IDL? The spec leaves it as any, with a link to a spec bug.
> > > https://drafts.css-houdini.org/css-typed-om/#stylepropertymapreadonly
> > 
> > I would use "any" and CallWith=ExecState in the IDL, and have this method
> > return a JSValue.
> 
> If it's OK with you, I'd like to leave it as-is. By the time
> StylePropertyMapReadOnly actually gets implemented according to the spec, I
> suspect that the spec issue about creating a type for (undefined or X) will
> be resolved.

Sure, I would not have r+ otherwise.

[Justin Michaud]

Created attachment 354188 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 354188 [details]
Patch

Rejecting attachment 354188 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 354188, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=354188&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=191309&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 354188 from bug 191309.
Fetching: https://bugs.webkit.org/attachment.cgi?id=354188
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 32 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/CMakeLists.txt
patching file Source/WebCore/DerivedSources.make
patching file Source/WebCore/Sources.txt
Hunk #2 succeeded at 2490 (offset 1 line).
Hunk #3 succeeded at 3164 (offset 1 line).
patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj
Hunk #2 succeeded at 7931 (offset 1 line).
Hunk #3 succeeded at 18107 (offset 3 lines).
Hunk #4 succeeded at 20300 (offset 3 lines).
Hunk #5 succeeded at 26107 (offset 5 lines).
Hunk #6 succeeded at 26214 (offset 5 lines).
Hunk #7 succeeded at 28211 (offset 5 lines).
Hunk #8 succeeded at 28253 (offset 5 lines).
Hunk #9 succeeded at 28263 (offset 5 lines).
Hunk #10 FAILED at 28681.
Hunk #11 FAILED at 30747.
Hunk #12 succeeded at 31042 (offset 6 lines).
2 out of 12 hunks FAILED -- saving rejects to file Source/WebCore/WebCore.xcodeproj/project.pbxproj.rej
patching file Source/WebCore/bindings/js/JSCSSStyleValueCustom.cpp
patching file Source/WebCore/bindings/js/WebCoreBuiltinNames.h
patching file Source/WebCore/bindings/js/WorkerScriptController.cpp
patching file Source/WebCore/css/CSSPaintCallback.h
patching file Source/WebCore/css/CSSPaintCallback.idl
patching file Source/WebCore/css/CSSPaintImageValue.cpp
patching file Source/WebCore/css/CSSPaintImageValue.h
patching file Source/WebCore/css/CSSPaintSize.h
patching file Source/WebCore/css/CSSPaintSize.idl
patching file Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp
patching file Source/WebCore/css/typedom/CSSNumericValue.h
patching file Source/WebCore/css/typedom/CSSNumericValue.idl
patching file Source/WebCore/css/typedom/CSSStyleValue.h
patching file Source/WebCore/css/typedom/CSSStyleValue.idl
patching file Source/WebCore/css/typedom/CSSUnitValue.h
patching file Source/WebCore/css/typedom/CSSUnitValue.idl
patching file Source/WebCore/css/typedom/CSSUnparsedValue.h
patching file Source/WebCore/css/typedom/CSSUnparsedValue.idl
patching file Source/WebCore/css/typedom/StylePropertyMapReadOnly.h
patching file Source/WebCore/css/typedom/StylePropertyMapReadOnly.idl
patching file Source/WebCore/platform/graphics/CustomPaintImage.cpp
patching file Source/WebCore/platform/graphics/CustomPaintImage.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/css-custom-paint/properties-expected.html
patching file LayoutTests/fast/css-custom-paint/properties.html
patching file LayoutTests/fast/css-custom-paint/worklet.html
patching file LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/typedom.tentative-expected.txt

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/9903588

[Justin Michaud]

Created attachment 354201 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 354201 [details]
Patch

Clearing flags on attachment: 354201

Committed r237981: <https://trac.webkit.org/changeset/237981>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45900538>