Bug 177099 - Remove the need for SetterMayThrowException
Summary: Remove the need for SetterMayThrowException
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-18 13:07 PDT by youenn fablet
Modified: 2017-09-27 12:55 PDT (History)
8 users (show)

See Also:


Attachments
WIP (13.84 KB, patch)
2017-09-18 13:14 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Proof of concept that works (1.55 KB, text/plain)
2017-09-18 16:32 PDT, Sam Weinig
no flags Details
Patch (118.93 KB, patch)
2017-09-21 11:45 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (112.21 KB, patch)
2017-09-21 16:14 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-09-18 13:07:21 PDT
Getting rid of SetterMayThrowException might be possible by inspecting the DOM class setter prototype.
Comment 1 youenn fablet 2017-09-18 13:14:46 PDT
Created attachment 321122 [details]
WIP
Comment 2 Build Bot 2017-09-18 13:18:04 PDT
Comment on attachment 321122 [details]
WIP

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

New failing tests:
(JS) JSTestCallTracer.cpp
(JS) JSTestCEReactions.cpp
(JS) JSTestCEReactionsStringifier.cpp
(JS) JSTestEnabledBySetting.cpp
(JS) JSTestGlobalObject.cpp
(JS) JSTestInterface.cpp
(JS) JSTestNode.cpp
(JS) JSTestObj.cpp
(JS) JSTestSerialization.cpp
(JS) JSTestSerializationInherit.cpp
(JS) JSTestSerializationInheritFinal.cpp
(JS) JSTestSerializedScriptValueInterface.cpp
(JS) JSTestStringifierReadWriteAttribute.cpp
(JS) JSTestTypedefs.cpp
Comment 3 youenn fablet 2017-09-18 14:07:29 PDT
Sam, Chris, any feedback on that patch before I finalize it?

This patch allows removing SetterMayThrowException and SetterCallWith from the binding generator.

A follow-up patch would clean the IDLs, remove GetterMayThrowException and probably CallWith from any attribute declaration.

The downside of the patch approach is that it reduces the flexibility on how to write setters. As can be seen in the patch, the removed flexibility is not huge.
Comment 4 youenn fablet 2017-09-18 14:07:36 PDT
Sam, Chris, any feedback on that patch before I finalize it?

This patch allows removing SetterMayThrowException and SetterCallWith from the binding generator.

A follow-up patch would clean the IDLs, remove GetterMayThrowException and probably CallWith from any attribute declaration.

The downside of the patch approach is that it reduces the flexibility on how to write setters. As can be seen in the patch, the removed flexibility is not huge.
Comment 5 youenn fablet 2017-09-18 14:31:16 PDT
If landing such patch, we should make sure there is no related perf regression.
Comment 6 Sam Weinig 2017-09-18 14:47:48 PDT
Rather than overloading like this, could instead do something like (I have not compiled this, this is a sketch):


template<bool canThrowException>
struct CallSetter;

template<>
struct CallSetter<true> {
    template<typename Functor>
    static void call(JSC::ExecState& state, JSC::ThrowScope& throwScope, Functor functor)
    {
        propagateException(state, throwScope, functor());
    }
};

template<>
struct CallSetter<false> {
    template<typename Functor>
    static void convert(JSC::ExecState&, JSC::ThrowScope&, Functor functor)
    {
        functor();
    }
};


---


CallSetter<WTF::IsTemplate<decltype(impl.setValues(WTFMove(nativeValue))), ExceptionOr>::value>::call(state, throwScope, [&] {
    return impl.setValue(WTFMove(nativeValue));
});
Comment 7 youenn fablet 2017-09-18 15:58:42 PDT
It is probably working and this would be probably applicable to methods as well.
It would not get rid of CallWith though.
Might be a good tradeoff.
Comment 8 Sam Weinig 2017-09-18 16:32:15 PDT
Created attachment 321151 [details]
Proof of concept that works

