Trevor Saunders (:tbsaunde) Assignee
	Description • 9 years ago

      No description provided.

	alexander :surkov (:asurkov)
	Comment 2 • 9 years ago

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; }

	Trevor Saunders (:tbsaunde) Assignee
	Comment 3 • 9 years ago

(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.

	alexander :surkov (:asurkov)
	Comment 4 • 9 years ago

why wouldn't inherit these classes from common interface?

	Trevor Saunders (:tbsaunde) Assignee
	Comment 5 • 9 years ago

(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.

	alexander :surkov (:asurkov)
	Comment 6 • 9 years ago

That makes sense. Do you have more ideas how to avoid these nasty copy/paste, should we have a bug on this?

	Trevor Saunders (:tbsaunde) Assignee
	Comment 7 • 9 years ago

(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.

	alexander :surkov (:asurkov)
	Comment 8 • 9 years ago

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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 9 • 9 years ago

(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.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 10 • 9 years ago

(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.

	alexander :surkov (:asurkov)
	Comment 11 • 9 years ago

(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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 12 • 9 years ago

(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...

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 13 • 9 years ago

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.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 14 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4e8054e7ff96

	Carsten Book [:Tomcat]
	Comment 15 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/4e8054e7ff96