[webkit-reviews] review granted: [Bug 204531] Web Inspector: TabActivity diagnostic event should sample the active tab uniformly : [Attachment 384204] 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 204531: Web Inspector: TabActivity diagnostic event should sample the
active tab uniformly
https://bugs.webkit.org/show_bug.cgi?id=204531

Attachment 384204: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 384204
  --> https://bugs.webkit.org/attachment.cgi?id=384204
Patch

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

r=me

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.js:36
> +	   this._eventSamplingTimerIdentifier = 0;
> +	   this._initialDelayBeforeSamplingTimerIdentifier = 0;

Given that we reset to `undefined`, we should start out with `undefined`.

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.js:51
> +	   this._startInitialDelayBeforeSamplingTimer();

NIT: shouldn't this use `_startEventSamplingTimer` if `setup` is called way
after Web Inspector is opened?

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.js:98
> +	   this._initialDelayBeforeSamplingTimerIdentifier =
setTimeout(this._sampleCurrentTabActivity.bind(this),
WI.TabActivityDiagnosticEventRecorder.InitialDelayBeforeSamplingInterval);

NIT: you can drop the `WI`

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.js:116
> +	   this._eventSamplingTimerIdentifier =
setTimeout(this._sampleCurrentTabActivity.bind(this),
WI.TabActivityDiagnosticEventRecorder.EventSamplingInterval);

Ditto (98)

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.js:134
> +	   let intervalSinceLastUserInteraction = Date.now() -
this._lastUserInteractionTimestamp;

Should we use `performance.now()` instead?

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.js:135
> +	   if (intervalSinceLastUserInteraction >
WI.TabActivityDiagnosticEventRecorder.EventSamplingInterval) {

Ditto (98)

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.js:136
> +	       if
(WI.settings.debugAutoLogDiagnosticEvents.valueRespectingDebugUIAvailability)

Please add another check for `WI.isDebugUIEnabled()`.

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.js:156
> +	   this._lastUserInteractionTimestamp = Date.now();

Ditto (134)

>
Source/WebInspectorUI/UserInterface/Controllers/TabActivityDiagnosticEventRecor
der.js:187
> +WI.TabActivityDiagnosticEventRecorder.InitialDelayBeforeSamplingInterval =
10 * 1000;

Rather than add logic to wait 10s, could we just move the `setup()` call to
right after `InspectorFrontendAPI.loadCompleted();` in `WI.contentLoaded`? 
That's when we start receiving messages anyways, so it's when Web Inspector
could be considered "usable".