Refactor ScriptController's proliferation of ExceptionDetails It's nonsensically everywhere and is getting in the way of upcoming feature work.
Created attachment 385479 [details] Patch
Created attachment 385480 [details] Patch
Created attachment 385507 [details] Patch
Created attachment 385510 [details] Patch
Comment on attachment 385510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385510&action=review > Source/WebCore/bindings/js/ScriptController.cpp:115 > +ValueAndException ScriptController::evaluateInWorld(const ScriptSourceCode& sourceCode, DOMWrapperWorld& world) It feels like this should return something called ValueOrException (which makes me wonder if the existing ExceptionOr<> class could be used here), since it looks like when an exception is thrown, the value is set to { }. If ExceptionOr<> isn't quite right, adding an ExceptionDetailsOr<> seems ok, or just using Expected<JSC::JSValue, ExceptionDetails>
Created attachment 385519 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 385522 [details] Patch
Created attachment 385523 [details] Patch
Created attachment 385524 [details] Patch
Created attachment 385525 [details] Patch
Created attachment 385533 [details] Patch
Finally got past other platform build errors. Now uploading with refreshed changelogs
Created attachment 385541 [details] Patch
I don't think this caused API tests QuotaDelegateReload and MediaCache to fail. I also can't identify how this patch would've cause the layout test changes the bot is still seeing. Keeping an eye on it...
(In reply to Sam Weinig from comment #5) > Comment on attachment 385510 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385510&action=review > > > Source/WebCore/bindings/js/ScriptController.cpp:115 > > +ValueAndException ScriptController::evaluateInWorld(const ScriptSourceCode& sourceCode, DOMWrapperWorld& world) > > It feels like this should return something called ValueOrException (which > makes me wonder if the existing ExceptionOr<> class could be used here), > since it looks like when an exception is thrown, the value is set to { }. Sorry I missed this earlier. The context missing from this standalone is that this is a precursor to a much larger patch that engrains ValueAndException(Details) in a callback. > ...or just using Expected<JSC::JSValue, ExceptionDetails> What is this even. Looking it up now.
(In reply to Brady Eidson from comment #16) > (In reply to Sam Weinig from comment #5) > > > ...or just using Expected<JSC::JSValue, ExceptionDetails> > > What is this even. > > Looking it up now. Never seen it before. Can use. 👍
Created attachment 385583 [details] Patch
Created attachment 385648 [details] Patch
Created attachment 385653 [details] Patch
Comment on attachment 385653 [details] Patch Clearing flags on attachment: 385653 Committed r253519: <https://trac.webkit.org/changeset/253519>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57933050>