Summary: | Implement DOMRect/DOMRectReadOnly | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||||
Component: | New Bugs | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | alex, buildbot, cdumez, commit-queue, darin, esprehn+autocc, kondapallykalyan, krit, rniwa, sam, simon.fraser, youennf | ||||||||||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 163473 | ||||||||||||||||||||
Bug Blocks: | 163467, 163505 | ||||||||||||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2016-10-14 15:58:24 PDT
Created attachment 291675 [details]
Patch
Does not apply? Created attachment 291680 [details]
Patch
Errors in generated code: -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSDOMRectReadOnly.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSDOMRectReadOnly.cpp:353:11: error: unused variable 'vm' [-Werror,-Wunused-variable] auto& vm = state->vm(); ^ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSDOMRectReadOnly.cpp:351:114: error: unused parameter 'thisObject' [-Werror,-Wunused-parameter] static inline EncodedJSValue jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) ^ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSDOMRectReadOnly.cpp:351:143: error: unused parameter 'throwScope' [-Werror,-Wunused-parameter] static inline EncodedJSValue jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) (In reply to comment #4) > Errors in generated code: > > -o > /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/ > Objects-normal/i386/JSDOMRectReadOnly.o > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > JSDOMRectReadOnly.cpp:353:11: error: unused variable 'vm' > [-Werror,-Wunused-variable] > auto& vm = state->vm(); > ^ > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > JSDOMRectReadOnly.cpp:351:114: error: unused parameter 'thisObject' > [-Werror,-Wunused-parameter] > static inline EncodedJSValue > jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, > JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) > > ^ > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > JSDOMRectReadOnly.cpp:351:143: error: unused parameter 'throwScope' > [-Werror,-Wunused-parameter] > static inline EncodedJSValue > jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, > JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) I would comment out the serializer in the IDL for now. Clearly our serializer support is not good. (In reply to comment #5) > (In reply to comment #4) > > Errors in generated code: > > > > -o > > /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/ > > Objects-normal/i386/JSDOMRectReadOnly.o > > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > > JSDOMRectReadOnly.cpp:353:11: error: unused variable 'vm' > > [-Werror,-Wunused-variable] > > auto& vm = state->vm(); > > ^ > > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > > JSDOMRectReadOnly.cpp:351:114: error: unused parameter 'thisObject' > > [-Werror,-Wunused-parameter] > > static inline EncodedJSValue > > jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, > > JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) > > > > ^ > > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > > JSDOMRectReadOnly.cpp:351:143: error: unused parameter 'throwScope' > > [-Werror,-Wunused-parameter] > > static inline EncodedJSValue > > jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, > > JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) > > I would comment out the serializer in the IDL for now. Clearly our > serializer support is not good. Also note that nothing prevents you from adding a toJSON() method to the IDL and implement its behavior by yourself. (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > Errors in generated code: > > > > > > -o > > > /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/ > > > Objects-normal/i386/JSDOMRectReadOnly.o > > > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > > > JSDOMRectReadOnly.cpp:353:11: error: unused variable 'vm' > > > [-Werror,-Wunused-variable] > > > auto& vm = state->vm(); > > > ^ > > > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > > > JSDOMRectReadOnly.cpp:351:114: error: unused parameter 'thisObject' > > > [-Werror,-Wunused-parameter] > > > static inline EncodedJSValue > > > jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, > > > JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) > > > > > > ^ > > > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > > > JSDOMRectReadOnly.cpp:351:143: error: unused parameter 'throwScope' > > > [-Werror,-Wunused-parameter] > > > static inline EncodedJSValue > > > jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, > > > JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) > > > > I would comment out the serializer in the IDL for now. Clearly our > > serializer support is not good. > > Also note that nothing prevents you from adding a toJSON() method to the IDL > and implement its behavior by yourself. ClientRect.idl and PerformanceTiming.idl already do this. Created attachment 291686 [details]
Patch
Created attachment 291687 [details]
Patch
Comment on attachment 291687 [details] Patch Attachment 291687 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2287499 New failing tests: js/dom/global-constructors-attributes-dedicated-worker.html Created attachment 291690 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 291687 [details] Patch Attachment 291687 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2287519 New failing tests: js/dom/global-constructors-attributes.html js/dom/global-constructors-attributes-dedicated-worker.html Created attachment 291691 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
(In reply to comment #4) > Errors in generated code: > > -o > /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/ > Objects-normal/i386/JSDOMRectReadOnly.o > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > JSDOMRectReadOnly.cpp:353:11: error: unused variable 'vm' > [-Werror,-Wunused-variable] > auto& vm = state->vm(); > ^ > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > JSDOMRectReadOnly.cpp:351:114: error: unused parameter 'thisObject' > [-Werror,-Wunused-parameter] > static inline EncodedJSValue > jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, > JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) > > ^ > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/ > JSDOMRectReadOnly.cpp:351:143: error: unused parameter 'throwScope' > [-Werror,-Wunused-parameter] > static inline EncodedJSValue > jsDOMRectReadOnlyPrototypeFunctionToJSONCaller(ExecState* state, > JSDOMRectReadOnly* thisObject, JSC::ThrowScope& throwScope) As explained this is not a supported behaviour, I think we missed the die in the parser for this case with the warning, like we do for inherit or getter keywords. > > > I would comment out the serializer in the IDL for now. Clearly our
> > > serializer support is not good.
> >
> > Also note that nothing prevents you from adding a toJSON() method to the IDL
> > and implement its behavior by yourself.
>
> ClientRect.idl and PerformanceTiming.idl already do this.
This gives some incentive to fix the binding generator then.
Created attachment 291752 [details]
Patch
Comment on attachment 291752 [details] Patch Attachment 291752 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2297478 New failing tests: js/dom/global-constructors-attributes.html Created attachment 291775 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
I've been following the constructor/from{} syntax in https://drafts.fxtf.org/geometry/, but maybe we need to be compatible with https://www.w3.org/TR/geometry-1/ That test just needs Mac results. Comment on attachment 291752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291752&action=review > Source/WebCore/dom/DOMRect.h:34 > + static Ref<DOMRect> create() { return adoptRef(*new DOMRect); } Do we need this? > Source/WebCore/dom/DOMRect.h:36 > + static Ref<DOMRect> create(const FloatRect& rect) { return adoptRef(*new DOMRect(rect)); } Do we need this yet? > Source/WebCore/dom/DOMRect.h:47 > + WEBCORE_EXPORT DOMRect(double x = 0, double y = 0, double width = 0, double height = 0) These defaults are only needed if we need the create() function above with no arguments. We do not need WEBCORE_EXPORT on an inline function. > Source/WebCore/dom/DOMRect.h:52 > + WEBCORE_EXPORT explicit DOMRect(const FloatRect& r) We do not need WEBCORE_EXPORT on an inline function. > Source/WebCore/dom/DOMRectReadOnly.h:39 > + static Ref<DOMRectReadOnly> create() { return adoptRef(*new DOMRectReadOnly); } Ditto. > Source/WebCore/dom/DOMRectReadOnly.h:41 > + static Ref<DOMRectReadOnly> create(const FloatRect& rect) { return adoptRef(*new DOMRectReadOnly(rect)); } Ditto. > Source/WebCore/dom/DOMRectReadOnly.h:54 > + double top() const { return WTF::nanPropagatingMin(m_y, m_y + m_height); } > + double right() const { return WTF::nanPropagatingMax(m_x, m_x + m_width); } > + double bottom() const { return WTF::nanPropagatingMax(m_y, m_y + m_height); } > + double left() const { return WTF::nanPropagatingMin(m_x, m_x + m_width); } Should not need WTF prefix if we follow the usual pattern in WTF of putting "using" in the header. Or we could start moving away from that pattern, but if so we should probably decide that across the board for the project. > Source/WebCore/dom/DOMRectReadOnly.h:57 > + WEBCORE_EXPORT DOMRectReadOnly(double x = 0, double y = 0, double width = 0, double height = 0) Same comments as above. > Source/WebCore/dom/DOMRectReadOnly.h:65 > + WEBCORE_EXPORT explicit DOMRectReadOnly(const FloatRect& r) We do not need WEBCORE_EXPORT on an inline function. > Source/WebCore/dom/DOMRectReadOnly.h:69 > + : m_x(r.x()) > + , m_y(r.y()) > + , m_width(r.width()) > + , m_height(r.height()) Could forward to the other constructor: : DOMRectReadOnly(r.x(), r.y(), r.width(), r.height()) > LayoutTests/js/dom/global-constructors-attributes-dedicated-worker-expected.txt:52 > +PASS [Worker] Object.getOwnPropertyDescriptor(global, 'DOMRect').value is DOMRect Need to update the other file with the same name. > LayoutTests/platform/mac/js/dom/global-constructors-attributes-expected.txt:336 > +PASS Object.getOwnPropertyDescriptor(global, 'DOMRect').value is DOMRect Need to update all the other files with the same name. |