[webkit-reviews] review granted: [Bug 204371] [Cocoa] Add _WKRemoteWebInspectorViewController SPI to set diagnostic logging delegate : [Attachment 383925] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 204371: [Cocoa] Add _WKRemoteWebInspectorViewController SPI to set
diagnostic logging delegate
https://bugs.webkit.org/show_bug.cgi?id=204371

Attachment 383925: Patch

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




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

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

r=me, overall looks good :)

> Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:186
> +    // Inspector's diagnostic logging client will never be used unless the
page setting is also enabled.

I think it's more accurate to say "should never be used".

> Source/WebKit/WebProcess/WebPage/RemoteWebInspectorUI.cpp:190
> +   
m_frontendAPIDispatcher.dispatchCommand("setDiagnosticLoggingAvailable"_s,
available);

NIT: do you want to use `m_diagnosticLoggingAvailable` instead, as that's this
object's actual state, instead of a parameter?

> Source/WebKit/WebProcess/WebPage/WebInspectorUI.cpp:306
> +    // Inspector's diagnostic logging client will never be used unless the
page setting is also enabled.

Ditto (RemoteWebInspectorUI.cpp:186)

> Source/WebKit/WebProcess/WebPage/WebInspectorUI.cpp:310
>     
m_frontendAPIDispatcher.dispatchCommand("setDiagnosticLoggingAvailable"_s,
available);

Ditto (RemoteWebInspectorUI.cpp:190)

> Source/WebKit/WebProcess/WebPage/WebInspectorUI.h:87
> +#if ENABLE(INSPECTOR_TELEMETRY)
> +    void setDiagnosticLoggingAvailable(bool avaliable);
> +#endif

Why did this move?

> Source/WebKit/WebProcess/WebPage/WebInspectorUI.h:160
> +
> +

Oops?