Summary: | [Cocoa] Add _WKInspector SPI to set diagnostic logging delegate for a local Web Inspector | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||
Component: | WebKit API | Assignee: | BJ Burg <bburg> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, bburg, cdumez, commit-queue, hi, thorton, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
BJ Burg
2019-11-18 15:06:21 PST
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. |