679786 - telemetry reporting for use of depricated ISimpleDOM* interfaces

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

The part of this outside of msaa/ builds for me on linux, I'll try to make sure everything else builds and works tomorrow.   Perhaps we should  wrap these histogram macros and the inline functions with  #ifdef WIN32 but I'm nto sure.

whoever gets to this first

Comment 2

[Marco Zehe (:MarcoZ)]

Setting the operating system to something Windows since iSimpleDom is only available in Windows environments.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 553822 [details] [diff] [review]
patch

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

do telemetry on query interface rather than on method call

::: accessible/src/base/Statistics.h
@@ +52,5 @@
> +  /**
> +   * Report that ISimpleDOMNode has been used.
> +   */
> +  inline void ISimpleDomNodeUsed()
> +  { Telemetry::Accumulate(Telemetry::ISIMPLE_DOM_NODE_USAGE, 1); }

nit: two spaces indentation for body please, below too

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +47,5 @@
>  /**
>   * a11y telemetry
>   */
>  HISTOGRAM(A11Y_INSTANTIATED, 0, 1, 2, BOOLEAN, "has accessibility support been instantiated")
> +HISTOGRAM(ISIMPLE_DOM_DOC_USAGE, 0, 1, 2, BOOLEAN, "has the depricated ISimpleDOMDocument accessibility interface been used")

strictly speaking it's not deprecated, we don't advise to use it but do not deprecate.

Comment 4

[alexander :surkov (:asurkov)]

btw, I don't think we need to have historgrams for every interface, all we need to know whether these set of interfaces are in use or not.

Comment 5

[Trevor Saunders (:tbsaunde)]

> do telemetry on query interface rather than on method call

Want to convince me that's more useful? I'm not sure yet which I think would be better.

> ::: toolkit/components/telemetry/TelemetryHistograms.h
> @@ +47,5 @@
> >  /**
> >   * a11y telemetry
> >   */
> >  HISTOGRAM(A11Y_INSTANTIATED, 0, 1, 2, BOOLEAN, "has accessibility support been instantiated")
> > +HISTOGRAM(ISIMPLE_DOM_DOC_USAGE, 0, 1, 2, BOOLEAN, "has the depricated ISimpleDOMDocument accessibility interface been used")
> 
> strictly speaking it's not deprecated, we don't advise to use it but do not
> deprecate.

I assume you refer to all three here right?

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #4)
> btw, I don't think we need to have historgrams for every interface, all we
> need to know whether these set of interfaces are in use or not.

well, sure, but we get strictly more information this way, and it seems like the cost should be pretty low.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > do telemetry on query interface rather than on method call
> 
> Want to convince me that's more useful? I'm not sure yet which I think would
> be better.

this is more practical, if interface was queried then it's intended to be used.

> > > +HISTOGRAM(ISIMPLE_DOM_DOC_USAGE, 0, 1, 2, BOOLEAN, "has the depricated ISimpleDOMDocument accessibility interface been used")
> > 
> > strictly speaking it's not deprecated, we don't advise to use it but do not
> > deprecate.
> 
> I assume you refer to all three here right?

sure

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > btw, I don't think we need to have historgrams for every interface, all we
> > need to know whether these set of interfaces are in use or not.
> 
> well, sure, but we get strictly more information this way, and it seems like
> the cost should be pretty low.

the problem I referred here is not cost but useless information. I don't have any idea why would we need to know that this particular app use ISimpleDOMNode but doesn't use ISimpleDOMDocument. All I want to know is whether it uses these interfaces or not. So that for example, we can contact them and ask specific questions or ask to stop it's usage if we deprecate these interfaces at some point.

Comment 8

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

(In reply to alexander surkov from comment #7)
 
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > > btw, I don't think we need to have historgrams for every interface, all we
> > > need to know whether these set of interfaces are in use or not.
> > 
> > well, sure, but we get strictly more information this way, and it seems like
> > the cost should be pretty low.
> 
> the problem I referred here is not cost but useless information. I don't
> have any idea why would we need to know that this particular app use
> ISimpleDOMNode but doesn't use ISimpleDOMDocument. All I want to know is
> whether it uses these interfaces or not. So that for example, we can contact
> them and ask specific questions or ask to stop it's usage if we deprecate
> these interfaces at some point.

Are you saying you would like these method specific data?

e.g.
nsAccessNodeWrap::get_nodeInfo
nsAccessNodeWrap::get_attributes
...

Or that you would just like to know if ISimpleDOM* is used at all or not (1 boolean, yes/no)?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #8)

> Or that you would just like to know if ISimpleDOM* is used at all or not (1
> boolean, yes/no)?

yes

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: patch v2

Alex I think this is the approach you wnat right?

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 562476 [details] [diff] [review]
patch v2

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

I'm not sure why you add using namespace mozilla::a11y; but it doesn't really hurt anyone.
Could you use if else construction for QueryInterface implementation? It really doesn't make sense to check other IIDs if one was succeeded.

::: accessible/src/base/Statistics.h
@@ +52,5 @@
> +  /**
> +   * Report that ISimpleDOM* has been used.
> +   */
> +  inline void ISimpleDomUsed()
> +  { Telemetry::Accumulate(Telemetry::ISIMPLE_DOM_USAGE, 1); }

nit: two spaces indentation for function body

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: patch v3

Comment 13

[Trevor Saunders (:tbsaunde)]

inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/7d8bf0bce55c

Comment 14

[(no longer active)]

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