[webkit-reviews] review granted: [Bug 217835] Web Inspector: move InspectorFrontendAPIDispatcher to WebCore, clean up uses : [Attachment 411613] 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 217835: Web Inspector: move InspectorFrontendAPIDispatcher to WebCore,
clean up uses
https://bugs.webkit.org/show_bug.cgi?id=217835

Attachment 411613: Patch v1

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




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

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

r=me this looks fine to me, so I'm gonna preemptively r+ on the assumption that
you'll fix build/test failures :)

please let me know if anything significant changes while doing so and I can
take another look

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

How would you feel about using a different name?  Maybe something like `invoke`
or `evaluate`?	Using "command" makes me thing this is related to protocol
commands (which I think go through `dispatchMessageAsync`).

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:396
>  bool InspectorFrontendClientLocal::evaluateAsBoolean(const String&
expression)

Why not also move this to `InspectorFrontendAPIDispatcher::evaluateAsBoolean`
(or with a different/better name)?

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:-433
> -    JSC::SuspendExceptionScope
scope(&m_frontendPage->inspectorController().vm());

Do we still need this somewhere?

> Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.cpp:75
>      StringBuilder builder;
>      JSON::Value::escapeString(builder, findString);

I think we can drop this now because you added a `writeJSON` call, which calls
`escapeString`.

Actually, keeping this may result in over-escaping ��

> Source/WebKit/WebProcess/Inspector/RemoteWebInspectorUI.h:95
> +    WebCore::InspectorFrontendAPIDispatcher& frontendAPIDispatcher() final {
return m_frontendAPIDispatcher; }

NIT: the `final` isn't necessary since this class is already `final`

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:247
> +    m_frontendAPIDispatcher.dispatchCommand("setDockSide"_s, {
JSON::Value::create(String(dockSideString)) });

NIT: is the `String` necessary?

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:267
>      StringBuilder builder;
>      JSON::Value::escapeString(builder, findString);

ditto (RemoteWebInspectorUI.cpp:74)

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.cpp:334
> +    m_frontendAPIDispatcher.dispatchCommand("showConsole"_s, { });

you can add `= { }` to the declaration of `dispatchCommand` to avoid having to
do this at callsites