.
Created attachment 384074 [details] Patch
Comment on attachment 384074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384074&action=review r=me, nice work! > Source/WebKit/ChangeLog:8 > + * WebKit.xcodeproj/project.pbxproj: Sort the project after recent changes. Is this really necessary in this patch? 😅 > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:30 > + console.assert(InspectorFrontendHost.supportsDiagnosticLogging); Will this fail when running inspector tests? > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:35 > + this._autoLogDiagnosticEventsToConsole = WI.settings.debugAutoLogDiagnosticEvents.value; We normally wrap debug settings with a check for `WI.isDebugUIEnabled()` so that production/engineering builds aren't able to configure these values. ``` this._autoLogDiagnosticEventsToConsole = WI.isDebugUIEnabled() ? WI.settings.debugAutoLogDiagnosticEvents.value : WI.settings.debugAutoLogDiagnosticEvents.defaultValue; ``` > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:44 > + get diagnosticLoggingAvailable() { return this._diagnosticLoggingAvailable; } Style: if one of a get-set pair is multi-line, we normally make both multi-line for consistency > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:47 > + if (this._diagnosticLoggingAvailable == available) Style: `===` > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:56 > + this._recorders.add(recorder); Should we `console.assert(!this._recorders.has(recorder));`? > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:78 > + this._updateRecorderStates(); Ditto (35) > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:83 > + this._autoLogDiagnosticEventsToConsole = WI.settings.debugAutoLogDiagnosticEvents.value; Ditto (35) > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:88 > + let isActive = this._diagnosticLoggingAvailable && WI.settings.debugEnableDiagnosticLogging.value; Ditto (35) > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticEventRecorder.js:30 > + console.assert(controller instanceof WI.DiagnosticController, controller); Should we also `console.assert(InspectorFrontendHost.supportsDiagnosticLogging);`? > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticEventRecorder.js:40 > + get active() { return this._active; } Ditto (DiagnosticController.js:44) > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticEventRecorder.js:44 > + if (this._active == value) Style: `===` > Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:36 > + // WI.DiagnosticEventRecorder Style: we normally just use `// Protected` > Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:70 > + handleEvent(event) This should really be in a `// Public` section given that it's called by external code. Also, can we please not use `handleEvent`? It makes it harder for people to follow/search code, as it's a totally invisible and sorta edge-case feature of DOM events. I'd far prefer we use the various callback names directly when adding/removing event listeners. > LayoutTests/inspector/unit-tests/diagnostic-controller.html:89 > + recorder.sendEvent("TestEvent.3", {"answer": 42}); Rather than use `42`, could we instead have text in the event that indicates whether we should expect to see it in the output or not? ``` InspectorTest.log("Triggering a diagnostic event that should be logged..."); recorder.sendEvent("TestEvent.2", {"answer": "should be logged"}); InspectorTest.log("Making diagnostics not available..."); diagnosticController.diagnosticLoggingAvailable = false; InspectorTest.log("Triggering a diagnostic event that should not be logged..."); recorder.sendEvent("TestEvent.3", {"answer": "should not be logged"}); ```
(In reply to Devin Rousso from comment #2) > Comment on attachment 384074 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=384074&action=review > > r=me, nice work! > > > Source/WebKit/ChangeLog:8 > > + * WebKit.xcodeproj/project.pbxproj: Sort the project after recent changes. > > Is this really necessary in this patch? 😅 I landed this separately. > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:30 > > + console.assert(InspectorFrontendHost.supportsDiagnosticLogging); > > Will this fail when running inspector tests? It's incorrect, so I removed it. > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:35 > > + this._autoLogDiagnosticEventsToConsole = WI.settings.debugAutoLogDiagnosticEvents.value; > > We normally wrap debug settings with a check for `WI.isDebugUIEnabled()` so > that production/engineering builds aren't able to configure these values. > ``` > this._autoLogDiagnosticEventsToConsole = WI.isDebugUIEnabled() ? > WI.settings.debugAutoLogDiagnosticEvents.value : > WI.settings.debugAutoLogDiagnosticEvents.defaultValue; > ``` > > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:44 > > + get diagnosticLoggingAvailable() { return this._diagnosticLoggingAvailable; } > > Style: if one of a get-set pair is multi-line, we normally make both > multi-line for consistency > > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:47 > > + if (this._diagnosticLoggingAvailable == available) > > Style: `===` > > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:56 > > + this._recorders.add(recorder); > > Should we `console.assert(!this._recorders.has(recorder));`? > > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:78 > > + this._updateRecorderStates(); > > Ditto (35) > > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:83 > > + this._autoLogDiagnosticEventsToConsole = WI.settings.debugAutoLogDiagnosticEvents.value; > > Ditto (35) > > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticController.js:88 > > + let isActive = this._diagnosticLoggingAvailable && WI.settings.debugEnableDiagnosticLogging.value; If this is really the pattern to use... then we should add a different getter that does it automatically. > Ditto (35) > > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticEventRecorder.js:30 > > + console.assert(controller instanceof WI.DiagnosticController, controller); > > Should we also > `console.assert(InspectorFrontendHost.supportsDiagnosticLogging);`? > > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticEventRecorder.js:40 > > + get active() { return this._active; } > > Ditto (DiagnosticController.js:44) > > > Source/WebInspectorUI/UserInterface/Controllers/DiagnosticEventRecorder.js:44 > > + if (this._active == value) > > Style: `===` > > > Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:36 > > + // WI.DiagnosticEventRecorder > > Style: we normally just use `// Protected` > > > Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecorder.js:70 > > + handleEvent(event) > > This should really be in a `// Public` section given that it's called by > external code. > > Also, can we please not use `handleEvent`? It makes it harder for people to > follow/search code, as it's a totally invisible and sorta edge-case feature > of DOM events. I'd far prefer we use the various callback names directly > when adding/removing event listeners. Hard shrug. handleEvent is annoying, but so is binding all event listeners and storing them simply to make code slightly easier to search. I found many instances of handleEvent in user event handling code, and just one instance where we bother to bind a class instance method. > > > LayoutTests/inspector/unit-tests/diagnostic-controller.html:89 > > + recorder.sendEvent("TestEvent.3", {"answer": 42}); > > Rather than use `42`, could we instead have text in the event that indicates > whether we should expect to see it in the output or not? Good point! > ``` > InspectorTest.log("Triggering a diagnostic event that should be > logged..."); > recorder.sendEvent("TestEvent.2", {"answer": "should be logged"}); > > InspectorTest.log("Making diagnostics not available..."); > diagnosticController.diagnosticLoggingAvailable = false; > > InspectorTest.log("Triggering a diagnostic event that should not be > logged..."); > recorder.sendEvent("TestEvent.3", {"answer": "should not be logged"}); > ```
Committed r252790: <https://trac.webkit.org/changeset/252790>
<rdar://problem/57436497>