Bug 191309

Created attachment 353973 [details] Patch

Created attachment 354026 [details] Patch

Created attachment 354031 [details] Patch

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

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

Created attachment 354046 [details] Patch

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<>?

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

Created attachment 354140 [details] Patch

(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.

Created attachment 354155 [details] Patch

Comment on attachment 354155 [details] Patch r=me if the bots are happy.

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!

(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.

Created attachment 354188 [details] Patch

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 commit-queue@webkit.org... 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

Created attachment 354201 [details] Patch

Comment on attachment 354201 [details] Patch Clearing flags on attachment: 354201 Committed r237981: <https://trac.webkit.org/changeset/237981>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45900538>