717507 - expandoify states

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

same thing as bug 716644 but for states, win might be a little less here since windows and mac don't use a state map, but maybe we can make them nice anyway.

Comment 1

[Andrzej Skalski]

Attachment: Proposed patch

Patch created after consultation with Trevor Saunders. Did not alter mochitests results on ubuntu 11.04 (21479 passed, 1 failed, 1168 todo, same as clean checkout build).

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 588866 [details] [diff] [review]
Proposed patch


>+
>+
>+#define STATE(name_, number_, atkState_, atkMappingType_, xpcomStateName_) { 

no blank lines here

>+    /*

please just delete all of this.

btw #if 0 is generally a better way to acomplish this.

>   nsTextAttrs.cpp \
>   TextUpdater.cpp \
>+  StateAsserts.cpp \

keep in alphabetical order

>+#include "mozilla/Assertions.h"
>+#include "nsAccessible.h"

do you actually need the second for something?
You should include nsIAccessibleStates.h directly.

>+
>+#define STATE(name_, number_, atkState_, atkMappingType_, xpcomStateName_) \
>+  MOZ_STATIC_ASSERT(number_ < 31 ? \
>+    (1 << number_) == nsIAccessibleStates::xpcomStateName_ : (1 << (number_ - 31)) == nsIAccessibleStates::xpcomStateName_ , \

you could make the xpcom state name argument to the macro by using the ## operator here like this

number < 31 ? blah == nsIAccessibleStates::STATE_ ## xpcomstatename : blah == nsIAccessibleStates::EXT_STATE_ ## xpcomstatename,

>+    "xpcom and internals states don't match");

internal not internals, for added cuteness you could use string concatonation to   add the states that don't match something like this
"internal state " name " with number " number_  " doesn't match xpcom state " xpcomstatename);

>+/**
>+ * This file contains the list of all states used in States.h file.

This file contains the list of internal states and there mappings to external states.

>+ */
>+
>+
>+/**
>+ * This file is designed to be used as inline input to States.h, nsStateMap.h , (to be continued)
>+ * and it should not be used other that within macro preprocessor.

this is worded oddly and I'm not really sure how to improve it I'd probably suggest just dropping it.

>+ *
>+ * All entries must be enclosed in the macro STATE
>+ * The arguments are as follows:
>+ *
>+ * The first argument is the C++ name of the state
>+ * The second argument is the state's number

is the position of the state in the bit field?

>+ * The third argument is the AtkState
>+ * The fourth argument is the AtkMappingType
>+ * The fifth argument is XPCOM state name

the xpcom state name

>+ */
>+
>+/**
>+ * Contrary to intuition, the fifth argument cannot be deduced from first one in all cases.
>+ * Incompatible names are:
>+ * (13) "STATE_CHECKABLE" and "STATE_MARQUEED" (has alias in nsIAccessibleStates.idl)
>+ * (26) "STATE_REQUIRED" and "STATE_ALERT_LOW" (has alias in nsIAccessibleStates.idl)
>+ * (27) "STATE_ALERT" and "STATE_ALERT_MEDIUM"
>+ * (28) "STATE_INVALID" and "STATE_ALERT_HIGH" (has alias in nsIAccessibleStates.idl)
>+ * (39) "EXT_STATE_OPAQUE1" and "EXT_STATE_OPAQUE" (collision in namespace)
>+ */

just /**
 * XXX we should be able to generate the xpcom state name from the internal one
 */


This looks good but I think I'd like to see  these fixed.

Comment 3

[Andrzej Skalski]

Attachment: second version of proposed patch, after applying some of :tbsaunde 's remarks

I was unable to generate xpcom name prefixes using preprocessor without breaking the build. I can describe the problem and describe non-working approaches I've taken.

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 589850 [details] [diff] [review]
second version of proposed patch, after applying some of :tbsaunde 's remarks


>-  { kNone,                                    kNoSuchState },   //                                 = 1 << 47
>+#define STATE(name_, number_, atkState_, atkMappingType_, xpcomStateName_) { atkState_,   atkMappingType_ },

