Bug 204323 – [Cocoa] Add _WKInspector SPI to set diagnostic logging delegate for a local Web Inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Blaze Burg]

.

[Blaze Burg]

Created attachment 383800 [details]
Patch

[Devin Rousso]

Comment on attachment 383800 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:80
> +        // FIXME: update diagnostic availability and start/stop diagnostic recorders.

Why not fill this in as part of this patch?  I'd imaging having an `_enabled` member on `WI.DiagnosticController` and moving the logic inside it's constructor to an `enable` function (which is called if `available` is truthy) wouldn't be too large of a diff.  This way, until we do fill this in, we aren't even trying to record diagnostic information, which is a complete waste right now.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorPrivate.h:32
> +@property (nonatomic, weak, setter=_setDiagnosticLoggingDelegate:) id<_WKDiagnosticLoggingDelegate> _diagnosticLoggingDelegate;

Shouldn't this also be wrapped by `#if ENABLE(INSPECTOR_TELEMETRY)`?

[Blaze Burg]

(In reply to Devin Rousso from comment #2)
> Comment on attachment 383800 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383800&action=review
> 
> > Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:80
> > +        // FIXME: update diagnostic availability and start/stop diagnostic recorders.
> 
> Why not fill this in as part of this patch?  I'd imaging having an
> `_enabled` member on `WI.DiagnosticController` and moving the logic inside
> it's constructor to an `enable` function (which is called if `available` is
> truthy) wouldn't be too large of a diff.  This way, until we do fill this
> in, we aren't even trying to record diagnostic information, which is a
> complete waste right now.

Due to priority changes I had to declare patch bankruptcy and put up everything in my branch.

I'm adding our other initial records presently so this will be coming up soon.

> > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorPrivate.h:32
> > +@property (nonatomic, weak, setter=_setDiagnosticLoggingDelegate:) id<_WKDiagnosticLoggingDelegate> _diagnosticLoggingDelegate;
> 
> Shouldn't this also be wrapped by `#if ENABLE(INSPECTOR_TELEMETRY)`?

[Blaze Burg]

Comment on attachment 383800 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorPrivate.h:32
>>> +@property (nonatomic, weak, setter=_setDiagnosticLoggingDelegate:) id<_WKDiagnosticLoggingDelegate> _diagnosticLoggingDelegate;
>> 
>> Shouldn't this also be wrapped by `#if ENABLE(INSPECTOR_TELEMETRY)`?
> 
> 

It is not allowed to have feature guards in private headers, other than TARGET_OS_* and other system defined macros.

So, it's only guarded out in shared code.

[Devin Rousso]

Comment on attachment 383800 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 383800 [details]
Patch

Clearing flags on attachment: 383800

Committed r252637: <https://trac.webkit.org/changeset/252637>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57327182>