Last Comment Bug 729154 - Telemetry for a11y instantiation by unknown cause.
: Telemetry for a11y instantiation by unknown cause.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla14
Assigned To: David Bolter [:davidb]
:
Mentors:
Depends on:
Blocks: telemetrya11y
  Show dependency treegraph
 
Reported: 2012-02-21 09:17 PST by David Bolter [:davidb]
Modified: 2012-07-29 12:28 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 (3.68 KB, patch)
2012-02-21 09:18 PST, David Bolter [:davidb]
no flags Details | Diff | Review
patch 1.01 (3.68 KB, patch)
2012-02-21 09:26 PST, David Bolter [:davidb]
andrzej.j.skalski: review+
Details | Diff | Review
patch 2 (5.12 KB, patch)
2012-04-11 08:40 PDT, David Bolter [:davidb]
no flags Details | Diff | Review
patch 2.1 (6.43 KB, patch)
2012-04-13 08:17 PDT, David Bolter [:davidb]
tbsaunde+mozbugs: review+
Details | Diff | Review
patch 3 (6.63 KB, patch)
2012-04-16 07:06 PDT, David Bolter [:davidb]
no flags Details | Diff | Review

Description David Bolter [:davidb] 2012-02-21 09:17:38 PST

    
Comment 1 David Bolter [:davidb] 2012-02-21 09:18:59 PST
Created attachment 599217 [details] [diff] [review]
patch 1
Comment 2 David Bolter [:davidb] 2012-02-21 09:26:39 PST
Created attachment 599220 [details] [diff] [review]
patch 1.01
Comment 3 Trevor Saunders (:tbsaunde) 2012-02-22 16:41:44 PST
Comment on attachment 599220 [details] [diff] [review]
patch 1.01

Askalski want to take this one?

I don't really like all these assignments to atDetected, but don't really have a better idea.

>+  if (!atdetected) {
>+    statistics::A11yConsumers(UNKNOWN);
>+  }

