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
Brady Eidson
2019-12-11 18:45:04 PST
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. |