[webkit-reviews] review granted: [Bug 219378] Web Inspector: InspectorFrontendAPIDispatcher should not ignore all exceptions : [Attachment 415074] Patch v1

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 219378: Web Inspector: InspectorFrontendAPIDispatcher should not ignore all
exceptions
https://bugs.webkit.org/show_bug.cgi?id=219378

Attachment 415074: Patch v1

https://bugs.webkit.org/attachment.cgi?id=415074&action=review




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 415074
  --> https://bugs.webkit.org/attachment.cgi?id=415074
Patch v1

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

r=mews

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:328
> +Optional<bool>
InspectorFrontendClientLocal::evaluationResultToBoolean(InspectorFrontendAPIDis
patcher::EvaluationResult result)

Rather than `Optional<bool>` should we always just `.valueOr(false)`?

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:334
> +    if (valueOrException)

I'd make this `if (!valueOrException)` so that we early-return for failure
(that seems to be our usual pattern too)

> Source/WebCore/inspector/InspectorFrontendClientLocal.h:137
> +    Optional<bool>
evaluationResultToBoolean(InspectorFrontendAPIDispatcher::EvaluationResult);

Why not make this `static` in the `.cpp`?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:59
> +    if (!result.has_value()) {

I don't think the `has_value` is needed as `operator bool` does the same thing

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:65
> +	   default:

NIT: I personally prefer avoiding `default` so that if new enum values are
added then there's a compile error (which brings visibility to the usage here,
causing the programmer to make a conscious decision about it instead of it
being silent)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:77
> +    if (!valueOrException.has_value()) {

ditto (:59)