Bug 174766

Summary: [WebIDL] Add support for generating timer bindings
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, cdumez, darin, dbates, esprehn+autocc, kondapallykalyan, mkwst, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Patch
none
Patch darin: review+

Description Sam Weinig 2017-07-23 17:42:22 PDT
[WebIDL] Add support for generating timer bindings
Comment 1 Sam Weinig 2017-07-23 17:59:58 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2017-07-23 19:04:15 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2017-07-23 19:04:17 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-07-23 19:10:59 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-07-23 19:11:00 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-07-23 19:29:04 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-07-23 19:29:06 PDT Comment hidden (obsolete)
Comment 8 Sam Weinig 2017-07-23 19:42:38 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-07-23 20:48:29 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-07-23 20:48:30 PDT Comment hidden (obsolete)
Comment 11 Sam Weinig 2017-07-23 21:17:57 PDT
Created attachment 316262 [details]
Patch
Comment 12 Sam Weinig 2017-07-23 21:39:12 PDT
Looks like I need to make JSC::Strong move aware.
Comment 13 Sam Weinig 2017-07-24 13:30:18 PDT
Created attachment 316314 [details]
Patch
Comment 14 Darin Adler 2017-07-24 16:01:27 PDT
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?
Comment 15 Sam Weinig 2017-07-24 16:46:50 PDT
(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.
Comment 16 Sam Weinig 2017-07-25 11:02:58 PDT
Committed r219873: <http://trac.webkit.org/changeset/219873>