telemetry for xforms use

RESOLVED FIXED in mozilla13

Status

()

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

People

(Reporter: tbsaunde, Assigned: Andrzej Skalski)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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?
For testing, install the about:telemetry addon.
(Assignee)

Updated

5 years ago
Assignee: nobody → askalski
(Assignee)

Comment 2

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

Comment 3

5 years ago
Created attachment 591880 [details]
xforms example 1 used in debugging
(Assignee)

Comment 4

5 years ago
Created attachment 591881 [details]
xforms example 2 used in debugging
(Assignee)

Comment 5

5 years ago
Created attachment 591882 [details]
xforms example 3 used in debugging
(Assignee)

Comment 6

5 years ago
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

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

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

Comment 10

5 years ago
Created attachment 593216 [details] [diff] [review]
a patch that I was unable to test
Attachment #593216 - Flags: review?(surkov.alexander)

Updated

5 years ago
Attachment #593216 - Attachment is patch: true

Comment 11

5 years ago
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?
Attachment #593216 - Flags: review?(trev.saunders)
Attachment #593216 - Flags: review?(surkov.alexander)
Attachment #593216 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #593216 - Flags: review?(trev.saunders) → review+
(Reporter)

Comment 12

5 years ago
> 
> @@ +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

5 years ago
ok, fine with me
(Assignee)

Comment 14

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

Comment 15

5 years ago
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.
Attachment #593216 - Attachment is obsolete: true
Attachment #594719 - Flags: review?(trev.saunders)

Comment 16

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

Comment 17

5 years ago
Created attachment 595044 [details] [diff] [review]
Third version of a patch, which is 1 line different than previous
Attachment #594719 - Attachment is obsolete: true
Attachment #594719 - Flags: review?(trev.saunders)
Attachment #595044 - Flags: review?(trev.saunders)
Attachment #595044 - Flags: review?(surkov.alexander)

Comment 18

5 years ago
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.
Attachment #595044 - Flags: review?(trev.saunders)
Attachment #595044 - Flags: review?(surkov.alexander)

Comment 19

5 years ago
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/4f795f4fa318
https://hg.mozilla.org/mozilla-central/rev/4f795f4fa318
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.