1394927 - Gecko Profiler fails to display various markers in the timeline view

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Jim Mathies [:jimm]]

Another bug was filed on this over on github a couple weeks ago. As I understand things some fixes landed but the original bug was never addressed.

previous landings:
https://github.com/devtools-html/perf.html/pull/511
https://github.com/devtools-html/perf.html/pull/506

STR:
1) open this profile - https://perfht.ml/2x5Djj1 
2) view the Markers list - note there are numerous 'A11y Events' listed
3) View the Timeline view - none of the a11y markers show up under any thread

Comment 1

[Markus Stange [:mstange]]

What does the C++ code look like that you used to insert these markers? It looks like it's a "tracing" marker whose "interval" property is undefined.

Comment 2

[Julien Wajsberg [:julienw]]

In those PR, I used the provided profile as a baseline to test my code. The provided profile was https://perfht.ml/2wmBYnw and it works for this one.

I can look why this doesn't for the profile in comment 0.

Comment 3

[Julien Wajsberg [:julienw]]

In the profile where it works, the markers look like:
{"name":"A11y Event: text removed","time":17218.68700949332,"data":null}

In the profile where it doesn't work, the markers look like:
{"name":"text inserted","time":6443.369634582893,"data":{"type":"tracing","category":"A11y Event"}}

I believe these markers are invalid: tracing markers are used to define interval markers, and as Markus said, they need a "interval" property in their "data" payload, with a value of either "start" or "end".

Did you change something in Gecko to change how the markers were generated? You should backout this as it's liekly invalid.

We can define together what a good marker is for accessibility work: what data you need, etc. And then we can surface that data in the tool.
That's what the spidermonkey developers did for GC markers. That's what the DOM developers did for DOMEvent markers. So let's do it :)

Comment 4

[Julien Wajsberg [:julienw]]

The relevant code in perf.html is [1].

As you see, the markers with a type "tracing" get a very special handling.

[1] https://github.com/devtools-html/perf.html/blob/da7c9d9a8f6547ce1d05ab57f30d0f3df5e90931/src/profile-logic/profile-data.js#L797-L827

Comment 5

[Jim Mathies [:jimm]]

The tracing C++ is right here - 
http://searchfox.org/mozilla-central/source/accessible/generic/Accessible.cpp#852

Single line that adds a tracing marker. Apparently this displayed properly in the old interface hence the expectation that it would work. Should we swap that out for profiler_add_marker?

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

To be fair, I don't think the old interface ever supported profiler_tracing, just profiler_add_marker.

Comment 7

[Jim Mathies [:jimm]]

Attachment: patch

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf1d0e361ab
Use profiler_add_marker vs. profiler_tracing for accessibility profile events tags. r=aklotz

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2bf1d0e361ab

Comment 10

[Jim Mathies [:jimm]]

Appears to be working - https://perfht.ml/2epyDNn

Comment 11

[Julien Wajsberg [:julienw]]

Perfect !

My proposal above still applies: I think it could make sense to define a specific a11y marker. As a nice consequence, they could all show on one line in a profile, and you could possibly add more data. For example for "text inserted" we could have the actual text.