Last Comment Bug 679786 - telemetry reporting for use of depricated ISimpleDOM* interfaces
: telemetry reporting for use of depricated ISimpleDOM* interfaces
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All Windows 7
: -- normal (vote)
: mozilla10
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: telemetrya11y 721772
  Show dependency treegraph
 
Reported: 2011-08-17 10:42 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-01-27 08:49 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (18.22 KB, patch)
2011-08-17 10:47 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review-
Details | Diff | Splinter Review
patch v2 (5.12 KB, patch)
2011-09-26 10:42 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review
patch v3 (5.42 KB, patch)
2011-09-27 07:53 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2011-08-17 10:42:28 PDT

    
Comment 1 Trevor Saunders (:tbsaunde) 2011-08-17 10:47:30 PDT
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
Comment 2 Marco Zehe (:MarcoZ) on PTO until August 15 2011-08-17 22:21:46 PDT
Setting the operating system to something Windows since iSimpleDom is only available in Windows environments.
Comment 3 alexander :surkov 2011-08-26 03:11:24 PDT
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 2011-08-26 03:40:05 PDT
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) 2011-08-26 03:48:47 PDT
> 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) 2011-08-26 03:50:52 PDT
(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 2011-08-26 04:03:54 PDT
(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] 2011-09-08 10:57:53 PDT
(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 2011-09-08 21:37:15 PDT
(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) 2011-09-26 10:42:38 PDT
Created attachment 562476 [details] [diff] [review]
patch v2

Alex I think this is the approach you wnat right?
Comment 11 alexander :surkov 2011-09-26 20:10:18 PDT
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) 2011-09-27 07:53:49 PDT
Created attachment 562767 [details] [diff] [review]
patch v3
Comment 13 Trevor Saunders (:tbsaunde) 2011-09-29 18:18:30 PDT
inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/7d8bf0bce55c
Comment 14 :Ehsan Akhgari (away Aug 1-5) 2011-09-30 07:31:03 PDT
https://hg.mozilla.org/mozilla-central/rev/7d8bf0bce55c

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