nit, braces
Comment 4 Andrzej Skalski 2012-02-23 06:45:58 PST
looks good. I presume that we there may be multiple A11yConsumers, right? Because otherwise we could get rid off atdetected with just if (){} else if() {} else if() {}... else { UNKNOWN } . I will run mochitests and do r+ when they're ok.
Comment 5 David Bolter [:davidb] 2012-02-23 07:07:04 PST
(In reply to Andrzej Skalski from comment #4)
> looks good. I presume that we there may be multiple A11yConsumers, right?
Right - we never know for certain.

D
Comment 6 David Bolter [:davidb] 2012-02-23 08:06:30 PST
Comment on attachment 599220 [details] [diff] [review]
patch 1.01

Alexander, thumbs up? (I removed the extra braces locally)
Comment 7 alexander :surkov 2012-02-23 19:55:48 PST
(In reply to David Bolter [:davidb] from comment #6)
> Comment on attachment 599220 [details] [diff] [review]
> patch 1.01
> 
> Alexander, thumbs up? (I removed the extra braces locally)

The farther into the forest, the thicker the trees. I'd say rename sModes into sATs and get rid telemetry enum. It doesn't contradict to compatibility modes interface since modes are exposed by static methods (sMode is private static member).
Comment 8 alexander :surkov 2012-02-24 19:23:52 PST
Comment on attachment 599220 [details] [diff] [review]
patch 1.01

canceling review until comment #7
Comment 9 Trevor Saunders (:tbsaunde) 2012-02-24 19:34:41 PST
(In reply to alexander :surkov from comment #7)
> (In reply to David Bolter [:davidb] from comment #6)
> > Comment on attachment 599220 [details] [diff] [review]
> > patch 1.01
> > 
> > Alexander, thumbs up? (I removed the extra braces locally)
> 
> The farther into the forest, the thicker the trees. I'd say rename sModes
> into sATs and get rid telemetry enum. It doesn't contradict to compatibility
> modes interface since modes are exposed by static methods (sMode is private
> static member).

yeah,  sounds nice
Comment 10 David Bolter [:davidb] 2012-03-23 08:27:43 PDT
Let's rework after or as part of bug 733510.
Comment 11 David Bolter [:davidb] 2012-04-05 08:56:24 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> (In reply to alexander :surkov from comment #7)
> > (In reply to David Bolter [:davidb] from comment #6)
> > > Comment on attachment 599220 [details] [diff] [review]
> > > patch 1.01
> > > 
> > > Alexander, thumbs up? (I removed the extra braces locally)
> > 
> > The farther into the forest, the thicker the trees. I'd say rename sModes
> > into sATs and get rid telemetry enum. It doesn't contradict to compatibility
> > modes interface since modes are exposed by static methods (sMode is private
> > static member).
> 
> yeah,  sounds nice

I'm the odd one out here but I probably don't understand the details of what is been requested. What kind of data structure do we want to keep? Put another way, what would be the new of implementation of for example:
1. IsJAWS()
2. statistics::A11yConsumers(COBRA)
?

Also note we kept telem separate as per Nathan (bug 678965).

Moving back to one bitflag would take us back to the problems in that bug AFAICT.
Comment 12 Trevor Saunders (:tbsaunde) 2012-04-05 16:29:10 PDT
(In reply to David Bolter [:davidb] from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > (In reply to alexander :surkov from comment #7)
> > > (In reply to David Bolter [:davidb] from comment #6)
> > > > Comment on attachment 599220 [details] [diff] [review]
> > > > patch 1.01
> > > > 
> > > > Alexander, thumbs up? (I removed the extra braces locally)
> > > 
> > > The farther into the forest, the thicker the trees. I'd say rename sModes
> > > into sATs and get rid telemetry enum. It doesn't contradict to compatibility
> > > modes interface since modes are exposed by static methods (sMode is private
> > > static member).
> > 
> > yeah,  sounds nice
> 
> I'm the odd one out here but I probably don't understand the details of what
> is been requested. What kind of data structure do we want to keep? Put
> another way, what would be the new of implementation of for example:
> 1. IsJAWS()
> 2. statistics::A11yConsumers(COBRA)
> ?
> 
> Also note we kept telem separate as per Nathan (bug 678965).
> 
> Moving back to one bitflag would take us back to the problems in that bug
> AFAICT.

I think not if we're smart about how we do it.

so, Init() would have things like

if (GetModuleHandeW("we.dll")
  sMode |= 1 << eWE;

....

uint32_t temp = sMode;
for (int i = 0; temp; i++) {
  if (temp & 0x1)
    statistics::Consumer(i);

  temp >>= 1;
}

or a little more verbosely

if (GetModuleHandleW("we.dll") {
  statistics::Consumer(WE);
  sMode |= 1 << WE;
}
....

then IsWe() is
return !!(sMode & (1 << WE));
Comment 13 alexander :surkov 2012-04-05 20:45:45 PDT
just note: not sMode but sATs perhaps
Comment 14 David Bolter [:davidb] 2012-04-06 07:17:23 PDT
re comment 12, OK got ya.

re comment 13, I don't think there is a great name actually since this holds info about ATs, modes, and non ATs...
Comment 15 David Bolter [:davidb] 2012-04-06 07:26:05 PDT
BTW you guys are sure you like the direction we're going with the sample code in comment 12 right?
Comment 16 alexander :surkov 2012-04-06 07:29:28 PDT
(In reply to David Bolter [:davidb] from comment #14)
> re comment 12, OK got ya.
> 
> re comment 13, I don't think there is a great name actually since this holds
> info about ATs, modes, and non ATs...

It doesn't contain any info about modes. non ATs - ok then maybe sConsumers.
Comment 17 Trevor Saunders (:tbsaunde) 2012-04-06 07:34:16 PDT
(In reply to David Bolter [:davidb] from comment #15)
> BTW you guys are sure you like the direction we're going with the sample
> code in comment 12 right?

I think so, you dislike it for some reason?
Comment 18 David Bolter [:davidb] 2012-04-06 07:42:20 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> (In reply to David Bolter [:davidb] from comment #15)
> > BTW you guys are sure you like the direction we're going with the sample
> > code in comment 12 right?
> 
> I think so, you dislike it for some reason?

I've grown to like it in the last 10 minutes or so. I was worried the code was becoming less straightforward but is still simple enough actually. I'll probably whip this up Monday or so.
Comment 19 alexander :surkov 2012-04-06 20:13:01 PDT
(In reply to David Bolter [:davidb] from comment #18)
> I've grown to like it in the last 10 minutes or so. I was worried the code
> was becoming less straightforward

the code was becoming less straightforward with your initial approach :)
Comment 20 David Bolter [:davidb] 2012-04-09 06:32:07 PDT
(In reply to alexander :surkov from comment #19)
> (In reply to David Bolter [:davidb] from comment #18)
> > I've grown to like it in the last 10 minutes or so. I was worried the code
> > was becoming less straightforward
> 
> the code was becoming less straightforward with your initial approach :)

"Straightforward" is in the eye of the beholder I guess :)
Comment 21 alexander :surkov 2012-04-09 06:36:28 PDT
(In reply to David Bolter [:davidb] from comment #20)
> > the code was becoming less straightforward with your initial approach :)
> 
> "Straightforward" is in the eye of the beholder I guess :)

yep, that's me ;)
Comment 22 David Bolter [:davidb] 2012-04-11 08:40:14 PDT
Created attachment 614014 [details] [diff] [review]
patch 2

I haven't carefully self-reviewed yet, but am posting now so I can get to Eitan's patch.
Comment 23 David Bolter [:davidb] 2012-04-11 18:07:59 PDT
Comment on attachment 614014 [details] [diff] [review]
patch 2

Note to self: I've got to tweak some things on this patch: (change the telemetry max, and special case NVDA since it is 0). Also there is another patch floating somewhere for uiaautomation... want to change that value to 11)

Trev, I'm canceling review.
Comment 24 David Bolter [:davidb] 2012-04-13 08:17:53 PDT
Created attachment 614790 [details] [diff] [review]
patch 2.1

I debugged/tested the following cases and they work as expected:
IsJaws()
UNKNOWN (accprobe)
NVDA
JAWS
NVDA + JAWS in combination

I confirmed via about:telemetry as well.
Comment 25 Trevor Saunders (:tbsaunde) 2012-04-13 15:58:06 PDT
Comment on attachment 614790 [details] [diff] [review]
patch 2.1

>+  // If we have a known consumer remove the unknown bit.
>+  if (sConsumers != (1 << Compatibility::UNKNOWN))
>+    sConsumers ^= (1 << Compatibility::UNKNOWN);

I think we usually use x &= ~y for this, however in this case what you have is actually safer against the case something is wrong and sConsumers is 0.

>+
>+  // Gather telemetry
>+  PRUint32 temp = sConsumers;
>+  for (int i = 0; temp; i++) {
>+    if (temp == 0 || temp & 0x1)
>+      statistics::A11yConsumers(i);

I'm pretty sure the first part of that test can never be true otherwise we wouldn't be in the loop, I also don't understand what its for.
Comment 26 alexander :surkov 2012-04-13 20:21:51 PDT
Comment on attachment 614790 [details] [diff] [review]
patch 2.1

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

::: accessible/src/msaa/Compatibility.h
@@ +101,5 @@
>      COBRA = 6,
>      ZOOMTEXT = 7,
>      KAZAGURU = 8,
> +    YOUDAO = 9,
> +    UNKNOWN = 10

why don't you keep these as 1 << 0 and prefer to do that through whole code?
Comment 27 Trevor Saunders (:tbsaunde) 2012-04-13 20:37:26 PDT
(In reply to alexander :surkov from comment #26)
> Comment on attachment 614790 [details] [diff] [review]
> patch 2.1
> 
> Review of attachment 614790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/msaa/Compatibility.h
> @@ +101,5 @@
> >      COBRA = 6,
> >      ZOOMTEXT = 7,
> >      KAZAGURU = 8,
> > +    YOUDAO = 9,
> > +    UNKNOWN = 10
> 
> why don't you keep these as 1 << 0 and prefer to do that through whole code?

probably because i suggested it when I wasn't thinking about it very much.  It'll make associating telemetry data with values a little weirder, but make the code nicer I think.
Comment 28 alexander :surkov 2012-04-13 21:21:18 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #27)

> > why don't you keep these as 1 << 0 and prefer to do that through whole code?
> 
> probably because i suggested it when I wasn't thinking about it very much. 
> It'll make associating telemetry data with values a little weirder, but make
> the code nicer I think.

yes. David, can you do a change please?
Comment 29 David Bolter [:davidb] 2012-04-16 06:39:27 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> Comment on attachment 614790 [details] [diff] [review]
> >+    if (temp == 0 || temp & 0x1)
> >+      statistics::A11yConsumers(i);
> 
> I'm pretty sure the first part of that test can never be true otherwise we
> wouldn't be in the loop, I also don't understand what its for.

Good catch - cruft.

(In reply to alexander :surkov from comment #28)
> (In reply to Trevor Saunders (:tbsaunde) from comment #27)
> 
> > > why don't you keep these as 1 << 0 and prefer to do that through whole code?
> > 
> > probably because i suggested it

> 
> yes. David, can you do a change please?

Heh. Yep.
Comment 30 David Bolter [:davidb] 2012-04-16 07:06:46 PDT
Created attachment 615321 [details] [diff] [review]
patch 3

Patch to land. I'll update the uiautomation telemetry patch and land them on the same push.
Comment 32 Marco Bonardo [::mak] 2012-04-17 07:12:19 PDT
https://hg.mozilla.org/mozilla-central/rev/7a74a4a363fc
Comment 33 David Bolter [:davidb] 2012-06-27 11:13:18 PDT
Looping back... seeing a *lot* of UNKNOWN cause...  sigh.
Comment 34 Jim Mathies [:jimm] 2012-07-28 10:32:30 PDT
(In reply to David Bolter [:davidb] Away July30-Aug3 from comment #33)
> Looping back... seeing a *lot* of UNKNOWN cause...  sigh.

Should we add an additional entry here for metrofx running on touch screen devices?
Comment 35 Jim Mathies [:jimm] 2012-07-28 10:34:24 PDT
Actually, on these devices the desktop browser activates accessibility as well. So it's not just metrofx.
Comment 36 Trevor Saunders (:tbsaunde) 2012-07-28 18:25:57 PDT
(In reply to Jim Mathies [:jimm] from comment #34)
> (In reply to David Bolter [:davidb] Away July30-Aug3 from comment #33)
> > Looping back... seeing a *lot* of UNKNOWN cause...  sigh.
> 
> Should we add an additional entry here for metrofx running on touch screen
> devices?

we look for uiautomationcore.dll or similar already, but if that doesn't cover this case sure.
Comment 37 Trevor Saunders (:tbsaunde) 2012-07-28 18:26:47 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #36)
> (In reply to Jim Mathies [:jimm] from comment #34)
> > (In reply to David Bolter [:davidb] Away July30-Aug3 from comment #33)
> > > Looping back... seeing a *lot* of UNKNOWN cause...  sigh.
> > 
> > Should we add an additional entry here for metrofx running on touch screen
> > devices?
> 
> we look for uiautomationcore.dll or similar already, but if that doesn't
> cover this case sure.

(see accessible/src/msaa/Compatibility.cpp for exactly what all we look for.)
Comment 38 David Bolter [:davidb] 2012-07-29 10:56:45 PDT
(In reply to Jim Mathies [:jimm] from comment #35)
> Actually, on these devices the desktop browser activates accessibility as
> well. So it's not just metrofx.

Yes I would like to know what proportion of our instantiation is due to these devices.
Comment 39 Jim Mathies [:jimm] 2012-07-29 12:27:22 PDT
The telemetry A11Y_CONSUMERS doesn't appear to be working, or maybe the data has to be pulled out manually?(In reply to David Bolter [:davidb] Away July30-Aug3 from comment #38)
> (In reply to Jim Mathies [:jimm] from comment #35)
> > Actually, on these devices the desktop browser activates accessibility as
> > well. So it's not just metrofx.
> 
> Yes I would like to know what proportion of our instantiation is due to
> these devices.

So, not sure if I'm reading the telemetry data correctly but it looks like the highest number of instantiations comes from uiautomation @ 97%, assuming that '10' column represents UIAUTOMATION.(In reply to Trevor Saunders (:tbsaunde) from comment #36)
> (In reply to Jim Mathies [:jimm] from comment #34)
> > (In reply to David Bolter [:davidb] Away July30-Aug3 from comment #33)
> > > Looping back... seeing a *lot* of UNKNOWN cause...  sigh.
> > 
> > Should we add an additional entry here for metrofx running on touch screen
> > devices?
> 
> we look for uiautomationcore.dll or similar already, but if that doesn't
> cover this case sure.

http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/Compatibility.cpp#87

This test fails on win8, the library name needs 'core' on the end of it to get picked up right. I filed bug 778569.
Comment 40 Jim Mathies [:jimm] 2012-07-29 12:28:30 PDT
Blah, ignore that jumble of early a11y telemetry comments.

Note You need to log in before you can comment on or make changes to this bug.