spun off bug 804040 comment #15: "Trev, I'm thinking to combine 1) AccTypes.h and protected Accessible::AccessibleTypes 2) make mFlags 64bit unit 3) introduce public Accessible::IsOfType(unit64_t aAccTypes) and protected SetAccType() which take into account shift of StateFlags and ChildrenFlags. How does it sound?" "sorry I forgot to answer that, I'm certainly fine with everything other than making mFlags 64 bits. I'll live with that if we need to, but of course I'd rather not. Note the mRoleMapEntry poitner is pretty low data for a pointer size object maybe we should make that an index into the array and stuff that in the mFlags blob. I'd actually sort of like to use packed bit field stuff to avoid the nasty shifting alltogether so we'd have like uint32_t mTypeFlags : 20; uint32_t mChildFlags : 2; uint32_t mRoleMap : 5; or something the one worrying thing about that is we could make accessibles bigger and not really notice, maybe we can add static asserts? The other thing I started pondering was creating some seperate mTypeInfo struct for accessibles like mNodeInfo for nodes."

(In reply to alexander :surkov from comment #0) > Note the mRoleMapEntry poitner is pretty low data > for a pointer size object maybe we should make that an index into the array > and stuff that in the mFlags blob. agree and it gives us excuse for 64 bit unsigned if we need. Those AccTypes aren't really good for bit mask (since having both eHTMLBRAccessible and eHTMLButtonAccessible is not possible for example) and used in a switch what might be unoptimal.

Trev, about your suggestion to use bitfields. I don't see they are used in Mozilla codebase (https://mxr.mozilla.org/mozilla-central/search?string=\w%2B\s*\%3A\s*\d%2B&regexp=1&find=\.h%24&findi=\.h%24&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central). I might do a wrong query but do you know any example of using it?

Neil, do you know anything about bitfield usage in Mozilla practice like { uint32_t mChildrenFlags: 2; uint32_t mStateFlags: 5; }

I'm sure bitfield usage exists but not in any code with which I am familiar. The nearest equivalent that I'm used to seeing is masks for groups of flags, of which the only example I can think of offhand is the NOTIFY_STATE_ALL web progress flag which is basically all of the NOTIFY_STATE_* flags ORed together.

(In reply to alexander :surkov from comment #2) > Trev, about your suggestion to use bitfields. I don't see they are used in > Mozilla codebase > (https://mxr.mozilla.org/mozilla-central/ > search?string=\w%2B\s*\%3A\s*\d%2B&regexp=1&find=\.h%24&findi=\. > h%24&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central). I might do a wrong > query but do you know any example of using it? nsHTMLInputElement.h is the only case I remember off hand, but I'm sure I've seen others.

Comment on attachment 686485 [details] [diff] [review] part1: split mFlags into bit fields > /** > * Flags used to describe the state of this accessible. > * @note keep these flags in sync with ChildrenFlags > */ update comment? > enum StateFlags { >+ eIsDefunct = 1 << 0, // accessible is defunct >+ eIsNotInDocument = 1 << 1, // accessible is not in document >+ eSharedNode = 1 << 2, // accessible shares DOM node from another accessible >+ eNotNodeMapEntry = 1 << 3, // accessible shouldn't be in document node map >+ eHasNumericValue = 1 << 4 // accessible has a numeric value > }; why not kill this enum, and have a seperate member for each element? so you'd have uint32_t mDefunct : 1; uint32_t mHasNumericValue : 1; ... > type of this accessible. > * @note keep these flags in sync with ChildrenFlags and StateFlags > */ you can remove this part too right? >+ uint32_t mFlags : 25; I'd prefer this was the actual size you need 23. If you want to make the padding explicit why not have something like uint32_t dummy : 2; or you could just let the compiler do it for you

(In reply to Trevor Saunders (:tbsaunde) from comment #7) > > enum StateFlags { > >+ eIsDefunct = 1 << 0, // accessible is defunct > >+ eIsNotInDocument = 1 << 1, // accessible is not in document > >+ eSharedNode = 1 << 2, // accessible shares DOM node from another accessible > >+ eNotNodeMapEntry = 1 << 3, // accessible shouldn't be in document node map > >+ eHasNumericValue = 1 << 4 // accessible has a numeric value > > }; > > why not kill this enum, and have a seperate member for each element? > so you'd have > uint32_t mDefunct : 1; > uint32_t mHasNumericValue : 1; > ... I think I like StateFlags > > > type of this accessible. not enough context, I don't understand the comment > >+ uint32_t mFlags : 25; > > I'd prefer this was the actual size you need 23. If you want to make the > padding explicit why not have something like > uint32_t dummy : 2; > or you could just let the compiler do it for you if you like, I think I wanted to specify explicitly a space we have

(In reply to alexander :surkov from comment #8) > (In reply to Trevor Saunders (:tbsaunde) from comment #7) > > > > enum StateFlags { > > >+ eIsDefunct = 1 << 0, // accessible is defunct > > >+ eIsNotInDocument = 1 << 1, // accessible is not in document > > >+ eSharedNode = 1 << 2, // accessible shares DOM node from another accessible > > >+ eNotNodeMapEntry = 1 << 3, // accessible shouldn't be in document node map > > >+ eHasNumericValue = 1 << 4 // accessible has a numeric value > > > }; > > > > why not kill this enum, and have a seperate member for each element? > > so you'd have > > uint32_t mDefunct : 1; > > uint32_t mHasNumericValue : 1; > > ... > > I think I like StateFlags I prefer IsDefunct() const { return mDefunct; } and mHasNodeMapEntry = true; instead of IsDefunct() const { return mStateFlags & eDecunt; } and mStateFlags |= eHasNodeMapEntry btw state flags doesn't seem great name since most of these flags are always the same based on the type of the accessible > > > > > type of this accessible. > > not enough context, I don't understand the comment me neither :) > > >+ uint32_t mFlags : 25; > > > > I'd prefer this was the actual size you need 23. If you want to make the > > padding explicit why not have something like > > uint32_t dummy : 2; > > or you could just let the compiler do it for you > > if you like, I think I wanted to specify explicitly a space we have yeah, but we can allocate the space as we like, not necessarily to the type bit.

(In reply to Trevor Saunders (:tbsaunde) from comment #9) > > I think I like StateFlags > > I prefer > IsDefunct() const { return mDefunct; } > and > mHasNodeMapEntry = true; > instead of > IsDefunct() const { return mStateFlags & eDecunt; } > and > mStateFlags |= eHasNodeMapEntry I understand the idea, just not sure I want to have many new members, that's why I liked StateFlags approach. In either following the StateFlags approach we just keep closer to what we have. In this bug I wanted to get rid bit masks we had and have separate member for accessible types to have a nice base to move on this bug. > btw state flags doesn't seem great name since most of these flags are always > the same based on the type of the accessible you can say those are permanent states, an accessible characteristic, it depends on type of course and it seems ok. > > > > > > > type of this accessible. > > > > not enough context, I don't understand the comment > > me neither :) it seems to be an quote from my patch, just '>' was omitted. Ignore it. > > > >+ uint32_t mFlags : 25; > > > > > > I'd prefer this was the actual size you need 23. If you want to make the > > > padding explicit why not have something like > > > uint32_t dummy : 2; > > > or you could just let the compiler do it for you > > > > if you like, I think I wanted to specify explicitly a space we have > > yeah, but we can allocate the space as we like, not necessarily to the type > bit. so uint32_t mFlags : 23 and that's all, that's your preference, correct?

(In reply to alexander :surkov from comment #10) > (In reply to Trevor Saunders (:tbsaunde) from comment #9) > > > > I think I like StateFlags > > > > I prefer > > IsDefunct() const { return mDefunct; } > > and > > mHasNodeMapEntry = true; > > instead of > > IsDefunct() const { return mStateFlags & eDecunt; } > > and > > mStateFlags |= eHasNodeMapEntry > > I understand the idea, just not sure I want to have many new members, that's I'm not really sure why the number of members matters... > why I liked StateFlags approach. In either following the StateFlags approach > we just keep closer to what we have. In this bug I wanted to get rid bit > masks we had and have separate member for accessible types to have a nice > base to move on this bug. I guess its true its sort of a while your here thing. > so > uint32_t mFlags : 23 and that's all, that's your preference, correct? sure, I really don't care if you add a dummy or not though.

(In reply to Trevor Saunders (:tbsaunde) from comment #11) > I'm not really sure why the number of members matters... > I guess its true its sort of a while your here thing. I'm fine to while you are here things as long as I share the vision, if I have doubts then I try to squeeze from doing this.

Trev, how do you feel about if AccType constants like eHTMLBRAccessible lost 'Accessible' and look like a11y::AccType nsBRFrame::AccessibleType() { return a11y::eHTMLBR; }

generic idea how to unify accTypes: // AccTypes.h enum AccType { // used for nsIFrame::AccessibleType() implementation eNoType = 0, eHTMLBR = 1, ... eImage = 19, eTextLeaf = 20, // other class dependent types (make sure to not exceed 1 << 5). eApplication = 21, .. // generic types, each accessible may have one or more types, two accessibles may have the same type eAutoComplete = 1 << 5, eAutoCompletePopup = 1 << 6, ... } // Accessible.h class Accessible { public: bool IsOfType(AccType aType) const { return mClassType & aType || mTypes & (aType >> 5); } protected: void SetClassType(AccType aType) { mClassType = aType; } void SetTypes(uint32_t aTypes) { mTypes |= aTypes >> 5; } ... uint32_t mStateFlags : 5; uint32_t mClassType : 5; uint32_t mTypes : 10; }; // HTMLTableRowAccessible.cpp HTMLTableRowAccessible::HTMLTableRowAccessible() { SetClassType(eHTMLTableRow); SetTypes(eTableRow); } // nsImageFrame.cpp a11y::AccType nsImageFrame::AccessibleType() const { return a11y::eImage; } thoughts?

(In reply to alexander :surkov from comment #13) > Trev, how do you feel about if AccType constants like eHTMLBRAccessible lost > 'Accessible' and look like seems fine (In reply to alexander :surkov from comment #14) > generic idea how to unify accTypes: > > // AccTypes.h > > enum AccType { > // used for nsIFrame::AccessibleType() implementation > eNoType = 0, > eHTMLBR = 1, > ... > eImage = 19, > eTextLeaf = 20, > > // other class dependent types (make sure to not exceed 1 << 5). add a static assert? > // generic types, each accessible may have one or more types, two > accessibles may have the same type > eAutoComplete = 1 << 5, > eAutoCompletePopup = 1 << 6, > ... why does putting these in the same enum get us? I'd really rather not have that funny bit shifting logic if there isn't a reason.

(In reply to Trevor Saunders (:tbsaunde) from comment #15) > why does putting these in the same enum get us? I'd really rather not have > that funny bit shifting logic if there isn't a reason. I don't like those shifting, I introduced them to get IsOfType(AccType) public method plain and simple, caller shouldn't care about whether this type is generic or class specific one. Alternatively we could auto generate methods like IsSomething() and AsSomething() as replacement to IsOfType() method. // AccTypeList.h ACCTYPE(HTMLBr) ACCTYPE(Image) ACCBASETYPE(AutoComplete, 1 << 1) ACCBASETYPE(AutoCompletePopup, 1 << 2) // AccType.h #define ACCTYPE(Type) e##Type, enum AccType { #include AccTypeList.h }; #undef ACCTYPE #define ACCBASETYPE(Type, Value) e##Type = ##Value, enum AccBaseType { #include AccTypeList.h } #undef ACCBASETYPE // Accessible.h #define ACCTYPE(Type) \ bool Is##Type(AccType aType) const { return mType == aType; } \ ##aTypeAccessible* As##Type(); #include AccTypeList.h #undef ACCTYPE #define ACCBASETYPE(Type) \ bool Is##Type(AccType aType) const { return (mBaseType & aType); } #include AccTypeList.h #undef ACCBASETYPE // HTMLBrAccessible.h inline HTMLBrAccessible* HTMLBrAccessible::AsHTMLBr() { return static_cast<HTMLBrAccessible*>(this); } This makes things less explicit but maybe nicer. Thoughts?

(In reply to alexander :surkov from comment #16) > (In reply to Trevor Saunders (:tbsaunde) from comment #15) > > > why does putting these in the same enum get us? I'd really rather not have > > that funny bit shifting logic if there isn't a reason. > > I don't like those shifting, I introduced them to get IsOfType(AccType) > public method plain and simple, caller shouldn't care about whether this > type is generic or class specific one. I didn't think of that reason. > Alternatively we could auto generate methods like IsSomething() and > AsSomething() as replacement to IsOfType() method. > > // AccTypeList.h > > ACCTYPE(HTMLBr) > ACCTYPE(Image) > > ACCBASETYPE(AutoComplete, 1 << 1) > ACCBASETYPE(AutoCompletePopup, 1 << 2) ACCBASETYPE is kind of funny way to describe the, not sure what's better though. > #define ACCBASETYPE(Type, Value) e##Type = ##Value, might be nice if you did the bit shift in the macro so you have ACCBASETYPE(AutoComplete, 0) ACCBASETYPE(Foo, 1) then #define ACCBASETYPE(type, bit) e ## type = 1 << bit; > inline HTMLBrAccessible* > HTMLBrAccessible::AsHTMLBr() { return static_cast<HTMLBrAccessible*>(this); } remember these check the type is actually correct :) > This makes things less explicit but maybe nicer. Thoughts? seems fairly nice overall.

maybe ACCGENERICTYPE?

(In reply to alexander :surkov from comment #18) > maybe ACCGENERICTYPE? I guess that works

or maybe rename ACCTYPE to ACCCLASSTYPE or ACCOBJECTTYPE and make the other one ACCTYPE?

You know I lean towards to not introduce this autogenerated code. It shouldn't be hard to write Is/AsSomething() stuff manually but it's explicit which is good. (In either case AsSomething implementation can't be done via autogeneration nicely).

(In reply to alexander :surkov from comment #21) > You know I lean towards to not introduce this autogenerated code. It > shouldn't be hard to write Is/AsSomething() stuff manually but it's explicit explicit is nice, but not having extranius stuff to skip past is nice too :) > which is good. (In either case AsSomething implementation can't be done via > autogeneration nicely). why not? #define ACCCLASSTYPE(type, bit) \ bool Is ##type() const { return mTypeFlag == e ## tag } type* As ##type() const { return Is ##type() ? static_cast<type*>(this) : nullptr; } or am I missing something?

Comment on attachment 686485 [details] [diff] [review] part1: split mFlags into bit fields I guess I'll leave convincing you we should get rid of state enum for another day. btw did you ask wrong neil?

(In reply to Trevor Saunders (:tbsaunde) from comment #25) > Comment on attachment 686485 [details] [diff] [review] > part1: split mFlags into bit fields > > I guess I'll leave convincing you we should get rid of state enum for > another day. ok > btw did you ask wrong neil? I did, thank you.

(In reply to Trevor Saunders (:tbsaunde) from comment #24) > (In reply to alexander :surkov from comment #21) > > You know I lean towards to not introduce this autogenerated code. It > > shouldn't be hard to write Is/AsSomething() stuff manually but it's explicit > > explicit is nice, but not having extranius stuff to skip past is nice too :) agree but I don't really see a solution that I like (I think it it's better what we have but that's not what I would like see) > > which is good. (In either case AsSomething implementation can't be done via > > autogeneration nicely). > > why not? > > #define ACCCLASSTYPE(type, bit) \ > bool Is ##type() const { return mTypeFlag == e ## tag } > type* As ##type() const { return Is ##type() ? static_cast<type*>(this) : > nullptr; } > > or am I missing something? ideally they should live in AccTypeAccessible.h files, we could put all of them in Accessible-inl.h but it doesn't seem nice.

Comment on attachment 688628 [details] [diff] [review] part3: combine Accessible::AccessibleTypes with AccTypes >+ * alphabetical order since they are used in switch statement. >+ */ >+ eNoType = 0, >+ eHTMLBR = 1, why not just let them be the default values? >+ eApplication = 25, is this actually used for something? >+enum AccGenericType { >+ eAutoCompletes = 1 << 0, please don't make these end in s, its really wierd to read. >+ eDocuments = 1 << 3, would it maybe be better to implement IsDoc() as return mType == eDoc || IsRoot() ? > uint32_t mChildrenFlags : 2; > uint32_t mStateFlags : 5; >- uint32_t mFlags : 25; >+ uint32_t mType : 5; >+ uint32_t mGenericTypes : 12; can we add static asserts to make sure things fit?

(In reply to Trevor Saunders (:tbsaunde) from comment #28) > Comment on attachment 688628 [details] [diff] [review] > part3: combine Accessible::AccessibleTypes with AccTypes > > >+ * alphabetical order since they are used in switch statement. > >+ */ > >+ eNoType = 0, > >+ eHTMLBR = 1, > > why not just let them be the default values? I wanted to keep them explicit to not count them when keeping them in sync with mType and mGenericTypes. > >+ eApplication = 25, > > is this actually used for something? in NotificationController::CoalesceReorderEvents > >+enum AccGenericType { > >+ eAutoCompletes = 1 << 0, > > please don't make these end in s, its really wierd to read. I want to differentiate them from type constants. Ideas? > > >+ eDocuments = 1 << 3, > > would it maybe be better to implement IsDoc() as return mType == eDoc || > IsRoot() ? we can do that but one day we will need eDocuments generic type to unify support of ARIA documents. > > uint32_t mChildrenFlags : 2; > > uint32_t mStateFlags : 5; > >- uint32_t mFlags : 25; > >+ uint32_t mType : 5; > >+ uint32_t mGenericTypes : 12; > > can we add static asserts to make sure things fit? can you give me an idea of this?

(In reply to alexander :surkov from comment #29) > (In reply to Trevor Saunders (:tbsaunde) from comment #28) > > Comment on attachment 688628 [details] [diff] [review] > > part3: combine Accessible::AccessibleTypes with AccTypes > > > > >+ * alphabetical order since they are used in switch statement. > > >+ */ > > >+ eNoType = 0, > > >+ eHTMLBR = 1, > > > > why not just let them be the default values? > > I wanted to keep them explicit to not count them when keeping them in sync > with mType and mGenericTypes. if you add static asserts we don't have to think about it :) > > >+ eApplication = 25, > > > > is this actually used for something? > > in NotificationController::CoalesceReorderEvents ok, maybe we just want to compare against ApplicationAcc() someday if we get short on space. > > >+enum AccGenericType { > > >+ eAutoCompletes = 1 << 0, > > > > please don't make these end in s, its really wierd to read. > > I want to differentiate them from type constants. Ideas? not really btw you dislike just using mFoo for each of these because you dislike a bunch of small members for some reason right? How do you feel about something like struct TypeInfo { uint32_t ClassType : 5; uint32_t AutoComplete : 1; uint32_t IsDoc : 1; }; TypeInfo mTypeInfo; I hope compilers generate the same code for that, and it in some sense doesn't have as many members. > > >+ eDocuments = 1 << 3, > > > > would it maybe be better to implement IsDoc() as return mType == eDoc || > > IsRoot() ? > > we can do that but one day we will need eDocuments generic type to unify > support of ARIA documents. true, but then we also ned to worry about how to deal with downcasting to DocAccessible class if aria documents don't get an object of that type. > > > uint32_t mChildrenFlags : 2; > > > uint32_t mStateFlags : 5; > > >- uint32_t mFlags : 25; > > >+ uint32_t mType : 5; > > >+ uint32_t mGenericTypes : 12; > > > > can we add static asserts to make sure things fit? > > can you give me an idea of this? probably the easiest thing to do is add a eLastFoo or something after the last real entry so if all the entries values are implicit it will be the greatest, and then you can put MOZ_STATIC_ASSERT(eLastFoo <= 0x1f, "foos should only use 5 bits!");

(In reply to Trevor Saunders (:tbsaunde) from comment #30) > > I wanted to keep them explicit to not count them when keeping them in sync > > with mType and mGenericTypes. > > if you add static asserts we don't have to think about it :) ok, I hope it helps to not count them to get rid that assertion > > > >+ eApplication = 25, > > > > > > is this actually used for something? > > > > in NotificationController::CoalesceReorderEvents > > ok, maybe we just want to compare against ApplicationAcc() someday if we get > short on space. we could, having a type for it might be an universal way but comparison works too > btw you dislike just using mFoo for each of these because you dislike a > bunch of small members for some reason right? > > How do you feel about something like > > struct TypeInfo { > uint32_t ClassType : 5; > uint32_t AutoComplete : 1; > uint32_t IsDoc : 1; > }; > > TypeInfo mTypeInfo; > > I hope compilers generate the same code for that, and it in some sense > doesn't have as many members. it seems doesn't work quite well for ARIA where generic types are used > > > > >+ eDocuments = 1 << 3, > true, but then we also ned to worry about how to deal with downcasting to > DocAccessible class if aria documents don't get an object of that type. yes, we need to differentiate those while they share the same logic. Anyway, I might not changing it for now. > probably the easiest thing to do is add a eLastFoo or something after the > last real entry so if all the entries values are implicit it will be the > greatest, and then you can put MOZ_STATIC_ASSERT(eLastFoo <= 0x1f, "foos > should only use 5 bits!"); Where should I put it? How does it work? Is it runtime assertion or compilation time?

(In reply to alexander :surkov from comment #31) > (In reply to Trevor Saunders (:tbsaunde) from comment #30) > > > > I wanted to keep them explicit to not count them when keeping them in sync > > > with mType and mGenericTypes. > > > > if you add static asserts we don't have to think about it :) > > ok, I hope it helps to not count them to get rid that assertion not sure what you mean here > > btw you dislike just using mFoo for each of these because you dislike a > > bunch of small members for some reason right? > > > > How do you feel about something like > > > > struct TypeInfo { > > uint32_t ClassType : 5; > > uint32_t AutoComplete : 1; > > uint32_t IsDoc : 1; > > }; > > > > TypeInfo mTypeInfo; > > > > I hope compilers generate the same code for that, and it in some sense > > doesn't have as many members. > > it seems doesn't work quite well for ARIA where generic types are used why? how is it worse than the way we do it now? > > > > >+ eDocuments = 1 << 3, > > > true, but then we also ned to worry about how to deal with downcasting to > > DocAccessible class if aria documents don't get an object of that type. > > yes, we need to differentiate those while they share the same logic. Anyway, > I might not changing it for now. > > > probably the easiest thing to do is add a eLastFoo or something after the > > last real entry so if all the entries values are implicit it will be the > > greatest, and then you can put MOZ_STATIC_ASSERT(eLastFoo <= 0x1f, "foos > > should only use 5 bits!"); > > Where should I put it? How does it work? Is it runtime assertion or > compilation time? its build time, and any random cpp file will do, in this case Accessible.cpp would seem to make sense.

(In reply to Trevor Saunders (:tbsaunde) from comment #32) > > > if you add static asserts we don't have to think about it :) > > > > ok, I hope it helps to not count them to get rid that assertion > > not sure what you mean here say if I added a new constant(s), I've got that static assertion then will I have an idea how to change bit field size? > > > btw you dislike just using mFoo for each of these because you dislike a > > > bunch of small members for some reason right? > > > > > > How do you feel about something like > > > > > > struct TypeInfo { > > > uint32_t ClassType : 5; > > > uint32_t AutoComplete : 1; > > > uint32_t IsDoc : 1; > > > }; > > > > > > TypeInfo mTypeInfo; > > > > > > I hope compilers generate the same code for that, and it in some sense > > > doesn't have as many members. > > > > it seems doesn't work quite well for ARIA where generic types are used > > why? how is it worse than the way we do it now? I'm not sure I see its implementation. It's either I need to have an enum for ARIA role map and then map it into these fields or copy/paste these fields for each ARIA role map entry. > its build time, and any random cpp file will do, in this case Accessible.cpp > would seem to make sense. ok, good. > MOZ_STATIC_ASSERT(eLastFoo <= 0x1f it seems I need to define somewhere 0x1f constant to use it in bit field definition and in assertion

(In reply to alexander :surkov from comment #33) > (In reply to Trevor Saunders (:tbsaunde) from comment #32) > > > > > if you add static asserts we don't have to think about it :) > > > > > > ok, I hope it helps to not count them to get rid that assertion > > > > not sure what you mean here > > say if I added a new constant(s), I've got that static assertion then will I > have an idea how to change bit field size? can you figure out what to do if an assert fails saying that eFoo wasn't less than some constant or some similar message? > > > > btw you dislike just using mFoo for each of these because you dislike a > > > > bunch of small members for some reason right? > > > > > > > > How do you feel about something like > > > > > > > > struct TypeInfo { > > > > uint32_t ClassType : 5; > > > > uint32_t AutoComplete : 1; > > > > uint32_t IsDoc : 1; > > > > }; > > > > > > > > TypeInfo mTypeInfo; > > > > > > > > I hope compilers generate the same code for that, and it in some sense > > > > doesn't have as many members. > > > > > > it seems doesn't work quite well for ARIA where generic types are used > > > > why? how is it worse than the way we do it now? > > I'm not sure I see its implementation. It's either I need to have an enum > for ARIA role map and then map it into these fields or copy/paste these > fields for each ARIA role map entry. hm, we could certainly just use the enum for the map, then cast a pointer to the first meber to uint32_t* and assign all of them from the enum at once, but I'm not sure its nicer. > > its build time, and any random cpp file will do, in this case Accessible.cpp > > would seem to make sense. > > ok, good. > > > MOZ_STATIC_ASSERT(eLastFoo <= 0x1f > > it seems I need to define somewhere 0x1f constant to use it in bit field > definition and in assertion so, maybe the thing to do is assert that is MOZ_STATIC_ASSERT(eFoo | eBar | eQux >> 11 == 0 of course there's no way to be sure the assert uses all the elements so...

So, it's unclear to me what the split into bit fields is supposed to achieve. From reading the patches, my understanding of the problem is that you're using separate bits for every accessible type even those that can never occur in combination with other types. It looks to me as if you can switch to uint16_t mFlags; uint16_t mType; without fiddling around with bit fields.

(In reply to neil@parkwaycc.co.uk from comment #35) > So, it's unclear to me what the split into bit fields is supposed to > achieve. From reading the patches, my understanding of the problem is that > you're using separate bits for every accessible type even those that can > never occur in combination with other types. It looks to me as if you can > switch to uint16_t mFlags; uint16_t mType; without fiddling around with bit > fields. right we run out of 32 bits so we wanted to separate bits for accessible types that can never occur with other types and accessible types that are can be shared. other bits separation was a cleaning up since using bit fields seems to be more friendly and has simpler logic, for example, { mFlags = (mFlags & ~kChildrenFlagsMask) | aFlag; } vs mChildrenFlags = aFlag;

(In reply to neil@parkwaycc.co.uk from comment #35) > So, it's unclear to me what the split into bit fields is supposed to > achieve. From reading the patches, my understanding of the problem is that > you're using separate bits for every accessible type even those that can > never occur in combination with other types. It looks to me as if you can > switch to uint16_t mFlags; uint16_t mType; without fiddling around with bit > fields. you forget the children flags stuff, and since it looks like after these patches there's space I may stuff the index of the aria role map where using in this 32 bit spot, to get rid of the mRoleMapEntry member. So, while we could make some combination of uint8_t and uint16_t members work with some bit shifting I don't see what the point would be.

Comment on attachment 686485 [details] [diff] [review] part1: split mFlags into bit fields Fair enough, but you could have done it under the old scheme if you'd managed your flags properly. enum ChildrenFlags { eChildrenInitialized = 1 << 0, eMixedChildren = 1 << 1 }; // Replaces !IsChildrenFlag(eChildrenUninitialized); inline bool IsChildrenInitialized() const { return mFlags & eChildrenInitialized; } // Replaces IsChildrenFlag(eMixedChildren); inline bool IsMixedChildren() const { return mFlags & eMixedChildren; } // Replaces SetChildrenFlag(eChildrenUninitialized); inline void SetChildrenUninitialized() { mFlags &= ~(eChildrenInitialized | eMixedChidlren); } // Replaces SetChildrenFlag(eEmbeddedChildren); inline void SetChildrenInitialized() { mFlags |= eChildrenInitialized; } // Replaces SetChildrenFlag(eMixedChildren); inline void SetMixedChildren() { mFlags |= eMixedChildren; } // Alternatively inline void SetChildrenFlag(ChildrenFlags aFlag) { mFlags |= aFlag; }

True. I did that work under impression that mChildrenFlags and mStateFlags as logical grouping seems nice.

Comment on attachment 688628 [details] [diff] [review] part3: combine Accessible::AccessibleTypes with AccTypes Neil, it'd be great if you can take a look at this one as well (overall and static asserts issue raised by Trevor).

Comment on attachment 688628 [details] [diff] [review] part3: combine Accessible::AccessibleTypes with AccTypes >+ eAutoCompletes = 1 << 0, >+ eAutoCompletePopups = 1 << 1, >+ eComboboxes = 1 << 2, These being plural is really weirding me out when I read patches on top of this one. > enum StateFlags { > eIsDefunct = 1 << 0, // accessible is defunct > eIsNotInDocument = 1 << 1, // accessible is not in document > eSharedNode = 1 << 2, // accessible shares DOM node from another accessible > eNotNodeMapEntry = 1 << 3, // accessible shouldn't be in document node map > eHasNumericValue = 1 << 4 // accessible has a numeric value > }; why do the generic type flags move to AccTypes.h but not these? I think I'd leave both here. > // HTMLComboboxAccessible: Accessible > > role > HTMLComboboxAccessible::NativeRole() > { >diff --git a/accessible/src/html/HTMLTableAccessible.cpp b/accessible/src/html/HTMLTableAccessible.cpp >--- a/accessible/src/html/HTMLTableAccessible.cpp >+++ b/accessible/src/html/HTMLTableAccessible.cpp >@@ -337,17 +337,17 @@ HTMLTableRowAccessible::NativeRole() > //////////////////////////////////////////////////////////////////////////////// > // HTMLTableAccessible > //////////////////////////////////////////////////////////////////////////////// > > HTMLTableAccessible:: > HTMLTableAccessible(nsIContent* aContent, DocAccessible* aDoc) : > AccessibleWrap(aContent, aDoc), xpcAccessibleTable(this) > { >- mFlags |= eTableAccessible; >+ mGenericTypes |= eTables; > } > > //////////////////////////////////////////////////////////////////////////////// > // HTMLTableAccessible: nsISupports implementation > > NS_IMPL_ISUPPORTS_INHERITED1(HTMLTableAccessible, Accessible, > nsIAccessibleTable) > >diff --git a/accessible/src/html/HTMLTableAccessible.h b/accessible/src/html/HTMLTableAccessible.h >--- a/accessible/src/html/HTMLTableAccessible.h >+++ b/accessible/src/html/HTMLTableAccessible.h >@@ -88,17 +88,20 @@ public: > /** > * HTML table row accessible (html:tr). > */ > class HTMLTableRowAccessible : public AccessibleWrap > { > public: > HTMLTableRowAccessible(nsIContent* aContent, DocAccessible* aDoc) : > AccessibleWrap(aContent, aDoc) >- { mFlags |= eTableRowAccessible | eHTMLTableRowAccessible; } >+ { >+ mType = eHTMLTableRow; >+ mGenericTypes |= eTableRows; >+ } > virtual ~HTMLTableRowAccessible() { } > > NS_DECL_ISUPPORTS_INHERITED > > // Accessible > virtual a11y::role NativeRole(); > }; > >diff --git a/accessible/src/xul/XULColorPickerAccessible.cpp b/accessible/src/xul/XULColorPickerAccessible.cpp >--- a/accessible/src/xul/XULColorPickerAccessible.cpp >+++ b/accessible/src/xul/XULColorPickerAccessible.cpp >@@ -83,17 +83,17 @@ XULColorPickerTileAccessible::ContainerW > //////////////////////////////////////////////////////////////////////////////// > // XULColorPickerAccessible > //////////////////////////////////////////////////////////////////////////////// > > XULColorPickerAccessible:: > XULColorPickerAccessible(nsIContent* aContent, DocAccessible* aDoc) : > XULColorPickerTileAccessible(aContent, aDoc) > { >- mFlags |= eMenuButtonAccessible; >+ mGenericTypes |= eMenuButtons; > } > > //////////////////////////////////////////////////////////////////////////////// > // XULColorPickerAccessible: Accessible > > uint64_t > XULColorPickerAccessible::NativeState() > { >diff --git a/accessible/src/xul/XULComboboxAccessible.cpp b/accessible/src/xul/XULComboboxAccessible.cpp >--- a/accessible/src/xul/XULComboboxAccessible.cpp >+++ b/accessible/src/xul/XULComboboxAccessible.cpp >@@ -23,19 +23,19 @@ using namespace mozilla::a11y; > //////////////////////////////////////////////////////////////////////////////// > > XULComboboxAccessible:: > XULComboboxAccessible(nsIContent* aContent, DocAccessible* aDoc) : > AccessibleWrap(aContent, aDoc) > { > if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type, > nsGkAtoms::autocomplete, eIgnoreCase)) >- mFlags |= eAutoCompleteAccessible; >+ mGenericTypes |= eAutoCompletes; > else >- mFlags |= eComboboxAccessible; >+ mGenericTypes |= eComboboxes; > } > > role > XULComboboxAccessible::NativeRole() > { > return IsAutoComplete() ? roles::AUTOCOMPLETE : roles::COMBOBOX; > } > >diff --git a/accessible/src/xul/XULFormControlAccessible.cpp b/accessible/src/xul/XULFormControlAccessible.cpp >--- a/accessible/src/xul/XULFormControlAccessible.cpp >+++ b/accessible/src/xul/XULFormControlAccessible.cpp >@@ -35,17 +35,17 @@ using namespace mozilla::a11y; > // XULButtonAccessible > //////////////////////////////////////////////////////////////////////////////// > > XULButtonAccessible:: > XULButtonAccessible(nsIContent* aContent, DocAccessible* aDoc) : > AccessibleWrap(aContent, aDoc) > { > if (ContainsMenu()) >- mFlags |= eMenuButtonAccessible; >+ mGenericTypes |= eMenuButtons; > } > > //////////////////////////////////////////////////////////////////////////////// > // XULButtonAccessible: nsISupports > > NS_IMPL_ISUPPORTS_INHERITED0(XULButtonAccessible, Accessible) > > //////////////////////////////////////////////////////////////////////////////// >diff --git a/accessible/src/xul/XULListboxAccessible.cpp b/accessible/src/xul/XULListboxAccessible.cpp >--- a/accessible/src/xul/XULListboxAccessible.cpp >+++ b/accessible/src/xul/XULListboxAccessible.cpp >@@ -103,17 +103,17 @@ XULListboxAccessible:: > XULListboxAccessible(nsIContent* aContent, DocAccessible* aDoc) : > XULSelectControlAccessible(aContent, aDoc), xpcAccessibleTable(this) > { > nsIContent* parentContent = mContent->GetParent(); > if (parentContent) { > nsCOMPtr<nsIAutoCompletePopup> autoCompletePopupElm = > do_QueryInterface(parentContent); > if (autoCompletePopupElm) >- mFlags |= eAutoCompletePopupAccessible; >+ mGenericTypes |= eAutoCompletePopups; > } > } > > NS_IMPL_ADDREF_INHERITED(XULListboxAccessible, XULSelectControlAccessible) > NS_IMPL_RELEASE_INHERITED(XULListboxAccessible, XULSelectControlAccessible) > > nsresult > XULListboxAccessible::QueryInterface(REFNSIID aIID, void** aInstancePtr) >diff --git a/accessible/src/xul/XULMenuAccessible.cpp b/accessible/src/xul/XULMenuAccessible.cpp >--- a/accessible/src/xul/XULMenuAccessible.cpp >+++ b/accessible/src/xul/XULMenuAccessible.cpp >@@ -431,22 +431,22 @@ XULMenuSeparatorAccessible::ActionCount( > //////////////////////////////////////////////////////////////////////////////// > > XULMenupopupAccessible:: > XULMenupopupAccessible(nsIContent* aContent, DocAccessible* aDoc) : > XULSelectControlAccessible(aContent, aDoc) > { > nsMenuPopupFrame* menuPopupFrame = do_QueryFrame(GetFrame()); > if (menuPopupFrame && menuPopupFrame->IsMenu()) >- mFlags |= eMenuPopupAccessible; >+ mType = eMenuPopup; > > // May be the anonymous <menupopup> inside <menulist> (a combobox) > mSelectControl = do_QueryInterface(mContent->GetParent()); > if (!mSelectControl) >- mFlags &= ~eSelectAccessible; >+ mGenericTypes &= ~eSelects; > } > > uint64_t > XULMenupopupAccessible::NativeState() > { > uint64_t state = Accessible::NativeState(); > > #ifdef DEBUG >diff --git a/accessible/src/xul/XULSelectControlAccessible.cpp b/accessible/src/xul/XULSelectControlAccessible.cpp >--- a/accessible/src/xul/XULSelectControlAccessible.cpp >+++ b/accessible/src/xul/XULSelectControlAccessible.cpp >@@ -26,17 +26,17 @@ using namespace mozilla::a11y; > //////////////////////////////////////////////////////////////////////////////// > // XULSelectControlAccessible > //////////////////////////////////////////////////////////////////////////////// > > XULSelectControlAccessible:: > XULSelectControlAccessible(nsIContent* aContent, DocAccessible* aDoc) : > AccessibleWrap(aContent, aDoc) > { >- mFlags |= eSelectAccessible; >+ mGenericTypes |= eSelects; > mSelectControl = do_QueryInterface(aContent); > } > > //////////////////////////////////////////////////////////////////////////////// > // XULSelectControlAccessible: nsAccessNode > > void > XULSelectControlAccessible::Shutdown() >diff --git a/accessible/src/xul/XULTabAccessible.h b/accessible/src/xul/XULTabAccessible.h >--- a/accessible/src/xul/XULTabAccessible.h >+++ b/accessible/src/xul/XULTabAccessible.h >@@ -62,17 +62,17 @@ protected: > /** > * A container of tab panels, xul:tabpanels element. > */ > class XULDeckAccessible : public AccessibleWrap > { > public: > XULDeckAccessible(nsIContent* aContent, DocAccessible* aDoc) : > AccessibleWrap(aContent, aDoc) >- { mFlags |= eXULDeckAccessible; } >+ { mType = eXULDeck; } > > // Accessible > virtual a11y::role NativeRole(); > }; > > > /** > * A tabpanel object, child elements of xul:tabpanels element. Note,the object >diff --git a/accessible/src/xul/XULTreeAccessible.cpp b/accessible/src/xul/XULTreeAccessible.cpp >--- a/accessible/src/xul/XULTreeAccessible.cpp >+++ b/accessible/src/xul/XULTreeAccessible.cpp >@@ -32,33 +32,34 @@ using namespace mozilla::a11y; > //////////////////////////////////////////////////////////////////////////////// > // XULTreeAccessible > //////////////////////////////////////////////////////////////////////////////// > > XULTreeAccessible:: > XULTreeAccessible(nsIContent* aContent, DocAccessible* aDoc) : > AccessibleWrap(aContent, aDoc) > { >- mFlags |= eSelectAccessible | eXULTreeAccessible; >+ mType = eXULTree; >+ mGenericTypes |= eSelects; > > mTree = nsCoreUtils::GetTreeBoxObject(aContent); > NS_ASSERTION(mTree, "Can't get mTree!\n"); > > if (mTree) { > nsCOMPtr<nsITreeView> treeView; > mTree->GetView(getter_AddRefs(treeView)); > mTreeView = treeView; > } > > nsIContent* parentContent = mContent->GetParent(); > if (parentContent) { > nsCOMPtr<nsIAutoCompletePopup> autoCompletePopupElm = > do_QueryInterface(parentContent); > if (autoCompletePopupElm) >- mFlags |= eAutoCompletePopupAccessible; >+ mGenericTypes |= eAutoCompletePopups; > } > > mAccessibleCache.Init(kDefaultTreeCacheSize); > } > > //////////////////////////////////////////////////////////////////////////////// > // XULTreeAccessible: nsISupports and cycle collection implementation > >diff --git a/accessible/src/xul/XULTreeGridAccessible.cpp b/accessible/src/xul/XULTreeGridAccessible.cpp >--- a/accessible/src/xul/XULTreeGridAccessible.cpp >+++ b/accessible/src/xul/XULTreeGridAccessible.cpp >@@ -23,17 +23,17 @@ using namespace mozilla::a11y; > //////////////////////////////////////////////////////////////////////////////// > // XULTreeGridAccessible > //////////////////////////////////////////////////////////////////////////////// > > XULTreeGridAccessible:: > XULTreeGridAccessible(nsIContent* aContent, DocAccessible* aDoc) : > XULTreeAccessible(aContent, aDoc), xpcAccessibleTable(this) > { >- mFlags |= eTableAccessible; >+ mGenericTypes |= eTables; > } > > //////////////////////////////////////////////////////////////////////////////// > // XULTreeGridAccessible: nsISupports implementation > > NS_IMPL_ISUPPORTS_INHERITED1(XULTreeGridAccessible, > XULTreeAccessible, > nsIAccessibleTable) >@@ -275,17 +275,17 @@ XULTreeGridAccessible::CreateTreeItemAcc > //////////////////////////////////////////////////////////////////////////////// > > XULTreeGridRowAccessible:: > XULTreeGridRowAccessible(nsIContent* aContent, DocAccessible* aDoc, > Accessible* aTreeAcc, nsITreeBoxObject* aTree, > nsITreeView* aTreeView, int32_t aRow) : > XULTreeItemAccessibleBase(aContent, aDoc, aTreeAcc, aTree, aTreeView, aRow) > { >- mFlags |= eTableRowAccessible; >+ mGenericTypes |= eTableRows; > > mAccessibleCache.Init(kDefaultTreeCacheSize); > } > > //////////////////////////////////////////////////////////////////////////////// > // XULTreeGridRowAccessible: nsISupports and cycle collection implementation > > NS_IMPL_CYCLE_COLLECTION_INHERITED_1(XULTreeGridRowAccessible,

(In reply to Trevor Saunders (:tbsaunde) from comment #41) > Comment on attachment 688628 [details] [diff] [review] > part3: combine Accessible::AccessibleTypes with AccTypes > > >+ eAutoCompletes = 1 << 0, > >+ eAutoCompletePopups = 1 << 1, > >+ eComboboxes = 1 << 2, > > These being plural is really weirding me out when I read patches on top of > this one. ideas please, I can't say I like those but they are short and different from AccTypes. > > enum StateFlags { > > eIsDefunct = 1 << 0, // accessible is defunct > > eIsNotInDocument = 1 << 1, // accessible is not in document > > why do the generic type flags move to AccTypes.h but not these? I think I'd > leave both here. StateFlags are used inside Accessible class hierarchies in protected part only, accessible types are used even outside a11y. It should be ok to keep them separate. I think the reason why moved those into separate file to not include Accessible.h into layout files.

(In reply to alexander :surkov from comment #42) > (In reply to Trevor Saunders (:tbsaunde) from comment #41) > > Comment on attachment 688628 [details] [diff] [review] > > part3: combine Accessible::AccessibleTypes with AccTypes > > > > >+ eAutoCompletes = 1 << 0, > > >+ eAutoCompletePopups = 1 << 1, > > >+ eComboboxes = 1 << 2, > > > > These being plural is really weirding me out when I read patches on top of > > this one. > > ideas please, I can't say I like those but they are short and different from > AccTypes. I'd probably just go with eDOcument / eAutocomplete, its not clear to me its important for them to be different from AccType ones. > > > enum StateFlags { > > > eIsDefunct = 1 << 0, // accessible is defunct > > > eIsNotInDocument = 1 << 1, // accessible is not in document > > > > why do the generic type flags move to AccTypes.h but not these? I think I'd > > leave both here. > > StateFlags are used inside Accessible class hierarchies in protected part > only, accessible types are used even outside a11y. It should be ok to keep > them separate. I think the reason why moved those into separate file to not > include Accessible.h into layout files. oh, I didn't think the generic ones where used in layout/ ?

(In reply to Trevor Saunders (:tbsaunde) from comment #43) > I'd probably just go with eDOcument / eAutocomplete, its not clear to me its > important for them to be different from AccType ones. it'd be very handy for review process to differentiate things like mType = eImage; mGenericTypes | = eDocument; > > > > enum StateFlags { > > > > eIsDefunct = 1 << 0, // accessible is defunct > > > > eIsNotInDocument = 1 << 1, // accessible is not in document > > > > > > why do the generic type flags move to AccTypes.h but not these? I think I'd > > > leave both here. > > > > StateFlags are used inside Accessible class hierarchies in protected part > > only, accessible types are used even outside a11y. It should be ok to keep > > them separate. I think the reason why moved those into separate file to not > > include Accessible.h into layout files. > > oh, I didn't think the generic ones where used in layout/ ? you told about generic types. They weren't supposed to be in use outside a11y but I'd say AccGenericTypes are closed to AccTypes rather than to StateFlags so keeping those together should be ok (additionally taking into account that AccGenericTypes are used outside Accessible classes hierarchies but StateFlags aren't).

(In reply to alexander :surkov from comment #44) > (In reply to Trevor Saunders (:tbsaunde) from comment #43) > > > I'd probably just go with eDOcument / eAutocomplete, its not clear to me its > > important for them to be different from AccType ones. > > it'd be very handy for review process to differentiate things like > mType = eImage; > mGenericTypes | = eDocument; then why not make accType an enum class using the macros somewhere in mfbt/, and then things should fail to compile you if you do wrong thing by assigning generic type into mType or accType into mGenericType. I suppose we should veryify that compilers do sane things with enum classes in bit fields, but I don't really see why they wouldn't.

example please?

(In reply to alexander :surkov from comment #46) > example please? nsDidReflowStatus in layout/generic/nsIFrame.h or just grep for moz-beGIN_ENUM_CLASS

I must miss something but I don't see how it helps to prevent: uint32_t mType : 5; uint32_t mGenericTypes : 20; enum AccType { eImage }, enum AccGenericType { eDocument } mType = eDocument; mGenericTypes |= eImage;

Comment on attachment 686485 [details] [diff] [review] part1: split mFlags into bit fields http://hg.mozilla.org/integration/mozilla-inbound/rev/b03d124ee821

(In reply to alexander :surkov from comment #48) > I must miss something but I don't see how it helps to prevent: > uint32_t mType : 5; > uint32_t mGenericTypes : 20; > > enum AccType { eImage }, enum AccGenericType { eDocument } > mType = eDocument; mGenericTypes |= eImage; I was thinking you'd do enum class AccType : uint8_t { eImage, ... }; AccType mType : 5; uint32_t mGenericType : 11; then mType = eDocuments would be illegal because your converting from int to AccType which is illegal, and mGericType |= eImage is ilegal because AccType doesn't implicitly convert to integer types. saddly however I suspect that approach doesn't work because while clang is just fine with that g++ 4.7 seems to want some convincing, and I suspect it plain work with the real class thing that gets used to implement enum class on gcc 4.4.

Comment on attachment 688627 [details] [diff] [review] part2: remove 'Accessible' postfix http://hg.mozilla.org/integration/mozilla-inbound/rev/5a1aee7197da

Trev, do you keep in mind that both enums have eHyperText constant which is weird but no better ideas.

Trev, we need to figure out approach asap, it blocks my queue.

added static assertions, (s) issue is not resolved

(In reply to alexander :surkov from comment #55) > Trev, we need to figure out approach asap, it blocks my queue. who about naming the bit ones eIsFoo instead of eFoos? I don't likeas much, but its much better than random s's how about making the AccType ones eFooClass or eFooType?

1) having eIsFoo doesn't look good for stuff like nsRoleMapEntry::IsOfType(AccGenericType) 2) enum AccType { eNoType, eHTMLBRType, eHTMLButtonType }; should be ok since they have restricted usage (inside Accessible class and for accessible creation).

(In reply to alexander :surkov from comment #58) > 1) having eIsFoo doesn't look good for stuff like > nsRoleMapEntry::IsOfType(AccGenericType) slightly yeah. > 2) > enum AccType > { > eNoType, > eHTMLBRType, > eHTMLButtonType > }; > > should be ok since they have restricted usage (inside Accessible class and > for accessible creation). ok, I'll live with that :)

Comment on attachment 692885 [details] [diff] [review] patch2.5: rename AccType constants again (comment #58) > already_AddRefed<Accessible> >- CreatePluginAccessible(nsObjectFrame* aFrame, nsIContent* aContent, >- Accessible* aContext); >+ CreateHTMLObjectFrameAccessible(nsObjectFrame* aFrame, nsIContent* aContent, not sure why you rename it again, but since you do why not make it private? > a11y::AccType > nsComboboxControlFrame::AccessibleType() > { >- return a11y::eHTMLCombobox; >+ return a11y::eHTMLComboboxType; it occurs to me me that if were' going to use Type in the name a better idea would have been to make AccType a enum class, so you'd have to use AccType::eFoo, but I'm fine with this.

(In reply to Trevor Saunders (:tbsaunde) from comment #61) > Comment on attachment 692885 [details] [diff] [review] > patch2.5: rename AccType constants again (comment #58) > > > already_AddRefed<Accessible> > >- CreatePluginAccessible(nsObjectFrame* aFrame, nsIContent* aContent, > >- Accessible* aContext); > >+ CreateHTMLObjectFrameAccessible(nsObjectFrame* aFrame, nsIContent* aContent, > > not sure why you rename it again, but since you do why not make it private? good catch, I just backed out the previous patch #2 into diff file and then renamed eFooAccessible stuff to eFooType. > > a11y::AccType > > nsComboboxControlFrame::AccessibleType() > > { > >- return a11y::eHTMLCombobox; > >+ return a11y::eHTMLComboboxType; > > it occurs to me me that if were' going to use Type in the name a better idea > would have been to make AccType a enum class, so you'd have to use > AccType::eFoo, but I'm fine with this. pls let's get something working, merges are painful. we can rename those later when we get a clear idea what we want to have in the end

(In reply to alexander :surkov from comment #62) > (In reply to Trevor Saunders (:tbsaunde) from comment #61) > > Comment on attachment 692885 [details] [diff] [review] > > patch2.5: rename AccType constants again (comment #58) > > > > > already_AddRefed<Accessible> > > >- CreatePluginAccessible(nsObjectFrame* aFrame, nsIContent* aContent, > > >- Accessible* aContext); > > >+ CreateHTMLObjectFrameAccessible(nsObjectFrame* aFrame, nsIContent* aContent, > > > > not sure why you rename it again, but since you do why not make it private? > > good catch, I just backed out the previous patch #2 into diff file and then > renamed eFooAccessible stuff to eFooType. > > > > a11y::AccType > > > nsComboboxControlFrame::AccessibleType() > > > { > > >- return a11y::eHTMLCombobox; > > >+ return a11y::eHTMLComboboxType; > > > > it occurs to me me that if were' going to use Type in the name a better idea > > would have been to make AccType a enum class, so you'd have to use > > AccType::eFoo, but I'm fine with this. > > pls let's get something working, merges are painful. we can rename those > later when we get a clear idea what we want to have in the end I didn't mean to suggest you do it know :)

Comment on attachment 692908 [details] [diff] [review] part3 (v3): combine Accessible::AccessibleTypes with AccTypes >- case ePlugin: { >+ case ePluginType: { why is that here? XULMenupopupAccessible:: > XULMenupopupAccessible(nsIContent* aContent, DocAccessible* aDoc) : > XULSelectControlAccessible(aContent, aDoc) > { > nsMenuPopupFrame* menuPopupFrame = do_QueryFrame(GetFrame()); > if (menuPopupFrame && menuPopupFrame->IsMenu()) >- mFlags |= eMenuPopupAccessible; >+ mType = eMenuPopup; how does it compile? shouldn't it be eMenuPopupType?

(In reply to Trevor Saunders (:tbsaunde) from comment #65) > >- case ePlugin: { > >+ case ePluginType: { > > why is that here? fixed in 2.5 > >- mFlags |= eMenuPopupAccessible; > >+ mType = eMenuPopup; > > how does it compile? shouldn't it be eMenuPopupType? it doesn't :) fixed locally

Comment on attachment 692885 [details] [diff] [review] patch2.5: rename AccType constants again (comment #58) http://hg.mozilla.org/integration/mozilla-inbound/rev/8697feee8fa1

part3: http://hg.mozilla.org/integration/mozilla-inbound/rev/2e0bc271e28f