telemetry reporting for use of depricated ISimpleDOM* interfaces

RESOLVED FIXED in mozilla10

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla10
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 553822 [details] [diff] [review]
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
Attachment #553822 - Flags: review?(surkov.alexander)
Attachment #553822 - Flags: review?(marco.zehe)
Attachment #553822 - Flags: review?(bolterbugz)

Comment 2

6 years ago
Setting the operating system to something Windows since iSimpleDom is only available in Windows environments.
OS: Linux → Windows 7
Hardware: x86_64 → All

Comment 3

6 years ago
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.
Attachment #553822 - Flags: review?(surkov.alexander)
Attachment #553822 - Flags: review?(marco.zehe)
Attachment #553822 - Flags: review?(bolterbugz)
Attachment #553822 - Flags: review-

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
> 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?
(Assignee)

Comment 6

6 years ago
(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

6 years ago
(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.
(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

6 years ago
(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
(Assignee)

Comment 10

6 years ago
Created attachment 562476 [details] [diff] [review]
patch v2

Alex I think this is the approach you wnat right?
Attachment #553822 - Attachment is obsolete: true
Attachment #562476 - Flags: review?(surkov.alexander)

Comment 11

6 years ago
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
Attachment #562476 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 12

6 years ago
Created attachment 562767 [details] [diff] [review]
patch v3
(Assignee)

Comment 13

6 years ago
inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/7d8bf0bce55c
https://hg.mozilla.org/mozilla-central/rev/7d8bf0bce55c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Blocks: 721772
You need to log in before you can comment on or make changes to this bug.