1160900 - Display circular buffer status per recording if supported

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Must handle when buffer status is not available on platform.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1160900-l10n.patch

L10N strings for buffer status. Needs to land in 40.

Strings are for displaying the text (on top of or next to?) the progress bar (Buffer capacity: 40%), the tooltip for the progress bar, and the message to display once the buffer has reached 100%.

Comment 2

[Victor Porof [:vporof][:vp]]

Comment on attachment 8603077 [details] [diff] [review]
1160900-l10n.patch

Review of attachment 8603077 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds about right to me.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1160900-displaybuffer.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b06b721b9c2

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Landing l10n patch https://hg.mozilla.org/integration/fx-team/rev/c615b1a62222

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 8603625 [details] [diff] [review]
1160900-displaybuffer.patch

Review of attachment 8603625 [details] [diff] [review]:
-----------------------------------------------------------------

r? again after comments addressed.

::: browser/devtools/performance/modules/actors.js
@@ +138,5 @@
>  
>    /**
> +   * Wrapper around `profiler.isActive()` to take buffer status data and emit.
> +   */
> +  isActive: Task.async(function *() {

Nit: Might be a good idea now to rename this to getStatus or something more accurate.

@@ +141,5 @@
> +   */
> +  isActive: Task.async(function *() {
> +    let data = yield (actorCompatibilityBridge("isActive").call(this));
> +    // if no data, actor's probably destroyed and this was a lingering
> +    // poll for buffer status.

Ugh. When can this happen?

Nit: capitalize first letter of a sentence.

@@ +147,5 @@
> +      return;
> +    }
> +    let { position, generation, totalSize } = data;
> +
> +    this.emit("buffer-status", { position, generation, totalSize });

Why not just emit `data`? Are we worried about emitting actor ids or mutation?

::: browser/devtools/performance/performance-view.js
@@ +149,5 @@
> +
> +    let $container = $("#details-pane-container");
> +    let $bufferLabel = $(".buffer-status-message", $container.selectedPanel);
> +
> +    if (percent === 100) {

Is the percentage always clamped to 100? Are we prone to floating point errors here? Is it always an integer? Possibly not.

`percent >= 99.9` is probably safer and does the same thing most of the time.

::: browser/devtools/performance/test/browser_perf-compatibility-06.js
@@ +15,2 @@
>    // all the buffer info is retrieved, and delete the buffer properties.
> +  let client = front._connection._profiler._target.client;

Remind me why whe have this stuff again?

::: browser/devtools/performance/test/browser_perf-compatibility-07.js
@@ +6,5 @@
> + * status on servers that do not support buffer statuses.
> + */
> +function spawnTest () {
> +  // Keep it large, but still get to 1% relatively quick
> +  Services.prefs.setIntPref(PROFILER_BUFFER_SIZE_PREF, 1000000);

I imagine we might have many oranges because of this. Not sure if we have a better way of testing. How about mocking it?

@@ +12,5 @@
> +  let { gFront: front, EVENTS, $, PerformanceController, PerformanceView, RecordingsView } = panel.panelWin;
> +
> +  // Explicitly override the profiler's `isActive` on the protocol level, where
> +  // all the buffer info is retrieved, and delete the buffer properties.
> +  let client = front._connection._profiler._target.client;

Isn't this done in head.js as well? I'm not sure I understand why this is useful.

::: browser/themes/shared/devtools/performance.inc.css
@@ +85,5 @@
> +
> +#details-pane-container .buffer-status-message {
> +  display: none;
> +}
> +#details-pane-container.has-buffer-status .buffer-status-message {

Instead of .has-buffer-status, using an attribute would be better. This way you can only have three cohesive states, instead of 2 extra classes.

#details-pane-container:not([buffer-status]) or something similar.
#details-pane-container[buffer-status="in-progress"]
#details-pane-container[buffer-status="full"]

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8603625 [details] [diff] [review]
1160900-displaybuffer.patch

Review of attachment 8603625 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/actors.js
@@ +141,5 @@
> +   */
> +  isActive: Task.async(function *() {
> +    let data = yield (actorCompatibilityBridge("isActive").call(this));
> +    // if no data, actor's probably destroyed and this was a lingering
> +    // poll for buffer status.

When a polling for status gets sent out during destruction, the profiler will wait for the return of that to clear itself up. But at some point, the target.client object becomes null, which throws in actorCompatibilityBridge (well, it now checks for client's existence, but returns null), so pre-emptively destructuring the result can fail.

@@ +147,5 @@
> +      return;
> +    }
> +    let { position, generation, totalSize } = data;
> +
> +    this.emit("buffer-status", { position, generation, totalSize });

Not necessarily, just want to control what data is being sent out. Could change all the buffer-status to be a general "profiler status", emitting active/current time status as well, but don't feel strongly either way

::: browser/devtools/performance/test/browser_perf-compatibility-06.js
@@ +15,2 @@
>    // all the buffer info is retrieved, and delete the buffer properties.
> +  let client = front._connection._profiler._target.client;

Because the profiler does NOT use the ActorClass newer actors have, and since now isActive emits buffer data implicitly, we could just rewrite it all here, but this way, we stub it out on the RDP level so we're actually testing the isActive compatibility layer. This sucks though, been thinking of ways to add this implicitly to the Fronts.

::: browser/devtools/performance/test/browser_perf-compatibility-07.js
@@ +6,5 @@
> + * status on servers that do not support buffer statuses.
> + */
> +function spawnTest () {
> +  // Keep it large, but still get to 1% relatively quick
> +  Services.prefs.setIntPref(PROFILER_BUFFER_SIZE_PREF, 1000000);

Actually, this should be removed in this test -- We don't need the buffer to get to 1%, as we just listen for a "buffer-status" event, which will be empty (as this is testing older geckos)

@@ +12,5 @@
> +  let { gFront: front, EVENTS, $, PerformanceController, PerformanceView, RecordingsView } = panel.panelWin;
> +
> +  // Explicitly override the profiler's `isActive` on the protocol level, where
> +  // all the buffer info is retrieved, and delete the buffer properties.
> +  let client = front._connection._profiler._target.client;

We want `isActive` in the ProfilerActorFacade or whatever to be tested. This best simulates not getting buffer info from the server on older geckos.

::: browser/devtools/performance/test/browser_perf-recording-notices-03.js
@@ +8,5 @@
> + */
> +function spawnTest () {
> +  loadFrameScripts();
> +  // Keep it large, but still get to 1% relatively quick
> +  Services.prefs.setIntPref(PROFILER_BUFFER_SIZE_PREF, 1000000);

However, here we could mock it, by hooking into the target.client again, and emit whatever we want.

::: browser/themes/shared/devtools/performance.inc.css
@@ +85,5 @@
> +
> +#details-pane-container .buffer-status-message {
> +  display: none;
> +}
> +#details-pane-container.has-buffer-status .buffer-status-message {

sgtm

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c615b1a62222

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1160900-displaybuffer.patch

Changed the buffer-status event to profiler-status, and it is always emitted now if alive, rather than not if buffer info does not exist.

Added a new target property for mocking, and filters out props returned from the profiler on isActive, removing the ugly target.client.request stubbing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d8cce39948

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

 https://hg.mozilla.org/integration/fx-team/rev/1fcc6c6ddf0f

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1fcc6c6ddf0f

Comment 11

[Francesco Lodolo [:flod]]

# LOCALIZATION NOTE (profiler.bufferStatus):
# This string is displayed illustrating how full the profiler's circular buffer is.
profiler.bufferStatus=Buffer capacity: %S%

Would be useful to explain in the localization note that the second % is needed, and the result will be something like "40%".

I hope I'm wrong, but I usually see the % escaped in strings (%%).

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Good call, Francesco, I'll add that comment, and looks like we do have another bug for updating l10n comments in general (bug 1050085) -- I'll add what you mentioned there.

Regarding escaping the "%", it does render correctly, and there's a `table.percent=%` entry in the same file that also is not escaped -- is there anything else that could go wrong with not escaping it?

Comment 13

[Francesco Lodolo [:flod]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #12)
> Regarding escaping the "%", it does render correctly, and there's a
> `table.percent=%` entry in the same file that also is not escaped -- is
> there anything else that could go wrong with not escaping it?

No, as long it works ;-)
http://hg.mozilla.org/mozilla-central/file/default/browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties#l51

In the c615b1a62222 changeset I saw only strings, not code, so I thought is a was just a pre-landing of strings (then realized that this bug has also a patch with code).