[webkit-reviews] review granted: [Bug 204430] Web Inspector: add support for new kinds of diagnostic events : [Attachment 384074] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 204430: Web Inspector: add support for new kinds of diagnostic events
https://bugs.webkit.org/show_bug.cgi?id=204430

Attachment 384074: Patch

https://bugs.webkit.org/attachment.cgi?id=384074&action=review




--- Comment #2 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 384074
  --> https://bugs.webkit.org/attachment.cgi?id=384074
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/TabActivityDiagnosticEventRecor
der.js:36
> +    // WI.DiagnosticEventRecorder

Style: we normally just use `// Protected`

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.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"});
```