[webkit-reviews] review granted: [Bug 225060] Web Inspector: CSS Grid - measure usage of grid overlay options : [Attachment 427072] Patch

[bugzilla-daemon at webkit.org]

BJ Burg <bburg at apple.com> has granted Razvan Caliman <rcaliman at apple.com>'s
request for review:
Bug 225060: Web Inspector: CSS Grid - measure usage of grid overlay options
https://bugs.webkit.org/show_bug.cgi?id=225060

Attachment 427072: Patch

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




--- Comment #2 from BJ Burg <bburg at apple.com> ---
Comment on attachment 427072
  --> https://bugs.webkit.org/attachment.cgi?id=427072
Patch

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

r=me, nice work!

>
Source/WebInspectorUI/UserInterface/Controllers/GridOverlayConfigurationDiagnos
ticEventRecorder.js:50
> +	   window.addEventListener("focus", this, options);

Eventually we should consolidate our various places in the code that listen for
user interaction and have a class manage it. For now, it is fine to continue in
this direction until we encounter perf / user input lag.

>
Source/WebInspectorUI/UserInterface/Controllers/GridOverlayConfigurationDiagnos
ticEventRecorder.js:118
> +	   if (this._eventSamplingTimerIdentifier) {

Nit: call _stopEventSamplingTimer() rather than duplicating.

>
Source/WebInspectorUI/UserInterface/Controllers/GridOverlayConfigurationDiagnos
ticEventRecorder.js:123
> +	   this._eventSamplingTimerIdentifier =
setTimeout(this._sampleCurrentOverlayConfiguration.bind(this),
GridOverlayConfigurationDiagnosticEventRecorder.eventSamplingInterval);

Nit: WI.GridOverlayConfigurationDiagnosticEventRecorder

>
Source/WebInspectorUI/UserInterface/Controllers/GridOverlayConfigurationDiagnos
ticEventRecorder.js:126
> +    _stopEventSamplingTimer()

Nit: _stopEventSamplingTimerIfNeeded

>
Source/WebInspectorUI/UserInterface/Controllers/GridOverlayConfigurationDiagnos
ticEventRecorder.js:142
> +	       if
(WI.settings.debugAutoLogDiagnosticEvents.valueRespectingDebugUIAvailability)

Neat.

>
Source/WebInspectorUI/UserInterface/Controllers/GridOverlayConfigurationDiagnos
ticEventRecorder.js:155
> +	   // Encode the configuration of overaly options as a sum of
increasing powers of 10 for each overlay option that is enabled (zero if
disabled), convert to string and pad with zero if necessary.

Nit: overlay