Working proof of concept.
Comment 9 youenn fablet 2017-09-18 17:13:06 PDT
(In reply to Sam Weinig from comment #8)
> Created attachment 321151 [details]
> Proof of concept that works
> 
> Working proof of concept.

Do you want to take over this one?
Otherwise, I will finalize the patch with your proposal.
Comment 10 Sam Weinig 2017-09-18 18:40:00 PDT
(In reply to youenn fablet from comment #9)
> (In reply to Sam Weinig from comment #8)
> > Created attachment 321151 [details]
> > Proof of concept that works
> > 
> > Working proof of concept.
> 
> Do you want to take over this one?
> Otherwise, I will finalize the patch with your proposal.

You should do it.
Comment 11 youenn fablet 2017-09-21 11:45:21 PDT
Created attachment 321457 [details]
Patch
Comment 12 Sam Weinig 2017-09-21 14:19:52 PDT
Comment on attachment 321457 [details]
Patch

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

> Source/WebCore/ChangeLog:56
> +        * bindings/scripts/test/JS/JSTestObj.cpp:
> +        (WebCore::setJSTestObjConstructorStaticStringAttrSetter):
> +        (WebCore::setJSTestObjEnumAttrSetter):
> +        (WebCore::setJSTestObjByteAttrSetter):
> +        (WebCore::setJSTestObjOctetAttrSetter):
> +        (WebCore::setJSTestObjShortAttrSetter):
> +        (WebCore::setJSTestObjClampedShortAttrSetter):

I think you can remove some of these function names that changed in test result lines.

> Source/WebCore/bindings/js/JSDOMAttribute.h:111
> +    static void call(JSC::ExecState&, JSC::ThrowScope&, Functor&& functor) { functor(); }

This should use the normal syntax, braces on their own lines.
Comment 13 Sam Weinig 2017-09-21 14:20:47 PDT
(In reply to Sam Weinig from comment #12)
> Comment on attachment 321457 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321457&action=review
> 
> > Source/WebCore/ChangeLog:56
> > +        * bindings/scripts/test/JS/JSTestObj.cpp:
> > +        (WebCore::setJSTestObjConstructorStaticStringAttrSetter):
> > +        (WebCore::setJSTestObjEnumAttrSetter):
> > +        (WebCore::setJSTestObjByteAttrSetter):
> > +        (WebCore::setJSTestObjOctetAttrSetter):
> > +        (WebCore::setJSTestObjShortAttrSetter):
> > +        (WebCore::setJSTestObjClampedShortAttrSetter):
> 
> I think you can remove some of these function names that changed in test
> result lines.
> 
> > Source/WebCore/bindings/js/JSDOMAttribute.h:111
> > +    static void call(JSC::ExecState&, JSC::ThrowScope&, Functor&& functor) { functor(); }
> 
> This should use the normal syntax, braces on their own lines.

Mind if I do the others?
Comment 14 youenn fablet 2017-09-21 14:31:39 PDT
(In reply to Sam Weinig from comment #13)
> (In reply to Sam Weinig from comment #12)
> > Comment on attachment 321457 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=321457&action=review
> > 
> > > Source/WebCore/ChangeLog:56
> > > +        * bindings/scripts/test/JS/JSTestObj.cpp:
> > > +        (WebCore::setJSTestObjConstructorStaticStringAttrSetter):
> > > +        (WebCore::setJSTestObjEnumAttrSetter):
> > > +        (WebCore::setJSTestObjByteAttrSetter):
> > > +        (WebCore::setJSTestObjOctetAttrSetter):
> > > +        (WebCore::setJSTestObjShortAttrSetter):
> > > +        (WebCore::setJSTestObjClampedShortAttrSetter):
> > 
> > I think you can remove some of these function names that changed in test
> > result lines.

OK

> > > Source/WebCore/bindings/js/JSDOMAttribute.h:111
> > > +    static void call(JSC::ExecState&, JSC::ThrowScope&, Functor&& functor) { functor(); }
> > 
> > This should use the normal syntax, braces on their own lines.

I like one-line getters/setters and it does seem very close to that.
But I don't have a strong opinion.

> Mind if I do the others?

Meaning to apply this for getters and operations? Sure!
I'll land this one and will update all IDLs using SetterRaisesException in a follow-up.
Comment 15 youenn fablet 2017-09-21 16:14:05 PDT
Created attachment 321489 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2017-09-21 16:37:12 PDT
Comment on attachment 321489 [details]
Patch for landing

Clearing flags on attachment: 321489

Committed r222365: <http://trac.webkit.org/changeset/222365>
Comment 17 WebKit Commit Bot 2017-09-21 16:37:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2017-09-27 12:55:44 PDT
<rdar://problem/34694289>