Bug 204371 – [Cocoa] Add _WKRemoteWebInspectorViewController SPI to set diagnostic logging delegate

• Component: WebKit API
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Blaze Burg]

Created attachment 383923 [details]
Patch

[Blaze Burg]

Created attachment 383925 [details]
Patch

[Devin Rousso]

Comment on attachment 383925 [details]
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?

[EWS Watchlist]

Comment on attachment 383925 [details]
Patch

Attachment 383925 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13268669

New failing tests:
fast/dom/domTokenListIterator.html
fast/dom/nodeListIterator.html
fast/dom/NodeList/nodelist-iterable.html
http/tests/security/cross-frame-access-enumeration.html

[EWS Watchlist]

Created attachment 383950 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Blaze Burg]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 383925 [details]
> 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".

OK 

> 
> > 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?

I never know which is preferable if the state is simply copied. I'll use the member.

> 
> > 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?

Oops forgot to mention in ChangeLog. I mistakenly put it below in the last patch, but it should be in the upper part with other methods that are mainly called via IPC messages.

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

[Blaze Burg]

Committed r252702: <https://trac.webkit.org/changeset/252702>

[Radar WebKit Bug Importer]

<rdar://problem/57368960>