nit, keep lines to 80 chars when you can

>+  StateList.h \
>   States.h \
>   Role.h \

nit, States.h first to be in order.

btw it appears Role.h isn't in order, feel free to fix that while here if you like

>+
>+/**
>+ * This checks compatibility between definitions from StateList.h with nsIAccessibleStates.idl

nit, between internal and xpcom states.

>+#define STATE(name_, number_, atkState_, atkMappingType_, xpcomStateName_) \
>+  MOZ_STATIC_ASSERT(number_ < 31 ? \
>+  (1 << number_) == nsIAccessibleStates::xpcomStateName_ : (1 << (number_ - 31)) == nsIAccessibleStates::xpcomStateName_ , \

nit, break up to be within 80 chars.

>+/**
>+ * XXX we should be able to generate the xpcom state name from the internal one.
>+ * States, that second argument are 13, 26, 27, 28 and 39 stop us from doing that.

nit, states in positions x, y, z stop us from doing that.

>+#define STATE(name_, number_, atkState_, atkMappingType_, xpcomStateName_) const PRUint64 name_ = ((PRUint64) 0x1) << number_;

nit, 80 chars


On closer examination I beieve you could add the string states in nsAccessibilityService.cpp and the ia2 states in msaa/nsAccessibleWrap.cpp to the macro, but this is already imho an improvement worth taking so I could live with that being in a second patch.

r=me, but  we  need  to convince Surkov this isn't bad.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> r=me, but  we  need  to convince Surkov this isn't bad.

I'd prefer that "this is cool" :) Again, what benefits do we get?

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> 
> > r=me, but  we  need  to convince Surkov this isn't bad.
> 
> I'd prefer that "this is cool" :) Again, what benefits do we get?

Having the state mapping info all in one place would be my preference because I can more easily see in one place how different mappings converge or diverge, as well as I suspect it will add up to saving me many hours over the year when I field specification questions. Also, in a general sense I like related info in one place. So this approach gets my vote.

The downside is that we have in the past had a rule about keeping platform specific stuff in the platform folders. I'd personally be okay with breaking that here.

My second choice would be this improvement landing with just the nits fixed and the discussion continuing on a follow up or perhaps we can roshambo at the next work week :)

Comment 7

[alexander :surkov (:asurkov)]

let's finish bug bug 716644 (about roles) and get back to this one. Perhaps we can end up with nice solution that suites everyone.

(In reply to David Bolter [:davidb] from comment #6)

> The downside is that we have in the past had a rule about keeping platform
> specific stuff in the platform folders. I'd personally be okay with breaking
> that here.

maybe we could have 'shared' folder for stuffs like this

Comment 8

[alexander :surkov (:asurkov)]

let's do similar to RoleMap.h

STATE(INTERNALSTATE,
      "stringstate",
      ATK_STATE, ATK_how_to_map,
      MSAA_STATE,
      IA2_STATE);

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #7)
> let's finish bug bug 716644 (about roles) and get back to this one. Perhaps
> we can end up with nice solution that suites everyone.
> 
> (In reply to David Bolter [:davidb] from comment #6)
> 
> > The downside is that we have in the past had a rule about keeping platform
> > specific stuff in the platform folders. I'd personally be okay with breaking
> > that here.
> 
> maybe we could have 'shared' folder for stuffs like this

Maybe. We might want to look at our folder structure again (e.g. base, generic, other, shared, would be overlapping concepts to me).

(In reply to alexander :surkov from comment #8)
> let's do similar to RoleMap.h
> 
> STATE(INTERNALSTATE,
>       "stringstate",
>       ATK_STATE, ATK_how_to_map,
>       MSAA_STATE,
>       IA2_STATE);

Andrzej, did you get a chance to start a WIP for this?

Comment 10

[alexander :surkov (:asurkov)]

Get an idea how to fix from the patch of bug 716644, for the reference take a look at attached wip (you should have States.h and StatesMap.h in the end as we did for roles). Don't hesitate to ask if you want to take this bug.

Comment 11

[Tawah Peggy]

Hello Everyone, I will like to work on this if possible