Last Comment Bug 717506 - telemetry for xforms use
: telemetry for xforms use
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Andrzej Skalski
:
Mentors:
Depends on:
Blocks: telemetrya11y
  Show dependency treegraph
 
Reported: 2012-01-11 19:49 PST by Trevor Saunders (:tbsaunde)
Modified: 2012-02-08 08:59 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
xforms example 1 used in debugging (1.73 KB, text/html)
2012-01-26 11:45 PST, Andrzej Skalski
no flags Details
xforms example 2 used in debugging (564 bytes, text/html)
2012-01-26 11:49 PST, Andrzej Skalski
no flags Details
xforms example 3 used in debugging (4.13 KB, text/html)
2012-01-26 11:49 PST, Andrzej Skalski
no flags Details
a patch that I was unable to test (2.12 KB, patch)
2012-01-31 14:22 PST, Andrzej Skalski
surkov.alexander: review+
tbsaunde+mozbugs: review+
Details | Diff | Review
A second version of proposed patch (2.09 KB, patch)
2012-02-06 08:39 PST, Andrzej Skalski
no flags Details | Diff | Review
Third version of a patch, which is 1 line different than previous (2.46 KB, patch)
2012-02-07 08:20 PST, Andrzej Skalski
no flags Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-01-11 19:49:55 PST
Would be nice to know if anyone uses this.  We haven't fixed any bugs in it since we moved away from cvs, only drive bys as something else was getting done.

irrc we talked about this being a good idea at some point, but didn't do anything with it.

The patch should pretty easy, a lot like bug 678799

Andrzej, want this one?
Comment 1 David Bolter [:davidb] 2012-01-20 10:09:29 PST
For testing, install the about:telemetry addon.
Comment 2 Andrzej Skalski 2012-01-26 11:42:46 PST
according to https://developer.mozilla.org/en/XForms , XForms are not actively supported:

This extension, while supporting a significant subset of the XForms 1.0 and 1.1 candidate recommendations, is not actively maintained any more since about 2010. The last official release has been done for Firefox 3.6 and is available for download on addons.mozilla.org

