Last Comment Bug 741398 - make ARIA state map extensible
: make ARIA state map extensible
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: cleana11y 740851 762876
  Show dependency treegraph
 
Reported: 2012-04-02 08:25 PDT by alexander :surkov
Modified: 2012-06-12 06:25 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (32.38 KB, patch)
2012-04-02 08:25 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (34.64 KB, patch)
2012-04-04 04:47 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (34.84 KB, patch)
2012-04-05 01:47 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-04-02 08:25:15 PDT
Created attachment 611462 [details] [diff] [review]
patch
Comment 1 Trevor Saunders (:tbsaunde) 2012-04-03 02:37:50 PDT
Comment on attachment 611462 [details] [diff] [review]
patch

>diff --git a/accessible/src/base/ARIAStateMap.cpp b/accessible/src/base/ARIAStateMap.cpp
>new file mode 100644
>--- /dev/null
>+++ b/accessible/src/base/ARIAStateMap.cpp
>@@ -0,0 +1,356 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public

editor lines :)

>+  nsIAtom* mAttrName;

any reason not to make all these things const? afaict you never modify one of these after making it.

>+
>+  // States if the attribute value is matched to the enum value.
>+  const char* mValue1;

I wonder how using atoms instead would effect performance, I suspect they'd atleast make things nicer.

>+static void MapEnumType(dom::Element* aElement, PRUint64* aState,
>+                        const EnumTypeData& aData);

if it were me I'd probably keep the function prototypes together if I could.

>+  nsIAtom* mAttrName;

make these const too?

>+aria::MapToState(EStateRule aRule, dom::Element* aElement, PRUint64* aState)


I think I might like having the aria:: here for clarity, but I don't think you should need it.

I'm not a huge fan of this function returning true or false, afaik we check in two places one being UniversalStatesFor() which is sort of reasonable, but the if Map(x) && Map(y) { Map(z); } thing is really weird, how crazy would it be to assert we were not called with a crazy  rule, and check type is not eNone at the call site?

