Bug 174766 – [WebIDL] Add support for generating timer bindings

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

[WebIDL] Add support for generating timer bindings

[Sam Weinig]

Created attachment 316246 [details]
Patch

[Build Bot]

Comment on attachment 316246 [details]
Patch

Attachment 316246 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4175695

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 316248 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 316246 [details]
Patch

Attachment 316246 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4175699

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Build Bot]

Comment on attachment 316246 [details]
Patch

Attachment 316246 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4175707

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 316251 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

Created attachment 316253 [details]
Patch

[Build Bot]

Comment on attachment 316253 [details]
Patch

Attachment 316253 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4176091

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 316259 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

Created attachment 316262 [details]
Patch

[Sam Weinig]

Looks like I need to make JSC::Strong move aware.

[Sam Weinig]

Created attachment 316314 [details]
Patch

[Darin Adler]

Comment on attachment 316314 [details]
Patch

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

I took the liberty of looking this over.

> Source/WebCore/bindings/js/JSDOMConvertInterface.h:77
> +        return std::optional<Item>(*result);

Surprised that you used parentheses here instead of curly braces.

Oh, I see now that this is moved code, not new code, but I am still l guessing it’s code you wrote.

> Source/WebCore/bindings/js/JSDOMConvertScheduledAction.h:37
> +template<> struct Converter<IDLScheduledAction> : DefaultConverter<IDLScheduledAction> {
> +
> +    static std::unique_ptr<ScheduledAction> convert(JSC::ExecState& state, JSC::JSValue value, JSDOMGlobalObject& globalObject)

I’d omit this stray blank line.

> Source/WebCore/bindings/js/JSDOMConvertScheduledAction.h:43
> +        if (JSC::getCallData(value, callData) == JSC::CallType::None) {

Argument dependent lookup means you should not need the JSC:: on the getCallData call here.

> Source/WebCore/bindings/js/ScheduledAction.h:40
> +    WTF_MAKE_NONCOPYABLE(ScheduledAction); WTF_MAKE_FAST_ALLOCATED;

I don’t think we need WTF_MAKE_NONCOPYABLE because this has a Ref as a member.

> Source/WebCore/bindings/js/ScheduledAction.h:51
> +    enum Type {
> +        Code,
> +        Function
>      };

Maybe do this as a one-liner instead of vertical?

[Sam Weinig]

(In reply to Darin Adler from comment #14)
> Comment on attachment 316314 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316314&action=review
> 
> I took the liberty of looking this over.
> 
> > Source/WebCore/bindings/js/JSDOMConvertInterface.h:77
> > +        return std::optional<Item>(*result);
> 
> Surprised that you used parentheses here instead of curly braces.
> 
> Oh, I see now that this is moved code, not new code, but I am still l
> guessing it’s code you wrote.
> 
> > Source/WebCore/bindings/js/JSDOMConvertScheduledAction.h:37
> > +template<> struct Converter<IDLScheduledAction> : DefaultConverter<IDLScheduledAction> {
> > +
> > +    static std::unique_ptr<ScheduledAction> convert(JSC::ExecState& state, JSC::JSValue value, JSDOMGlobalObject& globalObject)
> 
> I’d omit this stray blank line.
> 
> > Source/WebCore/bindings/js/JSDOMConvertScheduledAction.h:43
> > +        if (JSC::getCallData(value, callData) == JSC::CallType::None) {
> 
> Argument dependent lookup means you should not need the JSC:: on the
> getCallData call here.
> 
> > Source/WebCore/bindings/js/ScheduledAction.h:40
> > +    WTF_MAKE_NONCOPYABLE(ScheduledAction); WTF_MAKE_FAST_ALLOCATED;
> 
> I don’t think we need WTF_MAKE_NONCOPYABLE because this has a Ref as a
> member.
> 
> > Source/WebCore/bindings/js/ScheduledAction.h:51
> > +    enum Type {
> > +        Code,
> > +        Function
> >      };
> 
> Maybe do this as a one-liner instead of vertical?

Took all the suggestions.

[Sam Weinig]

Committed r219873: <http://trac.webkit.org/changeset/219873>