923289 - introduce relation type map

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

similar to RoleMap.h

Comment 1

[Trevor Saunders (:tbsaunde)]

Comment on attachment 813323 [details] [diff] [review]
patch

>+RELTYPE(LABELLED_BY,

what's wrong with RELATION_TYPE or RELATIONTYPE?

>+class ia2RelTypeIter

what's point of this approach? it seems slow and complicated.  Couldn't you just have static array and iterate over elements, with a bool should ignore flag or something?

>+  struct Pair

any reason you can't just use std::pair?

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 813323 [details] [diff] [review]
patch

>+ia2AccessibleRelation::get_relationType(BSTR* aRelationType)
> {
>   A11Y_TRYBLOCK_BEGIN
> 
>   if (!aRelationType)
>     return E_INVALIDARG;
>-
>   *aRelationType = nullptr;

We must always end up assigning to *aRelationType later so what is the point of this?

> 
>+#define RELTYPE(geckoRelType, stringRelType, atkRelType, \
>+                msaaRelType, ia2RelType) \
>+  case rels::geckoRelType: \
>+    *aRelationType = ::SysAllocString(ia2RelType); \
>+    break;
>+
>   switch (mType) {
>+#include "RelTypeMap.h"
>     default:
>       return E_FAIL;

why not make mType the enum type? then I think you can move the return into each case and get rid of the default case because the compiler should be able to prove its unreachable.

Well actually it might be smarter to move the string allocation out of the switch and have a variable that's uninitialized if no case in the switch is  hit, but maybe its worth it to have the call to the allocation function know which string it will copy.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Comment on attachment 813323 [details] [diff] [review]
> patch
> 
> >+RELTYPE(LABELLED_BY,
> 
> what's wrong with RELATION_TYPE or RELATIONTYPE?

nothing, what's wrong with RELTYPE? (it's short and nice) :)

> >+class ia2RelTypeIter
> 
> what's point of this approach? it seems slow and complicated.  Couldn't you
> just have static array and iterate over elements, with a bool should ignore
> flag or something?

to reduce code duplication and to get nice structure of while (iter.next()), otherwise I need to copy/paste ia2RelTypeIter::Next code, it's not big deal but provides some sugar I guess. Relation array is small and those methods aren't supposed to be used often so performance shouldn't be a bottle neck here. Btw, how much is it slower?

> >+  struct Pair
> 
> any reason you can't just use std::pair?

ok, I can do it I guess

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

> >   if (!aRelationType)
> >     return E_INVALIDARG;
> >-
> >   *aRelationType = nullptr;
> 
> We must always end up assigning to *aRelationType later so what is the point
> of this?

default section doesn't handle it but we shouldn't hit it, I guess, just common practice so we don't need to care about out args initialization in error case

> >   switch (mType) {
> >+#include "RelTypeMap.h"
> >     default:
> >       return E_FAIL;
> 
> why not make mType the enum type? then I think you can move the return into
> each case and get rid of the default case because the compiler should be
> able to prove its unreachable.

sounds good
 
> Well actually it might be smarter to move the string allocation out of the
> switch and have a variable that's uninitialized if no case in the switch is 
> hit, but maybe its worth it to have the call to the allocation function know
> which string it will copy.

why? can you give some code snippet?

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > Comment on attachment 813323 [details] [diff] [review]
> > patch
> > 
> > >+RELTYPE(LABELLED_BY,
> > 
> > what's wrong with RELATION_TYPE or RELATIONTYPE?
> 
> nothing, what's wrong with RELTYPE? (it's short and nice) :)

REL doesn't seem very nice as short form of relation

> > >+class ia2RelTypeIter
> > 
> > what's point of this approach? it seems slow and complicated.  Couldn't you
> > just have static array and iterate over elements, with a bool should ignore
> > flag or something?
> 
> to reduce code duplication and to get nice structure of while (iter.next()),
> otherwise I need to copy/paste ia2RelTypeIter::Next code, it's not big deal
> but provides some sugar I guess. Relation array is small and those methods
> aren't supposed to be used often so performance shouldn't be a bottle neck
> here. Btw, how much is it slower?

for (unsinged i = 0; i < ArrayLength(x); i++)
  if (x[i].use)
    doSomething;

doesn't really seem bad, I might argue its nicer than iterator over array.

if nothing else its just silly to store a bunch of pointers when you only need a bit for each role. (of course you have to be a little tricky to not end up with the same space being consumed because of alignment, but two arrays should be easy enough).

> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > >   if (!aRelationType)
> > >     return E_INVALIDARG;
> > >-
> > >   *aRelationType = nullptr;
> > 
> > We must always end up assigning to *aRelationType later so what is the point
> > of this?
> 
> default section doesn't handle it but we shouldn't hit it, I guess, just
> common practice so we don't need to care about out args initialization in
> error case

I thought technically you wheren't supposed to touch out args in error cases (of course nobody actually cares).

anyway I really want the default case to go away

> > switch and have a variable that's uninitialized if no case in the switch is 
> > hit, but maybe its worth it to have the call to the allocation function know
> > which string it will copy.
> 
> why? can you give some code snippet?


wchar_t *str;
switch (type) {
  case foo:
  str = L"blah";
...
}

*outArg = copy(str);

of course the compiler can probably do this transformation either direction so it probably doesn't matter.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 6

[Trevor Saunders (:tbsaunde)]

Comment on attachment 818794 [details] [diff] [review]
patch2

>+static void
>+UpdateAtkRelation(RelationType aType, Accessible* aAcc,
>+                  AtkRelationType aAtkType, AtkRelationSet* aAtkSet)
>+{
>+  if (aAtkType != ATK_RELATION_NULL) {

wouldn't it be nicer to return early?

>+    if (targets.Length()) {
>+      atkRelation = atk_relation_new(targets.Elements(),
>+                                     targets.Length(), aAtkType);

wouldn't it be safer to assign null to it if !targets.Length() ?

>     'nsTextEquivUtils.cpp',
>-    'RoleAsserts.cpp',
>+    'Asserts.cpp',

keep in alphabetical order?

>+#define RELATIONTYPE(geckoType, geckoTypeName, atkType, msaaType, ia2Type) \
>+  case RelationType::geckoType: \
>+    CopyUTF8toUTF16(geckoTypeName, aString); \

aString.AssignLiteral()  would be faster

 >+  RelationType relationType = static_cast<RelationType>(aRelationType);

I'd prefer you checked bounds before casting rather than relying on the default case not getting optimized out, but I guess only chrome code can shoot itself in the foot so whatever.

>+  for (unsigned int idx = 0; idx < ArrayLength(sRelationTypePairs); idx++) {

size_t would be shorter ;)

Comment 7

[alexander :surkov (:asurkov)]

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

> >+    if (targets.Length()) {
> >+      atkRelation = atk_relation_new(targets.Elements(),
> >+                                     targets.Length(), aAtkType);
> 
> wouldn't it be safer to assign null to it if !targets.Length() ?

why? it's not in use if no targets

>  >+  RelationType relationType = static_cast<RelationType>(aRelationType);
> 
> I'd prefer you checked bounds before casting rather than relying on the
> default case not getting optimized out, but I guess only chrome code can
> shoot itself in the foot so whatever.

ok

Comment 8

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1811271ac7e1

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1811271ac7e1

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> 
> > >+    if (targets.Length()) {
> > >+      atkRelation = atk_relation_new(targets.Elements(),
> > >+                                     targets.Length(), aAtkType);
> > 
> > wouldn't it be safer to assign null to it if !targets.Length() ?
> 
> why? it's not in use if no targets

yeah, I missed use was in if.