>+{
>+  switch (aRule) {
>+    case eARIAAutoComplete:
>+    {

I can't decide if I like this switch because its explicit or want to shorten it.

>+      static EnumTypeData data(
>+        nsGkAtoms::aria_autocomplete,
>+        "inline", states::SUPPORTS_AUTOCOMPLETION,
>+        "list", states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION,
>+        "both", states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION);

static const?

>+void

the prototype is static, for C style functions I think you should probably make the definition static as well although I'm not sure exactly what the spec says here.

>+      if (aData.mState1)
>+        *aState |= aData.mState1;

I think just *aState |= aData.mState1 would be clearer.

>+void

static here as well.

>+++ b/accessible/src/base/ARIAStateMap.h
>@@ -0,0 +1,60 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public

editor lines again.

>+#include <prtypes.h>

<prtypes.h> is weird any reason?

> #include "nsIAccessibleRole.h"

still needed?

> #include "nsIContent.h"

same

>+  if (aria::MapToState(mRoleMapEntry->attributeMap1, element, aState) &&
>+      aria::MapToState(mRoleMapEntry->attributeMap2, element, aState))
>+    aria::MapToState(mRoleMapEntry->attributeMap3, element, aState);
> }

that's better than it was but still funny.

> nsXULTextFieldAccessible::ApplyARIAState(PRUint64* aState)
> {
>   nsHyperTextAccessibleWrap::ApplyARIAState(aState);
> 
>-  nsStateMapEntry::MapToStates(mContent, aState, eARIAAutoComplete);
>+  aria::MapToState(aria::eARIAAutoComplete, mContent->AsElement(), aState);

are you sure that's valid? I'm fairly sure for html where we have real objects, but doesn't xul use a lot of funny xml / js stuffs?
Comment 2 alexander :surkov 2012-04-04 04:34:54 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #1)

> >+  nsIAtom* mAttrName;
> 
> any reason not to make all these things const? afaict you never modify one
> of these after making it.

which const, object, pointer or both? they are sort of internal objects without any wide usage so what is the best practice for those? i.e should we really care or the point is follow a good style?

> >+  // States if the attribute value is matched to the enum value.
> >+  const char* mValue1;
> 
> I wonder how using atoms instead would effect performance, I suspect they'd
> atleast make things nicer.

ok, done

> >+static void MapEnumType(dom::Element* aElement, PRUint64* aState,
> >+                        const EnumTypeData& aData);
> 
> if it were me I'd probably keep the function prototypes together if I could.

ok, if you like

> >+aria::MapToState(EStateRule aRule, dom::Element* aElement, PRUint64* aState)
> 
> 
> I think I might like having the aria:: here for clarity, but I don't think
> you should need it.

iirc I've got linker error.

> I'm not a huge fan of this function returning true or false, afaik we check
> in two places one being UniversalStatesFor() which is sort of reasonable,
> but the if Map(x) && Map(y) { Map(z); } thing is really weird, how crazy
> would it be to assert we were not called with a crazy  rule, and check type
> is not eNone at the call site?

that makes sense I'll file follow ups on this.

> >+{
> >+  switch (aRule) {
> >+    case eARIAAutoComplete:
> >+    {
> 
> I can't decide if I like this switch because its explicit or want to shorten
> it.

please say more, because switch was a point of the reorg.

> >+      static EnumTypeData data(
> >+        nsGkAtoms::aria_autocomplete,
> >+        "inline", states::SUPPORTS_AUTOCOMPLETION,
> >+        "list", states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION,
> >+        "both", states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION);
> 
> static const?

ah, keeping const the object, const its members. What's is the best practice again?

> >+void
> 
> the prototype is static, for C style functions I think you should probably
> make the definition static as well although I'm not sure exactly what the
> spec says here.

If you like I can add it but we never did it before.

> > nsXULTextFieldAccessible::ApplyARIAState(PRUint64* aState)
> > {
> >   nsHyperTextAccessibleWrap::ApplyARIAState(aState);
> > 
> >-  nsStateMapEntry::MapToStates(mContent, aState, eARIAAutoComplete);
> >+  aria::MapToState(aria::eARIAAutoComplete, mContent->AsElement(), aState);
> 
> are you sure that's valid? I'm fairly sure for html where we have real
> objects, but doesn't xul use a lot of funny xml / js stuffs?

I'm not sure I follow. This allows to use aria-autocomplete of XUL textboxes. What's wrong?
Comment 3 alexander :surkov 2012-04-04 04:47:06 PDT
Created attachment 612149 [details] [diff] [review]
patch2
Comment 4 Trevor Saunders (:tbsaunde) 2012-04-04 17:05:49 PDT
> > any reason not to make all these things const? afaict you never modify one
> > of these after making it.
> 
> which const, object, pointer or both? they are sort of internal objects
> without any wide usage so what is the best practice for those? i.e should we
> really care or the point is follow a good style?

I've never heard of a best practice.  Personally I tend to prefer to tell the compiler as much as I can unless I'm trying to play a trick.

> > if it were me I'd probably keep the function prototypes together if I could.
> 
> ok, if you like

just sort of usual thing I see people do when they write things like this.

> > I think I might like having the aria:: here for clarity, but I don't think
> > you should need it.
> 
> iirc I've got linker error.

ok, interesting

> > I'm not a huge fan of this function returning true or false, afaik we check
> > in two places one being UniversalStatesFor() which is sort of reasonable,
> > but the if Map(x) && Map(y) { Map(z); } thing is really weird, how crazy
> > would it be to assert we were not called with a crazy  rule, and check type
> > is not eNone at the call site?
> 
> that makes sense I'll file follow ups on this.

sounds good.

> 
> > >+{
> > >+  switch (aRule) {
> > >+    case eARIAAutoComplete:
> > >+    {
> > 
> > I can't decide if I like this switch because its explicit or want to shorten
> > it.
> 
> please say more, because switch was a point of the reorg.

well, its great tin that I think its a bit clearer how things work, and for flexability.  My only sort of issue was that its pretty long, and maybe feels a little  repeditive.

> > static const?
> 
> ah, keeping const the object, const its members. What's is the best practice
> again?

same answer.

> 
> > >+void
> > 
> > the prototype is static, for C style functions I think you should probably
> > make the definition static as well although I'm not sure exactly what the
> > spec says here.
> 
> If you like I can add it but we never did it before.

I don't think we have that many static C functions with prototypes do we?  I also sort of thought gcc didn't like your way, but maybe that's only in C not C++.

> > > nsXULTextFieldAccessible::ApplyARIAState(PRUint64* aState)
> > > {
> > >   nsHyperTextAccessibleWrap::ApplyARIAState(aState);
> > > 
> > >-  nsStateMapEntry::MapToStates(mContent, aState, eARIAAutoComplete);
> > >+  aria::MapToState(aria::eARIAAutoComplete, mContent->AsElement(), aState);
> > 
> > are you sure that's valid? I'm fairly sure for html where we have real
> > objects, but doesn't xul use a lot of funny xml / js stuffs?
> 
> I'm not sure I follow. This allows to use aria-autocomplete of XUL
> textboxes. What's wrong?

I was concerned that the nsIContent* might not be a dom::Element, but I guess if your sure I'll believe.
Comment 5 alexander :surkov 2012-04-05 01:47:10 PDT
Created attachment 612476 [details] [diff] [review]
patch

adding const stuff, it seems it goes with google style guide http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Use_of_const
Comment 6 Trevor Saunders (:tbsaunde) 2012-04-05 06:18:04 PDT
Comment on attachment 612476 [details] [diff] [review]
patch

\O/
Comment 8 David Bolter [:davidb] 2012-04-05 09:46:51 PDT
Clever :)
Comment 9 Matt Brubeck (:mbrubeck) 2012-04-06 11:30:44 PDT
https://hg.mozilla.org/mozilla-central/rev/fc2d53ab4582

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