Bug 204430 – Web Inspector: add support for new kinds of diagnostic events

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

.

[Blaze Burg]

Created attachment 384074 [details]
Patch

[Devin Rousso]

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"});
```

[Blaze Burg]

(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"});
> ```

[Blaze Burg]

Committed r252790: <https://trac.webkit.org/changeset/252790>

[Radar WebKit Bug Importer]

<rdar://problem/57436497>