.
Created attachment 383800 [details] Patch
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)`?
(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)`?
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.
Comment on attachment 383800 [details] Patch r=me
Comment on attachment 383800 [details] Patch Clearing flags on attachment: 383800 Committed r252637: <https://trac.webkit.org/changeset/252637>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57327182>