Bug 223256 – Web Inspector: add GridOverlay diagnostic event and related hooks

• Component: Web Inspector
• Status: RESOLVED FIXED

[Razvan Caliman]

Add telemetry for usage of CSS Grid Inspector

[Radar WebKit Bug Importer]

<rdar://problem/75478239>

[Razvan Caliman]

Created attachment 423335 [details]
Patch

patch for requesting feedback

[Blaze Burg]

Comment on attachment 423335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423335&action=review

> Source/WebInspectorUI/UserInterface/Base/Main.js:587
> +        WI.diagnosticController.addRecorder(new WI.GridOverlayDiagnosticEventRecorder(WI.diagnosticController));

Nit: add backend feature check (InspectorBackend.supports("DOM.showGridOverlay") or similar)

> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayDiagnosticEventRecorder.js:65
> +                options[value] = value;

As discussed, we should always include all the keys we know about and use 0 or 1 for booleans.

What do you think about collecting whether the grid overlay was drawn with a non-default color? (Probably need to query the overlay manager to determine this in a sane way)

> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayDiagnosticEventRecorder.js:74
> +WI.GridOverlayDiagnosticEventRecorder.TriggerType = {

Let's call this 'Initiator' which is a more common terminology in our codebase for hints like this.

[Devin Rousso]

Comment on attachment 423335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423335&action=review

>> Source/WebInspectorUI/UserInterface/Base/Main.js:587
>> +        WI.diagnosticController.addRecorder(new WI.GridOverlayDiagnosticEventRecorder(WI.diagnosticController));
> 
> Nit: add backend feature check (InspectorBackend.supports("DOM.showGridOverlay") or similar)

NIT: I'd also put this last (grid overlay is likely not as important/used as target types or tab stuff)

> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayDiagnosticEventRecorder.js:53
> +        // While changing the overlay color, WI.OverlayManager.Event.GridOverlayShown is dispatched with high frequency.
> +        // A trigger is not provided so as not to skew usage count of overlay options.

I feel like this would be better at the call site, not here.

> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayDiagnosticEventRecorder.js:57
> +        let usedOptions = [

IMO this is kinda odd.  Is there a way to do this more declaratively without `reduce`?

> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayDiagnosticEventRecorder.js:62
> +            "showAreaNames"

Style: missing comma

[Razvan Caliman]

Created attachment 423356 [details]
Patch

[Razvan Caliman]

Comment on attachment 423335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423335&action=review

>>> Source/WebInspectorUI/UserInterface/Base/Main.js:587
>>> +        WI.diagnosticController.addRecorder(new WI.GridOverlayDiagnosticEventRecorder(WI.diagnosticController));
>> 
>> Nit: add backend feature check (InspectorBackend.supports("DOM.showGridOverlay") or similar)
> 
> NIT: I'd also put this last (grid overlay is likely not as important/used as target types or tab stuff)

@BJ, I think you meant `InspectorBackend.hasCommand("DOM.showGridOverlay")`
@Devin, ok.

>> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayDiagnosticEventRecorder.js:53
>> +        // A trigger is not provided so as not to skew usage count of overlay options.
> 
> I feel like this would be better at the call site, not here.

Good idea. Moved.

>> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayDiagnosticEventRecorder.js:57
>> +        let usedOptions = [
> 
> IMO this is kinda odd.  Is there a way to do this more declaratively without `reduce`?

Declarative done.

>> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayDiagnosticEventRecorder.js:65
>> +                options[value] = value;
> 
> As discussed, we should always include all the keys we know about and use 0 or 1 for booleans.
> 
> What do you think about collecting whether the grid overlay was drawn with a non-default color? (Probably need to query the overlay manager to determine this in a sane way)

Made them integers with the expectation that these will be summed-up over time. That's what we're interested in anyway: which options are most used.
We may switch to booleans if we expect to use them as such and if the views allow us to filter the data as we need it.

>> Source/WebInspectorUI/UserInterface/Controllers/GridOverlayDiagnosticEventRecorder.js:74
>> +WI.GridOverlayDiagnosticEventRecorder.TriggerType = {
> 
> Let's call this 'Initiator' which is a more common terminology in our codebase for hints like this.

Done!

[Blaze Burg]

Comment on attachment 423356 [details]
Patch

r=me

[Blaze Burg]

Committed r274592 (235433@main): <https://commits.webkit.org/235433@main>