Tested with Nightly (Jan 26, 2012), after applying "forced compatibility", most of the web examples (including http://www.w3.org/TR/2002/WD-xforms-20020118/sliceE.html ) do not seem to display properly (although it's hard to tell, because I haven't found a browser that produces any sensible output with these examples - tried chromium).

In this compilation (Nightly + forced compatibility outdated plug-in) I placed breakpoint in nsXFormsAccessible.cpp, line 66 ( nsXFormsAccessibleBase::nsXFormsAccessibleBase() ) and run it has never been reached while browsing example files. It seems like the object has never been constructed.
Comment 3 Andrzej Skalski 2012-01-26 11:45:17 PST
Created attachment 591880 [details]
xforms example 1 used in debugging
Comment 4 Andrzej Skalski 2012-01-26 11:49:05 PST
Created attachment 591881 [details]
xforms example 2 used in debugging
Comment 5 Andrzej Skalski 2012-01-26 11:49:38 PST
Created attachment 591882 [details]
xforms example 3 used in debugging
Comment 6 Andrzej Skalski 2012-01-26 11:50:40 PST
btw, all examples has been copied-and-pasted from some top results for "xforms examples" in google, I don't really remember their sources.
Comment 7 alexander :surkov 2012-01-26 20:49:47 PST
(In reply to Andrzej Skalski from comment #2)

> In this compilation (Nightly + forced compatibility outdated plug-in)

I don't think it's going to ever work because they are binary incompatible
Comment 8 David Bolter [:davidb] 2012-01-31 06:12:33 PST
Doron, Aaron, can you see any reason for us to continue supporting xforms accessibility in future versions of Firefox? Do you have any plans to update the xforms addon?
Comment 9 aaronr 2012-01-31 10:02:41 PST
I don't see much momentum behind the Mozilla XForms extension anymore.  I'm fine with dropping support for xforms accessibility if it has no value to any other extensions.
Comment 10 Andrzej Skalski 2012-01-31 14:22:49 PST
Created attachment 593216 [details] [diff] [review]
a patch that I was unable to test
Comment 11 alexander :surkov 2012-01-31 16:46:53 PST
Comment on attachment 593216 [details] [diff] [review]
a patch that I was unable to test

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

r=me with comments addressed
let's get extra review from Trevor

::: accessible/src/base/Statistics.h
@@ +65,5 @@
>    inline void IAccessibleTableUsed()
>      { Telemetry::Accumulate(Telemetry::IACCESSIBLE_TABLE_USAGE, 1); }
>  
> +  /**
> +   * Report that XFormsAccessibleBase has been used.

that XForms accessibility has been instantiated?

@@ +68,5 @@
> +  /**
> +   * Report that XFormsAccessibleBase has been used.
> +   */
> +  inline void XFormsAccessibleUsed()
> +    { Telemetry::Accumulate(Telemetry::XFORMS_ACCESSIBLE_USED, true); }

shouldn't you accumulate it only once?
Accumulate takes int not boolean

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +59,4 @@
>  HISTOGRAM(A11Y_CONSUMERS, 1, 6, 7, LINEAR, "Accessibility client by enum id")
>  HISTOGRAM_BOOLEAN(ISIMPLE_DOM_USAGE, "have the ISimpleDOM* accessibility interfaces been used")
>  HISTOGRAM_BOOLEAN(IACCESSIBLE_TABLE_USAGE, "has the IAccessibleTable accessibility interface been used")
> +HISTOGRAM_BOOLEAN(XFORMS_ACCESSIBLE_USED, "has XForms accessible been used")

maybe has XForms accessibility been instantiated?
Comment 12 Trevor Saunders (:tbsaunde) 2012-01-31 21:48:53 PST
> 
> @@ +68,5 @@
> > +  /**
> > +   * Report that XFormsAccessibleBase has been used.
> > +   */
> > +  inline void XFormsAccessibleUsed()
> > +    { Telemetry::Accumulate(Telemetry::XFORMS_ACCESSIBLE_USED, true); }
> 
> shouldn't you accumulate it only once?

well, once per document would seem idea, so we now how many web sites are effected not just the number of users, but that seems a little complicated.  So I think knowing how many xforms accessibles we need is reasonable
Comment 13 alexander :surkov 2012-01-31 23:23:28 PST
ok, fine with me
Comment 14 Andrzej Skalski 2012-02-06 04:50:52 PST
Alexander: if Accumulate() takes int not boolean as you say, than we should fix line 51 as well:

    { Telemetry::Accumulate(Telemetry::A11Y_INSTANTIATED, true); }

That's where I took idea from. Should I change it?
Comment 15 Andrzej Skalski 2012-02-06 08:39:58 PST
Created attachment 594719 [details] [diff] [review]
A second version of proposed patch

The patch with applied changes. Again, only mochitests-a11y were run, as I cannot trigger the entire XForms mechanism to run. Mochitests were as clean build.
Comment 16 alexander :surkov 2012-02-07 02:17:15 PST
(In reply to Andrzej Skalski from comment #14)
> Alexander: if Accumulate() takes int not boolean as you say, than we should
> fix line 51 as well:
> 
>     { Telemetry::Accumulate(Telemetry::A11Y_INSTANTIATED, true); }
> 
> That's where I took idea from. Should I change it?

yes please
Comment 17 Andrzej Skalski 2012-02-07 08:20:20 PST
Created attachment 595044 [details] [diff] [review]
Third version of a patch, which is 1 line different than previous
Comment 18 alexander :surkov 2012-02-07 17:36:00 PST
Comment on attachment 595044 [details] [diff] [review]
Third version of a patch, which is 1 line different than previous

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

you don't need to rerequest review(s) when reviewers r+'d and you and reviewers agree on comments and the way they should be addressed by.
Comment 19 alexander :surkov 2012-02-07 23:26:24 PST
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/4f795f4fa318
Comment 20 Ed Morley [:emorley] 2012-02-08 08:59:24 PST
https://hg.mozilla.org/mozilla-central/rev/4f795f4fa318

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