Bug 163464 – Implement DOMRect/DOMRectReadOnly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Implement DOMRect/DOMRectReadOnly

[Simon Fraser (smfr)]

Created attachment 291675 [details]
Patch

[Chris Dumez]

Does not apply?

[Simon Fraser (smfr)]

Created attachment 291680 [details]
Patch

[Simon Fraser (smfr)]

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)

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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

[Simon Fraser (smfr)]

Created attachment 291686 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 291687 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Alejandro G. Castro]

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

[youenn fablet]

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

[Simon Fraser (smfr)]

Created attachment 291752 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Simon Fraser (smfr)]

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/

[Simon Fraser (smfr)]

That test just needs Mac results.

[Darin Adler]

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.

[Simon Fraser (smfr)]

https://trac.webkit.org/r207438