Bug 205151

Summary: Refactor ScriptController's proliferation of ExceptionDetails*
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, berto, calvaris, cdumez, cgarcia, commit-queue, dbates, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gustavo, gyuyoung.kim, hi, japhet, jer.noble, joepeck, kangil.han, philipj, sam, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Brady Eidson 2019-12-11 18:45:04 PST
Refactor ScriptController's proliferation of ExceptionDetails

It's nonsensically everywhere and is getting in the way of upcoming feature work.
Comment 1 Brady Eidson 2019-12-11 22:09:58 PST
Created attachment 385479 [details]
Patch
Comment 2 Brady Eidson 2019-12-11 22:32:15 PST
Created attachment 385480 [details]
Patch
Comment 3 Brady Eidson 2019-12-12 09:58:49 PST
Created attachment 385507 [details]
Patch
Comment 4 Brady Eidson 2019-12-12 10:22:57 PST
Created attachment 385510 [details]
Patch
Comment 5 Sam Weinig 2019-12-12 10:58:53 PST
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>
Comment 6 Brady Eidson 2019-12-12 11:47:03 PST
Created attachment 385519 [details]
Patch
Comment 7 EWS Watchlist 2019-12-12 11:47:54 PST
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
Comment 8 Brady Eidson 2019-12-12 11:57:18 PST
Created attachment 385522 [details]
Patch
Comment 9 Brady Eidson 2019-12-12 12:12:41 PST
Created attachment 385523 [details]
Patch
Comment 10 Brady Eidson 2019-12-12 12:20:44 PST
Created attachment 385524 [details]
Patch
Comment 11 Brady Eidson 2019-12-12 12:25:02 PST
Created attachment 385525 [details]
Patch
Comment 12 Brady Eidson 2019-12-12 12:51:32 PST
Created attachment 385533 [details]
Patch
Comment 13 Brady Eidson 2019-12-12 13:31:57 PST
Finally got past other platform build errors. Now uploading with refreshed changelogs
Comment 14 Brady Eidson 2019-12-12 13:32:23 PST
Created attachment 385541 [details]
Patch
Comment 15 Brady Eidson 2019-12-12 19:40:58 PST
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...
Comment 16 Brady Eidson 2019-12-12 22:28:55 PST
(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.
Comment 17 Brady Eidson 2019-12-12 22:39:02 PST
(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. 👍
Comment 18 Brady Eidson 2019-12-12 23:59:41 PST
Created attachment 385583 [details]
Patch
Comment 19 Brady Eidson 2019-12-13 15:28:40 PST
Created attachment 385648 [details]
Patch
Comment 20 Brady Eidson 2019-12-13 16:15:05 PST
Created attachment 385653 [details]
Patch
Comment 21 WebKit Commit Bot 2019-12-13 19:12:19 PST
Comment on attachment 385653 [details]
Patch

Clearing flags on attachment: 385653

Committed r253519: <https://trac.webkit.org/changeset/253519>
Comment 22 WebKit Commit Bot 2019-12-13 19:12:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2019-12-13 19:13:24 PST
<rdar://problem/57933050>