717506 - telemetry for xforms use

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

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

For testing, install the about:telemetry addon.

Comment 2

[Andrzej Skalski]

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]

Attachment: xforms example 1 used in debugging

Comment 4

[Andrzej Skalski]

Attachment: xforms example 2 used in debugging

Comment 5

[Andrzej Skalski]

Attachment: xforms example 3 used in debugging

Comment 6

[Andrzej Skalski]

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

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

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]

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]

Attachment: a patch that I was unable to test

Comment 11

[alexander :surkov (:asurkov)]

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

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

ok, fine with me

Comment 14

[Andrzej Skalski]

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]

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

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

Attachment: Third version of a patch, which is 1 line different than previous

Comment 18

[alexander :surkov (:asurkov)]

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

inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/4f795f4fa318

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4f795f4fa318