[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
bugzilla-daemon at webkit.org
Fri Nov 22 17:31:46 PST 2019
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".
More information about the webkit-reviews
mailing list