Closed Bug 1138436 Opened 5 years ago Closed 5 years ago

start on proxying IAccessible2

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(1 file)

No description provided.
Attachment #8571359 - Flags: review?(surkov.alexander)
Comment on attachment 8571359 [details] [diff] [review]
start on IAccessible2

Review of attachment 8571359 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/windows/ia2/ia2Accessible.cpp
@@ +239,5 @@
> +  a11y::role geckoRole;
> +  if (acc->IsProxy())
> +    geckoRole = acc->Proxy()->Role();
> +  else
> +    geckoRole = acc->Role();

wouldn't it be better to make Proxy() to return something to avoid this if/else code bloat everywhere, it could be like

Accessible* Intl() { return IsProxy() ? Proxy() : this; }
(In reply to alexander :surkov from comment #2)
> Comment on attachment 8571359 [details] [diff] [review]
> start on IAccessible2
> 
> Review of attachment 8571359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/windows/ia2/ia2Accessible.cpp
> @@ +239,5 @@
> > +  a11y::role geckoRole;
> > +  if (acc->IsProxy())
> > +    geckoRole = acc->Proxy()->Role();
> > +  else
> > +    geckoRole = acc->Role();
> 
> wouldn't it be better to make Proxy() to return something to avoid this
> if/else code bloat everywhere, it could be like
> 
> Accessible* Intl() { return IsProxy() ? Proxy() : this; }

given you are calling different functions I don't see how that would work.
why wouldn't inherit these classes from common interface?
(In reply to alexander :surkov from comment #4)
> why wouldn't inherit these classes from common interface?

that seems like a lot of work, and you don't even want to present the same interface a lot of the time.
That makes sense. Do you have more ideas how to avoid these nasty copy/paste, should we have a bug on this?
(In reply to alexander :surkov from comment #6)
> That makes sense. Do you have more ideas how to avoid these nasty
> copy/paste, should we have a bug on this?

I don't have any ideas I particularly like yet.  You could file a bug, but I don't really see what it would accomplish.
Comment on attachment 8571359 [details] [diff] [review]
start on IAccessible2

Review of attachment 8571359 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see anything wrong here except broken ia2Accessible::get_states, so r=me but I think David should take a look too, besides other things I'm not sure how performance is important for you at this implementation stage.

::: accessible/ipc/ProxyAccessible.h
@@ +40,5 @@
>    { mChildren.InsertElementAt(aIdx, aChild); }
>  
>    uint32_t ChildrenCount() const { return mChildren.Length(); }
>    ProxyAccessible* ChildAt(uint32_t aIdx) const { return mChildren[aIdx]; }
> +  size_t IndexInParent() const { return mParent->mChildren.IndexOf(this); }

this should be a perf problem, iirc we cached index in parent because we wanted to have it fast.

::: accessible/windows/ia2/ia2Accessible.cpp
@@ +70,5 @@
>  
> +  if (acc->IsProxy()) {
> +    nsTArray<RelationType> types;
> +    nsTArray<nsTArray<ProxyAccessible*>> targetSets;
> +    acc->Proxy()->Relations(&types, &targetSets);

all relation targets traversal is bad for perf too

@@ +107,5 @@
> +    nsTArray<RelationType> types;
> +    nsTArray<nsTArray<ProxyAccessible*>> targetSets;
> +    acc->Proxy()->Relations(&types, &targetSets);
> +
> +    size_t sets = targetSets.Length();

seems like a bad naming, 'targetSets' is array, 'sets' is number

@@ +110,5 @@
> +
> +    size_t sets = targetSets.Length();
> +    for (size_t i = 0; i < sets; i++) {
> +      uint32_t relTypeIdx = static_cast<uint32_t>(types[i]);
> +      MOZ_ASSERT(sRelationTypePairs[relTypeIdx].first == types[i]);

in general it's not safe assumption, assert doesn't help until you trigger the method. If you really want to base on this assumption then you'd rather had static assertion.

@@ +174,5 @@
> +    nsTArray<RelationType> types;
> +    nsTArray<nsTArray<ProxyAccessible*>> targetSets;
> +    acc->Proxy()->Relations(&types, &targetSets);
> +
> +    size_t count = std::min (targetSets.Length(),

nit: extra space after min

@@ +179,5 @@
> +                             static_cast<size_t>(aMaxRelations));
> +    size_t i = 0;
> +    while (i < count) {
> +      uint32_t relTypeIdx = static_cast<uint32_t>(types[i]);
> +      if (sRelationTypePairs[relTypeIdx].second == IA2_RELATION_NULL)

you don't need variable if it's used only here

@@ +252,5 @@
>    // Special case, if there is a ROLE_ROW inside of a ROLE_TREE_TABLE, then call
>    // the IA2 role a ROLE_OUTLINEITEM.
> +  if (acc->IsProxy()) {
> +    if (geckoRole == roles::ROW && acc->Proxy()->Parent()
> +        && acc->Proxy()->Parent()->Role() == roles::TREE_TABLE)

nit: && on previous line

@@ +351,5 @@
>    // XXX: bug 344674 should come with better approach that we have here.
>  
>    AccessibleWrap* acc = static_cast<AccessibleWrap*>(this);
> +  if (acc->IsDefunct())
> +    return CO_E_OBJNOTCONNECTED;

defunct accessible has to return defunct state

@@ +630,5 @@
> +  if (attrStr.IsEmpty())
> +    return S_FALSE;
> +
> +  *aAttributes = ::SysAllocStringLen(attrStr.get(), attrStr.Length());
> +  return *aAttributes ? S_OK : E_OUTOFMEMORY;

it'd be good to reuse this code for text attributes as well
Attachment #8571359 - Flags: review?(surkov.alexander)
Attachment #8571359 - Flags: review?(dbolter)
Attachment #8571359 - Flags: review+
(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to alexander :surkov from comment #6)
> > That makes sense. Do you have more ideas how to avoid these nasty
> > copy/paste, should we have a bug on this?
> 
> I don't have any ideas I particularly like yet.  You could file a bug, but I
> don't really see what it would accomplish.

I'd feel better if we at least commented copy/paste code for now.
(In reply to David Bolter [:davidb] from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > (In reply to alexander :surkov from comment #6)
> > > That makes sense. Do you have more ideas how to avoid these nasty
> > > copy/paste, should we have a bug on this?
> > 
> > I don't have any ideas I particularly like yet.  You could file a bug, but I
> > don't really see what it would accomplish.
> 
> I'd feel better if we at least commented copy/paste code for now.

its not so much copy and paste as similar logic implemented in somewhat different way.

> ::: accessible/ipc/ProxyAccessible.h
> @@ +40,5 @@
> >    { mChildren.InsertElementAt(aIdx, aChild); }
> >  
> >    uint32_t ChildrenCount() const { return mChildren.Length(); }
> >    ProxyAccessible* ChildAt(uint32_t aIdx) const { return mChildren[aIdx]; }
> > +  size_t IndexInParent() const { return mParent->mChildren.IndexOf(this); }
> 
> this should be a perf problem, iirc we cached index in parent because we
> wanted to have it fast.

I'm pretty unconvinced its a real problem, so I'd rather wait till someone comes up with numbers before changing it.

> ::: accessible/windows/ia2/ia2Accessible.cpp
> @@ +70,5 @@
> >  
> > +  if (acc->IsProxy()) {
> > +    nsTArray<RelationType> types;
> > +    nsTArray<nsTArray<ProxyAccessible*>> targetSets;
> > +    acc->Proxy()->Relations(&types, &targetSets);
> 
> all relation targets traversal is bad for perf too

more likely, but still I'd rather wait for it to be a real problem.

> @@ +110,5 @@
> > +
> > +    size_t sets = targetSets.Length();
> > +    for (size_t i = 0; i < sets; i++) {
> > +      uint32_t relTypeIdx = static_cast<uint32_t>(types[i]);
> > +      MOZ_ASSERT(sRelationTypePairs[relTypeIdx].first == types[i]);
> 
> in general it's not safe assumption, assert doesn't help until you trigger
> the method. If you really want to base on this assumption then you'd rather
> had static assertion.

That's how the array is defined, so imo its fine though maybe the assert is pointless because its definitionally true.

> @@ +179,5 @@
> > +                             static_cast<size_t>(aMaxRelations));
> > +    size_t i = 0;
> > +    while (i < count) {
> > +      uint32_t relTypeIdx = static_cast<uint32_t>(types[i]);
> > +      if (sRelationTypePairs[relTypeIdx].second == IA2_RELATION_NULL)
> 
> you don't need variable if it's used only here

I think lines broke up more reasonably that way.
(In reply to Trevor Saunders (:tbsaunde) from comment #10)

> > I'd feel better if we at least commented copy/paste code for now.
> 
> its not so much copy and paste as similar logic implemented in somewhat
> different way.

that depends, but either way it'd be nice to reduce code bloat, at least for sake of readability.

> > ::: accessible/ipc/ProxyAccessible.h

> > this should be a perf problem, iirc we cached index in parent because we
> > wanted to have it fast.
> 
> I'm pretty unconvinced its a real problem, so I'd rather wait till someone
> comes up with numbers before changing it.

> more likely, but still I'd rather wait for it to be a real problem.

that's basically why I asked David for a second review

> > @@ +110,5 @@
> > > +
> > > +    size_t sets = targetSets.Length();
> > > +    for (size_t i = 0; i < sets; i++) {
> > > +      uint32_t relTypeIdx = static_cast<uint32_t>(types[i]);
> > > +      MOZ_ASSERT(sRelationTypePairs[relTypeIdx].first == types[i]);
> > 
> > in general it's not safe assumption, assert doesn't help until you trigger
> > the method. If you really want to base on this assumption then you'd rather
> > had static assertion.
> 
> That's how the array is defined, so imo its fine though maybe the assert is
> pointless because its definitionally true.

the code is just harder to maintain, because of implicit code relations like this


> > > +    size_t i = 0;
> > > +    while (i < count) {
> > > +      uint32_t relTypeIdx = static_cast<uint32_t>(types[i]);
> > > +      if (sRelationTypePairs[relTypeIdx].second == IA2_RELATION_NULL)
> > 
> > you don't need variable if it's used only here
> 
> I think lines broke up more reasonably that way.

are you sure that optimizer will do a right thing here? you can break it into two lines btw
(In reply to alexander :surkov from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)

> > > ::: accessible/ipc/ProxyAccessible.h
> 
> > > this should be a perf problem, iirc we cached index in parent because we
> > > wanted to have it fast.
> > 
> > I'm pretty unconvinced its a real problem, so I'd rather wait till someone
> > comes up with numbers before changing it.
> 
> > more likely, but still I'd rather wait for it to be a real problem.
> 
> that's basically why I asked David for a second review

I seem to recall work to make our relations code faster but I don't remember if we have benchmarks. I suspect NVDA grabs all relations but not sure...
Comment on attachment 8571359 [details] [diff] [review]
start on IAccessible2

Review of attachment 8571359 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if you can add some kind of "// XXX perf?" comments and we can profile later.
Attachment #8571359 - Flags: review?(dbolter) → review+
https://hg.mozilla.org/mozilla-central/rev/4e8054e7ff96
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee: nobody → tbsaunde+mozbugs
Depends on: 1159872
You need to log in before you can comment on or make changes to this bug.