Bug 163464

Summary: Implement DOMRect/DOMRectReadOnly
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Patch
darin: review+, buildbot: commit-queue-
Archive of layout-test-results from ews105 for mac-yosemite-wk2 none

Description Simon Fraser (smfr) 2016-10-14 15:58:24 PDT
Implement DOMRect/DOMRectReadOnly
Comment 1 Simon Fraser (smfr) 2016-10-14 16:00:13 PDT
Created attachment 291675 [details]
Patch
Comment 2 Chris Dumez 2016-10-14 16:26:19 PDT
Does not apply?
Comment 3 Simon Fraser (smfr) 2016-10-14 16:38:29 PDT
Created attachment 291680 [details]
Patch
Comment 4 Simon Fraser (smfr) 2016-10-14 16:43:43 PDT
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)
Comment 5 Chris Dumez 2016-10-14 16:44:41 PDT
(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.
Comment 6 Chris Dumez 2016-10-14 16:45:41 PDT
(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.
Comment 7 Chris Dumez 2016-10-14 16:49:59 PDT
(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.
Comment 8 Simon Fraser (smfr) 2016-10-14 17:27:17 PDT
Created attachment 291686 [details]
Patch
Comment 9 Simon Fraser (smfr) 2016-10-14 17:34:57 PDT
Created attachment 291687 [details]
Patch
Comment 10 Build Bot 2016-10-14 18:22:06 PDT
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
Comment 11 Build Bot 2016-10-14 18:22:10 PDT
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 12 Build Bot 2016-10-14 18:26:43 PDT
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
Comment 13 Build Bot 2016-10-14 18:26:48 PDT
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
Comment 14 Alejandro G. Castro 2016-10-14 23:43:02 PDT
(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.
Comment 15 youenn fablet 2016-10-15 03:33:06 PDT
> > > 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.
Comment 16 Simon Fraser (smfr) 2016-10-16 11:23:26 PDT
Created attachment 291752 [details]
Patch
Comment 17 Build Bot 2016-10-16 12:42:34 PDT
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
Comment 18 Build Bot 2016-10-16 12:42:39 PDT
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
Comment 19 Simon Fraser (smfr) 2016-10-16 13:40:54 PDT
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/
Comment 20 Simon Fraser (smfr) 2016-10-16 14:07:06 PDT
That test just needs Mac results.
Comment 21 Darin Adler 2016-10-17 10:36:25 PDT
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.
Comment 22 Simon Fraser (smfr) 2016-10-17 15:34:57 PDT
https://trac.webkit.org/r207438