[webkit-reviews] review granted: [Bug 217835] Web Inspector: move InspectorFrontendAPIDispatcher to WebCore, clean up uses : [Attachment 412761] Patch v2.4 - for EWS

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 217835: Web Inspector: move InspectorFrontendAPIDispatcher to WebCore,
clean up uses
https://bugs.webkit.org/show_bug.cgi?id=217835

Attachment 412761: Patch v2.4 - for EWS

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




--- Comment #14 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 412761
  --> https://bugs.webkit.org/attachment.cgi?id=412761
Patch v2.4 - for EWS

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

r=me, with two fixes for `updateFindString` (I still don't know how that's not
throwing a compiler error because of the unused variable o_O)

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp:85
> +	       if (!protectedThis->isSuspended())
> +		   return;

NIT: this check seems unnecessary since it's repeated in `unsuspend`

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:65
> +    EvaluationResult dispatchCommandSync(const String& command,
Vector<Ref<JSON::Value>>&& arguments = { });

NIT: wouldn't it be more accurate to call this `dispatchCommandWithResultSync`?

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:66
> +    WEBCORE_EXPORT void dispatchCommandWithResult(const String& command,
Vector<Ref<JSON::Value>>&& arguments, EvaluationResultHandler&&);

Seeing as `dispatchCommand` and `dispatchCommandWithResult` basically have the
same logic, could we combine them and avoid the repeated logic?
```
    EvaluationResult dispatchCommandWithResultSync(const String& command,
Vector<Ref<JSON::Value>>&& arguments = { });
    WEBCORE_EXPORT void dispatchCommandWithResultAsync(const String& command,
Vector<Ref<JSON::Value>>&& arguments = { }, EvaluationResultHandler&& = { });
```

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:79
> +    void evaluateOrQueueExpression(const String&,
WTF::Optional<EvaluationResultHandler>&& = WTF::nullopt);

it shouldn't be necessary to use `Optional` as `CompletionHandler` already has
a way to check "not set"

> Source/WebCore/inspector/InspectorFrontendAPIDispatcher.h:85
> +    Vector<std::pair<String, Optional<EvaluationResultHandler>>>
m_queuedEvaluations;

ditto (:79)

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:-188
> -    m_frontendLoaded = true;

Don't we still need this?  It's still used later.

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:331
> +    auto result =
m_frontendAPIDispatcher->dispatchCommandSync("isDebuggingEnabled"_s, { });

NIT: don't need the `{ }`

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:348
> +    auto result =
m_frontendAPIDispatcher->dispatchCommandSync("isTimelineProfilingEnabled"_s, {
});

ditto (:331)

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:369
> +    if (!result ||
!result.value().toBoolean(m_frontendAPIDispatcher->frontendGlobalObject()))
> +	   return false;
> +
> +    return true;

NIT: you could simplify this
```
    return result &&
result.value().toBoolean(m_frontendAPIDispatcher->frontendGlobalObject());
```

> Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.cpp:75
> +    m_frontendAPIDispatcher->dispatchCommand("updateFindString"_s, {
JSON::Value::create(builder.toString()) });

this should use `findString`, not `builder`

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:267
> +    m_frontendAPIDispatcher->dispatchCommand("updateFindString"_s, {
JSON::Value::create(builder.toString()) });

this should use `findString`, not `builder`

> LayoutTests/inspector/dom/dom-remove-events.html:74
> +	       delete InspectorProtocol.eventHandler["DOM.setChildNodes"];

I wonder if we should just change `ProtocolTest.completeTest` to automatically
nuke `InspectorProtocol.eventHandler` and dispatch `disable` commands to every
domain ��