Bug 205151 – Refactor ScriptController's proliferation of ExceptionDetails*

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Refactor ScriptController's proliferation of ExceptionDetails

It's nonsensically everywhere and is getting in the way of upcoming feature work.

[Brady Eidson]

Created attachment 385479 [details]
Patch

[Brady Eidson]

Created attachment 385480 [details]
Patch

[Brady Eidson]

Created attachment 385507 [details]
Patch

[Brady Eidson]

Created attachment 385510 [details]
Patch

[Sam Weinig]

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>

[Brady Eidson]

Created attachment 385519 [details]
Patch

[EWS Watchlist]

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

[Brady Eidson]

Created attachment 385522 [details]
Patch

[Brady Eidson]

Created attachment 385523 [details]
Patch

[Brady Eidson]

Created attachment 385524 [details]
Patch

[Brady Eidson]

Created attachment 385525 [details]
Patch

[Brady Eidson]

Created attachment 385533 [details]
Patch

[Brady Eidson]

Finally got past other platform build errors. Now uploading with refreshed changelogs

[Brady Eidson]

Created attachment 385541 [details]
Patch

[Brady Eidson]

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...

[Brady Eidson]

(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.

[Brady Eidson]

(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. 👍

[Brady Eidson]

Created attachment 385583 [details]
Patch

[Brady Eidson]

Created attachment 385648 [details]
Patch

[Brady Eidson]

Created attachment 385653 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 385653 [details]
Patch

Clearing flags on attachment: 385653

Committed r253519: <https://trac.webkit.org/changeset/253519>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57933050>