729154 - Telemetry for a11y instantiation by unknown cause.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 1

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 1.01

Comment 3

[Trevor Saunders (:tbsaunde)]

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]

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] (NeedInfo me for attention)]

(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] (NeedInfo me for attention)]

Comment on attachment 599220 [details] [diff] [review]
patch 1.01

Alexander, thumbs up? (I removed the extra braces locally)

Comment 7

[alexander :surkov (:asurkov)]

(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 (:asurkov)]

Comment on attachment 599220 [details] [diff] [review]
patch 1.01

canceling review until comment #7

Comment 9

[Trevor Saunders (:tbsaunde)]

(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] (NeedInfo me for attention)]

Let's rework after or as part of bug 733510.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

(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)]

(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 (:asurkov)]

just note: not sMode but sATs perhaps

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

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] (NeedInfo me for attention)]

BTW you guys are sure you like the direction we're going with the sample code in comment 12 right?

Comment 16

[alexander :surkov (:asurkov)]

(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)]

(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] (NeedInfo me for attention)]

(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 (:asurkov)]

(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] (NeedInfo me for attention)]

(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 (:asurkov)]

(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] (NeedInfo me for attention)]

Attachment: 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] (NeedInfo me for attention)]

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] (NeedInfo me for attention)]

Attachment: 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)]

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 (:asurkov)]

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)]

(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 (:asurkov)]

(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] (NeedInfo me for attention)]

(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] (NeedInfo me for attention)]

Attachment: patch 3

Patch to land. I'll update the uiautomation telemetry patch and land them on the same push.

Comment 31

[David Bolter [:davidb] (NeedInfo me for attention)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a74a4a363fc

Comment 32

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/7a74a4a363fc

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

Looping back... seeing a *lot* of UNKNOWN cause...  sigh.

Comment 34

[Jim Mathies [:jimm]]

(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]]

Actually, on these devices the desktop browser activates accessibility as well. So it's not just metrofx.

Comment 36

[Trevor Saunders (:tbsaunde)]

(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)]

(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] (NeedInfo me for attention)]

(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]]

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]]

Blah, ignore that jumble of early a11y telemetry comments.