641838 - dexpcom accessible relation methods

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

1) Provide GetRelationByType() alternative (use nsAccessibleRelation)
2) Fix nsAccessibleRelation and nsRelUtils to work with nsAccessible

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: part 1 provide non xpcom relation AccRelation

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: temp

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: kill *RelationWrap

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: rename non xpcom GetRelationByType to RelationByType()

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: create non xpcom RelationsCount()

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: provide non xpcom version of GetRelations()

Comment 7

[Trevor Saunders (:tbsaunde)]

Attachment: cleanup mostly atk ref relation set

Comment 8

[alexander :surkov (:asurkov)]

perhaps I missed but where AccRelation class is defined?

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: add missing files AccRelation.{cpp,h}

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: (wip) rework accessible relations

very rough patch, but should have most of the general idea.

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: calculate most of a relations targets lazzily

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: patch

use iterators to back relations.  still very much just a idea on a white board, we could probably get rid of InternalIter fiarly easily.  The transfer of iterators ownership is a little weird, but its internal to the iterators so I can live with it if we decide this is the best approach.

Comment 13

[alexander :surkov (:asurkov)]

Trevor, could you show how new iterators are used?

Comment 14

[Trevor Saunders (:tbsaunde)]

Attachment: next iteration

ok, a bit cleaner, uses nsAutoPtr's to hold the next relation in the list and pass new relations into a relation.  Also uses inheritance from AccIterable instead of defining new classes.

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: patch

this builds for me on both linux and windows.  I haven't started on the tests yet.

Comment 16

[Trevor Saunders (:tbsaunde)]

We'd like to do something like this.

class iter
{
  AdoptIter(iter aIter) { mIter = aIter }

  ....
  protected:
  nsAutoPtr<iter> mIter;
};

class iter2: public iter
{
...
}

then we'd like to have methods on other classes like
iter* GetIter(type)
{
  switch (type) {
  case 1:
    foo = new iter(blah);
    foo->AdoptIter(new iter2(baz));
  }

  return foo;
}

Neil, we're looking for some advice on reasonable ways to return the created iterator, and ways for the AdoptIter() method to work.

Comment 17

[alexander :surkov (:asurkov)]

Approach where AccIterable is a list that's managed by Relation class.

class AccIterable
{
public:
  virtual ~AccIterable()
  {
    NS_ASSERTION(mNextIterable, "Memory leak!");
  }

  // iterate through accessibles, do not touch next iterable.
  virtual nsAccessible* Next() const;

private:
  // mNextIterable is used by Relation class to organize a list of iterables
  friend class Relation;
  AccIterable* mNextIterable;
};

class SomeIterator : public AccIterable {};

class SomeIterator2 : public AccIterable {};

class Relation
{
public:
  Relation();

  // take ownership of iterables from other iterator
  Relation(const Relation& aRelation) :
    mFirst(aRelation.mFirst), mLast(aRelation.mLast)
  {
    aRelation.mFirst = nsnull;
    aRelation.mLast = nsnull;
  }

  // if owns iterables then run through them and free memory 
  ~Relation()
  {
    while (mFirst) {
      AccIterable* nextIterable = mFirst.mNextIterable;
      mFirst.mNextIterable = nsnull;
      delete mFirst;
      mFirst = nextIterable;
    }
    mLast = nsnull;
  }

  // return next target from interable if any, otherwise from next iterable
  // free iterables as we go, adjust mFirst.
  nsAccessible* NextTarget() const
  {
    while (mFirst) {
      nsAccessible* target = mFirst->Next();
      if (target)
        return target;

      AccIterable* nextIterable = mFirst.mNextIterable;
      mFirst.mNextIterable = nsnull;
      delete mFirst;
      mFirst = nextIterable;
    }
    return nsnull;
  }

  // put into the end of list, keep ownership
  void AdoptIter(AccIterable* aIter)
  {
    if (!mFirst) { mFirst = aIter; mLast = aIter; return; }
    mLast->mNextIterable = aIter;
    mLast = aIter;
  }

private:
  AccIterable* mFirst;
  AccIterable* mLast;
};

Relation
nsAccessible::RelationByType(PRUint32 aType)
{
  Relation rel;
  if (statement)
    rel.AdoptIter(new SomeIterator());
  if (statement)
    rel.AdoptIter(new SomeIterator2());

  return rel;
}

void
nsAccessible::SomeMethod()
{
  Relation rel = RelationByType(SOME_TYPE);
  while ((nsAccessible* target = rel.NextTarget()))
    DoSomething();
}

Comment 18

[Neil Lee [:neilio]]

The wrong neil is cc'ed on this - can you please double check when you cc someone?

Comment 19

[alexander :surkov (:asurkov)]

Trevor, it really makes sense to cc right Neil ;)

Comment 20

[[e-mail]]

(In reply to comment #17)
> Approach where AccIterable is a list that's managed by Relation class.
I quite like this approach. It reminds me of nsTreeSelection/nsTreeRange.

>   virtual ~AccIterable()
>   {
>     NS_ASSERTION(mNextIterable, "Memory leak!");
[nsTreeRange just does delete mNextIterable; this makes ~Relation() simpler.]

>   // iterate through accessibles, do not touch next iterable.
[It's private, so subclasses can't touch iterable if they tried!]

>   virtual nsAccessible* Next() const;
[Probably should be pure virtual i.e. = 0;]

>   Relation();
Relation(): mFirst(nsnull), mLast(nsnull) ?

>   void AdoptIter(AccIterable* aIter)
[AppendIter might make it clearer that it's keeping a list.]

>     if (!mFirst) { mFirst = aIter; mLast = aIter; return; }
>     mLast->mNextIterable = aIter;
Could write like this:
if (!mFirst)
  mFirst = aIter;
else
  mLast->mNextIterable = aIter;

>   AccIterable* mLast;
Is Relation going to be a stack-based class? I'm just wondering how long this pointer will be needed for.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> (In reply to comment #17)
> > Approach where AccIterable is a list that's managed by Relation class.
> I quite like this approach. It reminds me of nsTreeSelection/nsTreeRange.

great to hear! Trevor, so that's the way to go.

> >   AccIterable* mLast;
> Is Relation going to be a stack-based class? I'm just wondering how long
> this pointer will be needed for.

I can't think of more than three

Comment 22

[Trevor Saunders (:tbsaunde)]

(In reply to comment #19)
> Trevor, it really makes sense to cc right Neil ;)

yes, I thought  that was the right email before :/

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #17)
> > > Approach where AccIterable is a list that's managed by Relation class.
> > I quite like this approach. It reminds me of nsTreeSelection/nsTreeRange.
> 
> great to hear! Trevor, so that's the way to go.

yeah, I like it too, good idea :)

> > >   AccIterable* mLast;
> > Is Relation going to be a stack-based class? I'm just wondering how long
> > this pointer will be needed for.
> 
> I can't think of more than three

three what???

Is there a reason you don't want to add the new iter to the front of the list and get rid of the mLast pointer?

(In reply to comment #17)
>   Relation(const Relation& aRelation) :
>     mFirst(aRelation.mFirst), mLast(aRelation.mLast)
>   {
>     aRelation.mFirst = nsnull;
>     aRelation.mLast = nsnull;
>   }

doesn't that need to take a non const relation? and don't we also want to define operator =?

>   AccIterable* mFirst;

wouldn't making this and mNextIter nsAutoPtr's make this a bit simpler and do the same exact thing?

> >   void AdoptIter(AccIterable* aIter)
> [AppendIter might make it clearer that it's keeping a list.]

on the other hand that seems less clear that ownership of the iter it took changed

Comment 24

[[e-mail]]

(In reply to comment #23)
> Is there a reason you don't want to add the new iter to the front of the
> list and get rid of the mLast pointer?
You're going to do more iterating than constructing, so it makes sense to make iteration faster, so you want the first iterator to be at the front of the list. You could of course reverse your construction code to construct the iterators in reverse order...

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to comment #24)
> (In reply to comment #23)
> > Is there a reason you don't want to add the new iter to the front of the
> > list and get rid of the mLast pointer?
> You're going to do more iterating than constructing, so it makes sense to
> make iteration faster, so you want the first iterator to be at the front of
> the list. You could of course reverse your construction code to construct
> the iterators in reverse order...

well, that assumes we care about ordering in the first place

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > Is there a reason you don't want to add the new iter to the front of the
> > > list and get rid of the mLast pointer?
> > You're going to do more iterating than constructing, so it makes sense to
> > make iteration faster, so you want the first iterator to be at the front of
> > the list. You could of course reverse your construction code to construct
> > the iterators in reverse order...
> 
> well, that assumes we care about ordering in the first place

we care, but as Neil said you can always reverse construction code to achieve this. So makes sense I think.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #23)

> > > >   AccIterable* mLast;
> > > Is Relation going to be a stack-based class? I'm just wondering how long
> > > this pointer will be needed for.
> > 
> > I can't think of more than three
> 
> three what???

iterators of course

> >   Relation(const Relation& aRelation) :
> >     mFirst(aRelation.mFirst), mLast(aRelation.mLast)
> >   {
> >     aRelation.mFirst = nsnull;
> >     aRelation.mLast = nsnull;
> >   }
> 
> doesn't that need to take a non const relation? and don't we also want to
> define operator =?

right/right

> >   AccIterable* mFirst;
> 
> wouldn't making this and mNextIter nsAutoPtr's make this a bit simpler and
> do the same exact thing?

depends how you manage it, I didn't use it in my code snippet because previous iterator is destroyed early than next iterator, nsAutoPtr would destroy next iterator as well.

> > >   void AdoptIter(AccIterable* aIter)
> > [AppendIter might make it clearer that it's keeping a list.]
> 
> on the other hand that seems less clear that ownership of the iter it took
> changed

maybe ownership can be reasonable assumption when we name the method AppendIter because AdoptIter says nothing that we add iterator (for example, vs not replace the existing one).

Comment 28

[[e-mail]]

(In reply to comment #27)
> (In reply to comment #23)
> > >   AccIterable* mFirst;
> > wouldn't making this and mNextIter nsAutoPtr's make this a bit simpler and
> > do the same exact thing?
> depends how you manage it, I didn't use it in my code snippet because
> previous iterator is destroyed early than next iterator, nsAutoPtr would
> destroy next iterator as well.
Actually I think nsAutoPtr can handle this:

      AccIterable* nextIterable = mFirst.mNextIterable;
      mFirst.mNextIterable = nsnull;
      delete mFirst;
      mFirst = nextIterable;

becomes

     mFirst = mFirst.mNextIterable;

Strange but true! This is because = is syntactic sugar for

     mFirst.assign(mFirst.mNextIterable.forget());

and the assign then deletes the old mFirst object, but not its mNextIterable, because the old mFirst object has already forgotten about it.

Comment 29

[Trevor Saunders (:tbsaunde)]

Attachment: wip

Comment 30

[Trevor Saunders (:tbsaunde)]

Attachment: wip 2

This builds and passes a significant number of tests on linux for me.  However it crashes when you try to get things related to a root accessible with RELATION_EMBEDS.  It looks like a const Relation is passed in to the constructor in GetRelationByType() and then while the new Relations mFirstIter points to the right place the object in nsRootAccessible::RelationByType() also points at a object instead of being made null, I assume because it is const so the compiler uses .get(0 to get its mRawPtr instead of .forget() then when the stack frame for nsRootAccessible::RelationByType(0 is torn down the destructor on Relation calls the nsAutoPtr destructor causing the iterator to get deleted but the other nsAutoPtr to have a pointer to freed memory.  then when we create the nsAccessibleRelation its allocated the space where the iterator was.  Which means that when we do Relation.Next() in the nsAccessibleRelation constructor we end up calling nsAccessibleRelation::Release() instead of Next() on a AccIterable.  Relase return 0xffffffff since the refcount was 0 since its never been incromented, and from there we get a nsIAccessible* of 0x1000000f or so which we try to add to the array so we try AddRef() 0x1000000f which of course crashes.

I've just started thinking about how to get around this.

Comment 31

[Trevor Saunders (:tbsaunde)]

Attachment: wip2

I hear it helps to add new files ;)

Comment 32

[Trevor Saunders (:tbsaunde)]

Attachment: patch v1

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 547911 [details] [diff] [review]
patch v1

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

note for myself: stopped at msaa/nsAccessibleRelationWrap.cpp and no nsAccessible review

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +54,2 @@
>  #include "nsStateMap.h"
> +#include "Relation.h"

isn't it more comfortable to keep #include nsIAccessibleRelation.h in Relation.h even it's not used there?

@@ +946,5 @@
>  AtkRelationSet *
>  refRelationSetCB(AtkObject *aAtkObj)
>  {
> +  AtkRelationSet* relation_set =
> +    ATK_OBJECT_CLASS(parent_class)->ref_relation_set(aAtkObj);

new line please

::: accessible/src/base/AccIterator.h
@@ +45,5 @@
>  
> +#include "nsIDOMDocumentXBL.h"
> +
> +/**
> + * AccIterable is a basic interface for iterators over accessibles

nit: dot in the end

@@ +49,5 @@
> + * AccIterable is a basic interface for iterators over accessibles
> + */
> +class AccIterable
> +{
> +  public:

nit: no indentation for public

@@ +55,5 @@
> +    virtual nsAccessible* Next() = 0;
> +
> +private:
> +    friend class Relation;
> +    nsAutoPtr<AccIterable> mNextIter;

nit: two space indentation

@@ +62,5 @@
>  /**
>   * Allows to iterate through accessible children or subtree complying with
>   * filter function.
>   */
> +class AccIterator: public AccIterable

nit: space before :

@@ -94,1 @@
>    IteratorState *mState;

not worth to remove 'private' keyword, it separates visually private methods from private members

@@ +115,5 @@
>  /**
>   * Allows to traverse through related accessibles that are pointing to the given
>   * dependent accessible by relation attribute.
>   */
> +class RelatedAccIterator: public AccIterable

nit: space before :

@@ -136,1 @@
>  /**

nit: don't remove this extra new line, we keep two empty lines between classes in header files

@@ +154,4 @@
>  /**
>   * Used to iterate through HTML labels associated with the given element.
>   */
> +class HTMLLabelIterator: public AccIterable

nit: space before :

@@ -166,1 @@
>  /**

too

@@ +185,4 @@
>  /**
>   * Used to iterate through HTML outputs associated with the given element.
>   */
> +class HTMLOutputIterator: public AccIterable

nit: space before :

@@ +253,5 @@
>    RelatedAccIterator mRelIter;
>  };
>  
> +/**
> + * Used to iterate through IDs or elements pointed by IDRefs attribute. Note,

IDs, elements or accessibles

@@ +262,5 @@
> +{
> +public:
> +  IDRefsIterator(nsIContent* aContent, nsIAtom* aIDRefsAttr);
> +
> +  virtual ~IDRefsIterator() { }

nit: no new line between constructor and desctuctor

@@ +292,5 @@
> +  nsCOMPtr<nsIDOMElement> mBindingParent;
> +};
> +
> +/**
> + * iterator that points to a single accessible returning it on the first call

iterator -> Iterator

@@ +293,5 @@
> +};
> +
> +/**
> + * iterator that points to a single accessible returning it on the first call
> + * to Next()

nit: dot in the end

@@ +295,5 @@
> +/**
> + * iterator that points to a single accessible returning it on the first call
> + * to Next()
> + */
> +class SingleAccIterator: public AccIterable

nit: space before :

@@ +297,5 @@
> + * to Next()
> + */
> +class SingleAccIterator: public AccIterable
> +{
> +  public:

nit: no indentation for public

@@ +300,5 @@
> +{
> +  public:
> +  SingleAccIterator(nsAccessible* aTarget): mAcc(aTarget) { }
> +
> +  virtual ~SingleAccIterator() { }

no empty line between constructor and desctructor

@@ +308,5 @@
> +    {
> +      nsAccessible* nextAcc = mAcc;
> +      mAcc = nsnull;
> +      return nextAcc;
> +    }

nit: don't define virtuals in headers

@@ +310,5 @@
> +      mAcc = nsnull;
> +      return nextAcc;
> +    }
> +
> +  private:

nit: no indentation for private

@@ +311,5 @@
> +      return nextAcc;
> +    }
> +
> +  private:
> +  nsAccessible* mAcc;

how do you guarantee that accessible doesn't go away before relation iterator is destroyed?

::: accessible/src/atk/nsAccessibleRelationWrap.h
@@ +44,2 @@
>  
> +struct RelationHelper {

nit: { on new line
I prefer to declare it below main Relation class
give short comment why we need it

@@ +52,5 @@
> +  AccIterable* mFirstIter;
> +  AccIterable* mLastIter;
> +};
> +
> +class Relation

comment why is it for

@@ +57,5 @@
> +{
> +public:
> +  Relation() :
> +      mFirstIter(nsnull), mLastIter(nsnull)
> +  { }

nit: put all of this one one line

@@ +66,5 @@
> +    mLastIter = aRelation.mLastIter;
> +  }
> +
> +  Relation&
> +    operator=(RelationHelper& aRH)

ditto

@@ +74,5 @@
> +      return *this;
> +    }
> +
> +  Relation&
> +  operator=(Relation& aRelation)

ditto

@@ +81,5 @@
> +    mLastIter = aRelation.mLastIter;
> +    return *this;
> +  }
> +
> +    operator RelationHelper()

wrong indentation

@@ +87,5 @@
> +      return RelationHelper(mFirstIter.forget(), mLastIter);
> +    }
> +
> +  void
> +  AppendIter(AccIterable* aIter)

on one line

@@ +97,5 @@
> +
> +    mLastIter = aIter;
> +  }
> +
> +  inline void AppendAccessible(nsAccessible* aAcc)

I'd prefer AppendTarget

@@ +98,5 @@
> +    mLastIter = aIter;
> +  }
> +
> +  inline void AppendAccessible(nsAccessible* aAcc)
> +  { AppendIter(new SingleAccIterator(aAcc)); }

two spaced indentation for { Append

@@ +100,5 @@
> +
> +  inline void AppendAccessible(nsAccessible* aAcc)
> +  { AppendIter(new SingleAccIterator(aAcc)); }
> +
> +  inline void AppendAccFromContent(nsIContent* aContent)

AppendTarget too

@@ +101,5 @@
> +  inline void AppendAccessible(nsAccessible* aAcc)
> +  { AppendIter(new SingleAccIterator(aAcc)); }
> +
> +  inline void AppendAccFromContent(nsIContent* aContent)
> +  { AppendAccessible(GetAccService()->GetAccessible(aContent)); }

ditto

@@ +103,5 @@
> +
> +  inline void AppendAccFromContent(nsIContent* aContent)
> +  { AppendAccessible(GetAccService()->GetAccessible(aContent)); }
> +
> +  nsAccessible* Next()

please keep 'inline' keyword for inline methods

@@ +118,5 @@
> +  }
> +
> +private:
> +  nsAutoPtr<AccIterable> mFirstIter;
> +  AccIterable* mLastIter;

honestly I would go with reverse order approach here rather than in follow up. It's nice idea to keep Relation object light as possible

::: accessible/src/base/nsARIAGridAccessible.cpp
@@ +322,5 @@
>  
>    NS_ENSURE_ARG(IsValidColumn(aColumn));
>  
>    AccIterator rowIter(this, filters::GetRow);
> +  nsAccessible *row = rowIter.Next();

nit: nsAccessible* row

@@ +449,5 @@
>        continue;
>      }
>  
>      AccIterator cellIter(row, filters::GetCell);
> +    nsAccessible *cell = cellIter.Next();

nsAccessible* cell

@@ +596,5 @@
>        continue;
>      }
>  
>      AccIterator cellIter(row, filters::GetCell);
> +    nsAccessible *cell = cellIter.Next();

nsAccessible* cell

@@ +761,5 @@
>    PRInt32 rowIdx = aRow;
>  
>    AccIterator rowIter(this, filters::GetRow);
>  
> +  nsAccessible *row = rowIter.Next();

nsAccessible* row

@@ +774,5 @@
>  {
>    PRInt32 colIdx = aColumn;
>  
>    AccIterator cellIter(aRow, filters::GetCell);
> +  nsAccessible *cell = cellIter.Next();

nsAccessible* cell

@@ +863,5 @@
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
>    AccIterator rowIter(this, filters::GetRow);
> +  nsAccessible *row = rowIter.Next();

nsAccessible* row

::: accessible/src/base/nsAccessible.h
@@ +233,5 @@
>    virtual void GetPositionAndSizeInternal(PRInt32 *aPosInSet,
>                                            PRInt32 *aSetSize);
>  
> +  /**
> +   * get the relation of the given type

nit: get -> get, dot in the end

::: accessible/src/base/nsApplicationAccessible.cpp
@@ +174,2 @@
>  {
> +    return Relation();

too many spaces?

::: accessible/src/base/nsApplicationAccessible.h
@@ +120,5 @@
>    virtual PRUint64 State();
>    virtual PRUint64 NativeState();
>    virtual nsAccessible* ChildAtPoint(PRInt32 aX, PRInt32 aY,
>                                       EWhichChildAtPoint aWhichChild);
> +  virtual Relation RelationByType(PRUint32 aType);

nit: relation is property getter, put it before ChildAtPoint

::: accessible/src/base/nsCoreUtils.h
@@ -425,5 @@
> -  nsString mIDs;
> -  nsAString::index_type mCurrIdx;
> -
> -  nsIDocument* mDocument;
> -  nsCOMPtr<nsIDOMDocumentXBL> mXBLDocument;

please remove #include "nsIDOMDocumentXBL.h" from nsCoreUtils.h

::: accessible/src/base/nsDocAccessible.cpp
@@ +35,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +#include "AccIterator.h"

why is it for?

::: accessible/src/base/nsRootAccessible.cpp
@@ +838,2 @@
>  
> +  Relation rel;

nit: define where it's used

@@ +841,5 @@
>      nsCoreUtils::GetDocShellTreeItemFor(mDocument);
>    nsCOMPtr<nsIDocShellTreeItem> contentTreeItem = GetContentDocShell(treeItem);
>    // there may be no content area, so we need a null check
> +  if (!contentTreeItem)
> +    return rel;

return Relation();

::: accessible/src/base/nsTextEquivUtils.cpp
@@ +38,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "nsTextEquivUtils.h"
>  
> +#include "AccIterator.h"

just curious if there's specific reason

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +643,2 @@
>  
> +  // Look for groupbox parent

excess comment, remove it

@@ +643,4 @@
>  
> +  // Look for groupbox parent
> +  nsAccessible* groupbox = GetParent();
> +    // XXX is there any way this group box could be labeled by someone else

absolutely, like aria-labelledby, so remove this comment

::: accessible/src/html/nsHTMLTextAccessible.cpp
@@ +208,1 @@
>    }

nit: wrong indentation

::: accessible/src/msaa/nsAccessibleRelationWrap.h
@@ +49,2 @@
>  
> +class CAccessibleRelation: public IAccessibleRelation

let's prefix class by ia2
space before :

@@ +70,5 @@
>        /* [in] */ long maxTargets,
>        /* [length_is][size_is][out] */ IUnknown **target,
>        /* [retval][out] */ long *nTargets);
>  
> +  virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID aIID, void** aOutPtr);

put IUnknown before IAccessibleRelation

@@ +72,5 @@
>        /* [retval][out] */ long *nTargets);
>  
> +  virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID aIID, void** aOutPtr);
> +  virtual ULONG STDMETHODCALLTYPE AddRef()
> +  { return mReferences++; }

shouldn't you use threadsafe impl similar to NS_IMPL_THREADSAFE_ADDREF

@@ +74,5 @@
> +  virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID aIID, void** aOutPtr);
> +  virtual ULONG STDMETHODCALLTYPE AddRef()
> +  { return mReferences++; }
> +
> +  virtual ULONG STDMETHODCALLTYPE Release()

please put virtuals body into cpp file

::: accessible/src/msaa/Makefile.in
@@ +69,5 @@
>    CAccessibleText.cpp \
>    CAccessibleEditableText.cpp \
>    CAccessibleHyperlink.cpp \
>    CAccessibleHypertext.cpp \
> +	CAccessibleRelation.cpp \

wrong indentation

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +891,5 @@
>  
> +  if (xpRelation) {
> +    Relation rel = RelationByType(xpRelation);
> +    xpAccessibleResult = rel.Next();
> +    NS_ADDREF(xpAccessibleResult);

empty result means crash, but actually this is a leak

@@ +1061,2 @@
>  
> +      *aNRelations = 0;

move it before IsDefunct() check

@@ +1062,5 @@
> +      *aNRelations = 0;
> +  for (PRUint32 relType = nsIAccessibleRelation::RELATION_FIRST; relType < nsIAccessibleRelation::RELATION_LAST; relType++) {
> +    Relation rel = RelationByType(relType);
> +    if (rel.Next())
> +      (*aNRelations)++;

you return target*relation count rather than relation count

::: accessible/src/base/nsAccessibleRelation.h
@@ +20,4 @@
>   * the Initial Developer. All Rights Reserved.
>   *
>   * Contributor(s):
> + *   Trevor Saunders <[e-mail]> (original author)

copied or renamed file shouldn't get new year and replace existing contributors

@@ +59,5 @@
>   */
>  class nsAccessibleRelation: public nsIAccessibleRelation
>  {
>  public:
> +  nsAccessibleRelation(PRUint32 aType, Relation& aRel);

Relation& aRel shouldn't be used (until it's const), use Relation* instead

Comment 34

[alexander :surkov (:asurkov)]

Comment on attachment 547911 [details] [diff] [review]
patch v1

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

::: accessible/src/atk/nsAccessibleRelationWrap.h
@@ +98,5 @@
> +    mLastIter = aIter;
> +  }
> +
> +  inline void AppendAccessible(nsAccessible* aAcc)
> +  { AppendIter(new SingleAccIterator(aAcc)); }

it should have null check since you call it without null check

::: accessible/src/base/nsAccessible.cpp
@@ +86,5 @@
>  #include "nsIScrollableFrame.h"
>  #include "nsFocusManager.h"
>  
> +#include "nsAutoPtr.h"
> +#include "nsTArray.h"

why are these for?

@@ +1513,5 @@
>        state |= states::SELECTED;
>      } else {
>        // If focus is in a child of the tab panel surely the tab is selected!
> +      Relation rel = RelationByType(nsIAccessibleRelation::RELATION_LABEL_FOR);
> +      nsAccessible* tempAcc = nsnull;

tmpAcc -> relTarget

@@ +2008,5 @@
> +
> +Relation
> +nsAccessible::RelationByType(PRUint32 aType)
> +{
> +  Relation rel;

define it where it's used, otherwise it can be confusing since Relation is for specific type but the code below process multiple types.

@@ +2067,2 @@
>  
> +        rel.AppendIter(new SingleAccIterator(groupInfo->ConceptualParent()));

you have shortcut for that: rel.AppendTarget

@@ +2079,5 @@
>          nsIView *view = frame->GetViewExternal();
>          if (view) {
>            nsIScrollableFrame *scrollFrame = do_QueryFrame(frame);
> +          if (scrollFrame || view->GetWidget() || !frame->GetParent())
> +            rel.AppendIter(new SingleAccIterator(GetParent()));

ditto

@@ +2084,5 @@
>          }
>        }
>  
> +      return rel;
> +                                                        }

nit: wrong indentation

@@ +2088,5 @@
> +                                                        }
> +    case nsIAccessibleRelation::RELATION_CONTROLLED_BY:
> +      rel.AppendIter(new RelatedAccIterator(GetDocAccessible(), mContent,
> +                                            nsAccessibilityAtoms::aria_controls));
> +      return rel;

it should be nice to keep Relation(AccIterable*) constructor for this case:
return Relation(new RelatedAccIterator());

@@ +2112,5 @@
>            if (form) {
>              nsCOMPtr<nsIContent> formContent =
>                do_QueryInterface(form->GetDefaultSubmitElement());
> +            rel.AppendAccFromContent(formContent);
> +            return rel;

for these things it might be nice to have Relation(nsIContent*) constructor

@@ +2161,4 @@
>      }
> +    case nsIAccessibleRelation::RELATION_MEMBER_OF:
> +                                                         rel.AppendAccFromContent(GetAtomicRegion());
> +                                                         return rel;

nit: wrong indentation

@@ +2176,5 @@
> +nsAccessible::GetRelations(nsIArray **aRelations)
> +{
> +  NS_ENSURE_ARG_POINTER(aRelations);
> +  nsCOMPtr<nsIMutableArray> relations = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  NS_ENSURE_TRUE(relations, NS_ERROR_OUT_OF_MEMORY);

you don't have null check in nsAccessibleRelation but here you do, what's difference?

@@ +2196,1 @@
>  }

btw, this piece of code shouldn't be changed, it sounds like you moved it in file and remove new lines, please undo this

@@ -2285,3 @@
>  {
>    NS_ENSURE_ARG_POINTER(aCount);
> -  *aCount = 0;

don't remove it

@@ -2289,4 @@
>    nsCOMPtr<nsIArray> relations;
>    nsresult rv = GetRelations(getter_AddRefs(relations));
>    NS_ENSURE_SUCCESS(rv, rv);
> -

ditto

@@ +2214,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  nsCOMPtr<nsIAccessibleRelation> relation = do_QueryElementAt(relations, aIndex, &rv);
> +  NS_ADDREF(*aRelation = relation);
> +  return rv;

it appears no changes are required here, please revert the code

::: accessible/src/base/nsAccessible.h
@@ +654,5 @@
>    /**
>     *  Get the container node for an atomic region, defined by aria-atomic="true"
>     *  @return the container node
>     */
> +  nsIContent* GetAtomicRegion();

make it const please

::: accessible/src/msaa/nsAccessibleRelationWrap.cpp
@@ +51,5 @@
>  {
> +  nsAccessible* target = nsnull;
> +  while ((target = aRel.Next()))
> +      mTargets.AppendElement(target);
> +      }

wrong indentation

@@ -143,5 @@
>        return E_FAIL;
>    }
>  
>    return *aRelationType ? S_OK : E_OUTOFMEMORY;
> -

nit: don't remove empty lines

@@ -154,4 @@
>  {
>  __try {
>    *aLocalizedRelationType = NULL;
> -

ditto

@@ -174,2 @@
>    return S_OK;
> -

don't remove empty line please

@@ +167,5 @@
>  
> +  nsAccessible* target = mTargets[aTargetIndex];
> +  void* nativeTarget = nsnull;
> +  target->GetNativeInterface(&nativeTarget);
> +  NS_ADDREF(*aTarget = static_cast<IUnknown*>(nativeTarget));

use IUnknown::QueryInteraface instead, it casts and addref

@@ -199,2 @@
>    return S_OK;
> -

don't remove empty lines please

@@ +179,1 @@
>                                        long *aNTargets)

nit: wrong indentation

@@ +179,4 @@
>                                        long *aNTargets)
>  {
>  __try {
>    *aNTargets = 0;

please add null checks for arguments

@@ +186,3 @@
>  
> +  for (PRUint32 idx = 0; idx < maxTargets; idx++)
> +    get_target(idx, aTarget + idx);

how does it work on 64bit platform?

@@ -252,2 @@
>    return S_OK;
> -

don't remove empty line

::: accessible/src/msaa/nsAccessibleRelationWrap.h
@@ +86,5 @@
> +  }
> +
> +  inline PRUint32 TargetsCount()
> +  { return mTargets.Length(); }
> +

I prefer IsEmpty() or HasTargets() since it's used this way

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +1074,5 @@
>  nsAccessibleWrap::get_relation(long aRelationIndex,
>                                 IAccessibleRelation **aRelation)
>  {
>  __try {
>    *aRelation = NULL;

you should have IsDefunct() check

@@ +1075,5 @@
>                                 IAccessibleRelation **aRelation)
>  {
>  __try {
>    *aRelation = NULL;
> +  PRUint32 relType = aRelationIndex + nsIAccessibleRelation::RELATION_FIRST;

the behavior is changed, we didn't expose relation when no targets (and of course it doesn't make sense). Relation class could provide IsEmpty() to deal with it or (since it's tricky) you can use AccessibleRelation class like you do below

@@ +1085,2 @@
>  
> +  NS_ADDREF(*aRelation = nativeRelation);

funny to use XPCOM macros for COM

@@ +1095,5 @@
>                                  long *aNRelations)
>  {
>  __try {
>    *aRelation = NULL;
>    *aNRelations = 0;

add IsDefunct() check

@@ +1097,5 @@
>  __try {
>    *aRelation = NULL;
>    *aNRelations = 0;
>  
> +  for (PRUint32 relType = nsIAccessibleRelation::RELATION_FIRST, idx = 0; relType < nsIAccessibleRelation::RELATION_LAST && idx < static_cast<PRUint32>(aMaxRelations); relType++, idx++) {

it looks unreadable

@@ +1100,5 @@
>  
> +  for (PRUint32 relType = nsIAccessibleRelation::RELATION_FIRST, idx = 0; relType < nsIAccessibleRelation::RELATION_LAST && idx < static_cast<PRUint32>(aMaxRelations); relType++, idx++) {
> +    Relation rel = RelationByType(relType);
> +    nsRefPtr<CAccessibleRelation> relation = new CAccessibleRelation(relType,
> +                                                                     rel);

if it exceeds 80 chars restriction then put new CAccessibleRelation on new line

@@ +1107,2 @@
>  
> +    NS_ADDREF(aRelation[idx] = relation);

swap it instead?

::: accessible/src/xpcom/Makefile.in
@@ +48,5 @@
>  LIBXUL_LIBRARY = 1
>  
>  CPPSRCS = \
>    nsAccEvent.cpp \
> +	nsAccessibleRelation.cpp \

wrong indentation

::: accessible/src/base/nsAccessibleRelation.cpp
@@ +20,4 @@
>   * the Initial Developer. All Rights Reserved.
>   *
>   * Contributor(s):
> + *   Trevor Saunders <[e-mail]> (original author)

you shouldn't change year and existing contributors for renamed file

@@ +72,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(aCount);
> +  PRUint32 targets = 0;
> +  nsresult rv = mTargets->GetLength(&targets);
> +  *aCount = targets;

use aCount for mTargets->GetLength()

like return mTargets->GetLenght(aCount);

@@ +84,4 @@
>    nsresult rv = NS_OK;
>    nsCOMPtr<nsIAccessible> target = do_QueryElementAt(mTargets, aIndex, &rv);
> +  NS_ADDREF(*aTarget = target);
> +  return rv;

why is this change for?

::: accessible/src/base/nsAccessibleRelation.h
@@ +71,2 @@
>    nsCOMPtr<nsIMutableArray> mTargets;
> +  PRUint32 mType;

any particular reason to move it?

::: accessible/src/xul/nsXULFormControlAccessible.cpp
@@ +421,5 @@
>  {
>    // XXX: we use the first related accessible only.
> +  nsAccessible* label =
> +    RelationByType(nsIAccessibleRelation::RELATION_LABELLED_BY).Next();
> +      if (label)

wrong indentation

@@ -438,2 @@
>  
> -  if (aRelationType == nsIAccessibleRelation::RELATION_LABELLED_BY) {

if you remove it then please make correct indentation of 'if' body

@@ +444,5 @@
>        if (childAcc->Role() == nsIAccessibleRole::ROLE_LABEL) {
>          // Ensure that it's our label
> +        Relation reverseRel =
> +          childAcc->RelationByType(nsIAccessibleRelation::RELATION_LABEL_FOR);
> +        nsAccessible* testGroupboxAcc = nsnull;

testGroupboxAcc -> testGroupbox

::: accessible/src/xul/nsXULTextAccessible.cpp
@@ -108,3 @@
>  }
>  
> -

please don't remove empty lines

Comment 35

[Trevor Saunders (:tbsaunde)]

Comment on attachment 547911 [details] [diff] [review]
patch v1

i'M STARTING ON V2

Comment 36

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/atk/nsAccessibleWrap.cpp
> @@ +54,2 @@
> >  #include "nsStateMap.h"
> > +#include "Relation.h"
> 
> isn't it more comfortable to keep #include nsIAccessibleRelation.h in
> Relation.h even it's not used there?

I'm not sure, at the end of day I'd like to do the same thing we did for states, and will do for relations, then we wont need nsIAccessibleRelation.h in the first place.

> > +  private:
> > +  nsAccessible* mAcc;
> 
> how do you guarantee that accessible doesn't go away before relation
> iterator is destroyed?

well, its been a while, but iirc we talked about this along time ago, and your view was that we should only have  relations for a short period of time, only within an accessible getter, so it should be imposible for the accessibles to go away so holding references was unneccessary, but I'd be happy to use an nsRefPtr there, although we'll need to figure something  else out if we ever want to stop refcounting accessibles.

> @@ +118,5 @@
> > +  }
> > +
> > +private:
> > +  nsAutoPtr<AccIterable> mFirstIter;
> > +  AccIterable* mLastIter;
> 
> honestly I would go with reverse order approach here rather than in follow
> up. It's nice idea to keep Relation object light as possible

true, I'll consider it.

> >   * ***** END LICENSE BLOCK ***** */
> >  
> > +#include "AccIterator.h"
> 
> why is it for?

the file uses IDRefsIterator which is now defined there instead of nsCoreUtils.h

> ::: accessible/src/base/nsTextEquivUtils.cpp
> @@ +38,5 @@
> >   * ***** END LICENSE BLOCK ***** */
> >  
> >  #include "nsTextEquivUtils.h"
> >  
> > +#include "AccIterator.h"
> 
> just curious if there's specific reason

same IDRefsIterator

> > +class CAccessibleRelation: public IAccessibleRelation
> 
> let's prefix class by ia2

so  ia2AccessibleRelation?

> > +  virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID aIID, void** aOutPtr);
> > +  virtual ULONG STDMETHODCALLTYPE AddRef()
> > +  { return mReferences++; }
> 
> shouldn't you use threadsafe impl similar to NS_IMPL_THREADSAFE_ADDREF

I gues? I'm not sure if it can actually be accessed from multiple threads

Comment 37

[Trevor Saunders (:tbsaunde)]

> > +nsAccessible::GetRelations(nsIArray **aRelations)
> > +{
> > +  NS_ENSURE_ARG_POINTER(aRelations);
> > +  nsCOMPtr<nsIMutableArray> relations = do_CreateInstance(NS_ARRAY_CONTRACTID);
> > +  NS_ENSURE_TRUE(relations, NS_ERROR_OUT_OF_MEMORY);
> 
> you don't have null check in nsAccessibleRelation but here you do, what's
> difference?

I don't think there is accept if I thought I needed the check when writing it.  Can it actually fail? because of a failable alocator or something else?

> @@ -143,5 @@
> >        return E_FAIL;
> >    }
> >  
> >    return *aRelationType ? S_OK : E_OUTOFMEMORY;
> > -
> 
> nit: don't remove empty lines

there are a few too many of them for my taste, but ok.

> > +  for (PRUint32 idx = 0; idx < maxTargets; idx++)
> > +    get_target(idx, aTarget + idx);
> 
> how does it work on 64bit platform?

huh? am I missing something, that's just standard pointer arathmatic

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #36)

> > isn't it more comfortable to keep #include nsIAccessibleRelation.h in
> > Relation.h even it's not used there?
> 
> I'm not sure, at the end of day I'd like to do the same thing we did for
> states, and will do for relations, then we wont need nsIAccessibleRelation.h
> in the first place.

this make sense, but the same things is going to be included or defined in Relation.h? It's now to think about names. Are we going to go with "relations" namespace?

> > how do you guarantee that accessible doesn't go away before relation
> > iterator is destroyed?
> 
> well, its been a while, but iirc we talked about this along time ago, and
> your view was that we should only have  relations for a short period of
> time, only within an accessible getter, so it should be imposible for the
> accessibles to go away so holding references was unneccessary, but I'd be
> happy to use an nsRefPtr there, although we'll need to figure something 
> else out if we ever want to stop refcounting accessibles.

I asked before I reviewed use cases, so there's no problem in your patch. If IA2 is going introduce iterable targets then we should fix it. At this point, it makes sense to use nsRefPtr and check IsDefunct() when returning a target.

> > >   * ***** END LICENSE BLOCK ***** */
> > >  
> > > +#include "AccIterator.h"
> > 
> > why is it for?
> 
> the file uses IDRefsIterator which is now defined there instead of
> nsCoreUtils.h

I'm not sure of context of the question, but I can see in code places where you include "AccIterator.h" and "Relation.h" both and this doesnt' make sense.

> > > +class CAccessibleRelation: public IAccessibleRelation
> > 
> > let's prefix class by ia2
> 
> so  ia2AccessibleRelation?

yeah, are you fine with it?

> > shouldn't you use threadsafe impl similar to NS_IMPL_THREADSAFE_ADDREF
> 
> I gues? I'm not sure if it can actually be accessed from multiple threads

at least from two, from our thread and from caller thread

(In reply to comment #37)
> > > +nsAccessible::GetRelations(nsIArray **aRelations)
> > > +{
> > > +  NS_ENSURE_ARG_POINTER(aRelations);
> > > +  nsCOMPtr<nsIMutableArray> relations = do_CreateInstance(NS_ARRAY_CONTRACTID);
> > > +  NS_ENSURE_TRUE(relations, NS_ERROR_OUT_OF_MEMORY);
> > 
> > you don't have null check in nsAccessibleRelation but here you do, what's
> > difference?
> 
> I don't think there is accept if I thought I needed the check when writing
> it.  Can it actually fail? because of a failable alocator or something else?

I don't have strong opinion, please make sure to ask Neil if we need null check here.

> > how does it work on 64bit platform?
> 
> huh? am I missing something, that's just standard pointer arathmatic

never mind, you're right.

Comment 39

[Trevor Saunders (:tbsaunde)]

(In reply to comment #38)
> (In reply to comment #36)
> 
> > > isn't it more comfortable to keep #include nsIAccessibleRelation.h in
> > > Relation.h even it's not used there?
> > 
> > I'm not sure, at the end of day I'd like to do the same thing we did for
> > states, and will do for relations, then we wont need nsIAccessibleRelation.h
> > in the first place.
> 
> this make sense, but the same things is going to be included or defined in
> Relation.h? It's now to think about names. Are we going to go with
> "relations" namespace?

personally, I like Relation::node_child_of etc, but if you want to use a namespace I can live with that though it seems overkill.

> 
> > > how do you guarantee that accessible doesn't go away before relation
> > > iterator is destroyed?
> > 
> > well, its been a while, but iirc we talked about this along time ago, and
> > your view was that we should only have  relations for a short period of
> > time, only within an accessible getter, so it should be imposible for the
> > accessibles to go away so holding references was unneccessary, but I'd be
> > happy to use an nsRefPtr there, although we'll need to figure something 
> > else out if we ever want to stop refcounting accessibles.
> 
> I asked before I reviewed use cases, so there's no problem in your patch. If
> IA2 is going introduce iterable targets then we should fix it. At this
> point, it makes sense to use nsRefPtr and check IsDefunct() when returning a
> target.

ok, so you're saying I should change to nsRefPtr, and add IsDefunct() check to Next() right?

> > > >   * ***** END LICENSE BLOCK ***** */
> > > >  
> > > > +#include "AccIterator.h"
> > > 
> > > why is it for?
> > 
> > the file uses IDRefsIterator which is now defined there instead of
> > nsCoreUtils.h
> 
> I'm not sure of context of the question, but I can see in code places where
> you include "AccIterator.h" and "Relation.h" both and this doesnt' make
> sense.

In this particular case Relation.h isn't needed just iterators.

in the general case I would gues that AccIterator.h was already included, and I just didn't remove it.  However I not sure it would make sense to remove it since relations and iterators are different concepts, so I'm not convinced its nice to make use of the detail that Relations.h includes AccIterator.h

> > > > +class CAccessibleRelation: public IAccessibleRelation
> > > 
> > > let's prefix class by ia2
> > 
> > so  ia2AccessibleRelation?
> 
> yeah, are you fine with it?

sure

> > > shouldn't you use threadsafe impl similar to NS_IMPL_THREADSAFE_ADDREF
> > 
> > I gues? I'm not sure if it can actually be accessed from multiple threads
> 
> at least from two, from our thread and from caller thread

so the thing is that we create the  object, and after the getter returns, I believe only the getter's caller has a reference  to the object.  (I'm just trying to understand this, I'm fine with the change)

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #39)

> personally, I like Relation::node_child_of etc, but if you want to use a
> namespace I can live with that though it seems overkill.

I'd like to be consistent in constants handling. I'm fine to keep relation constants in Relation object, but then we'd should end up with States object to keep constants and use it as return type.

> ok, so you're saying I should change to nsRefPtr, and add IsDefunct() check
> to Next() right?

I think it makes sense.

> in the general case I would gues that AccIterator.h was already included,
> and I just didn't remove it.  However I not sure it would make sense to
> remove it since relations and iterators are different concepts, so I'm not
> convinced its nice to make use of the detail that Relations.h includes
> AccIterator.h

following this rule we should get big lists of excess includes, in practice we should try to reduce amount of includes to reduce compilation time (iirc there was a bug about this)

> > > > shouldn't you use threadsafe impl similar to NS_IMPL_THREADSAFE_ADDREF
> > > 
> > > I gues? I'm not sure if it can actually be accessed from multiple threads
> > 
> > at least from two, from our thread and from caller thread
> 
> so the thing is that we create the  object, and after the getter returns, I
> believe only the getter's caller has a reference  to the object.  (I'm just
> trying to understand this, I'm fine with the change)

we AddRef the object before we return it, the caller can do with object whatever it wants (at least it must call Release()).

Comment 41

[Trevor Saunders (:tbsaunde)]

(In reply to comment #40)
> (In reply to comment #39)
> 
> > personally, I like Relation::node_child_of etc, but if you want to use a
> > namespace I can live with that though it seems overkill.
> 
> I'd like to be consistent in constants handling. I'm fine to keep relation
> constants in Relation object, but then we'd should end up with States object
> to keep constants and use it as return type.

I'd be fine with that if you mena just typedef PRUint64 States;

> > > > > shouldn't you use threadsafe impl similar to NS_IMPL_THREADSAFE_ADDREF
> > > > 
> > > > I gues? I'm not sure if it can actually be accessed from multiple threads
> > > 
> > > at least from two, from our thread and from caller thread
> > 
> > so the thing is that we create the  object, and after the getter returns, I
> > believe only the getter's caller has a reference  to the object.  (I'm just
> > trying to understand this, I'm fine with the change)
> 
> we AddRef the object before we return it, the caller can do with object
> whatever it wants (at least it must call Release()).

sure, but is there ever a case where after the getter's call gets a reference we would need to addref or release the object?  Since technically we only need to make them thread safe if there is a way for two threads to call them at the same time right?

Comment 42

[Trevor Saunders (:tbsaunde)]

Neil,

> I don't have strong opinion, please make sure to ask Neil if we need null
> check here.

also, thoughts on the atomic refrence counting issue? how would you implement AddRef / Relase with PR_ATOMIC_INCREMENT, or can we just rely  on an msvc version with _InterlockedIncrement()?

Comment 43

[[e-mail]]

You need to use the threadsafe addref/release if the object will be addrefed or released on more than one thread (including creating a threadsafe proxy...)

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> You need to use the threadsafe addref/release if the object will be addrefed
> or released on more than one thread (including creating a threadsafe
> proxy...)

Neil, it's valid if object is addrefed/released on more than on process, right?

Comment 45

[[e-mail]]

(In reply to comment #44)
> Neil, it's valid if object is addrefed/released on more than on process, right?
I have no idea what you mean by that. I don't know how IPDL works.

Comment 46

[alexander :surkov (:asurkov)]

(In reply to comment #45)
> (In reply to comment #44)
> > Neil, it's valid if object is addrefed/released on more than on process, right?
> I have no idea what you mean by that. I don't know how IPDL works.

I meant Firefox and screen reader lives in different processes (excepting dll injection case).

Comment 47

[[e-mail]]

Sorry, I'm not clear on how COM works with threading, so I don't know on which thread COM will try to access your object.

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #47)
> Sorry, I'm not clear on how COM works with threading, so I don't know on
> which thread COM will try to access your object.

Me too unfortunately. Would threadsafe addref/release work in either case?

Jamie, do you know?

Comment 49

[Trevor Saunders (:tbsaunde)]

(In reply to comment #48)
> (In reply to comment #47)
> > Sorry, I'm not clear on how COM works with threading, so I don't know on
> > which thread COM will try to access your object.
> 
> Me too unfortunately. Would threadsafe addref/release work in either case?

yes, but it would be somewhat slower (I don't know exactly how much).
fwiw the AccessibleEnum class in nsAccessibleWrap.cpp uses non-threadsafe AddRef / Release similar to what I used here.

> Jamie, do you know?

ccing jamie for this part)

Comment 50

[Trevor Saunders (:tbsaunde)]

> @@ +118,5 @@
> > +  }
> > +
> > +private:
> > +  nsAutoPtr<AccIterable> mFirstIter;
> > +  AccIterable* mLastIter;
> 
> honestly I would go with reverse order approach here rather than in follow
> up. It's nice idea to keep Relation object light as possible


> @@ +2008,5 @@
> > +
> > +Relation
> > +nsAccessible::RelationByType(PRUint32 aType)
> > +{
> > +  Relation rel;
> 
> define it where it's used, otherwise it can be confusing since Relation is
> for specific type but the code below process multiple types.

I started working on these, but the first couple times I declared the relation object where I needed it (I'm just talking about nsAccessible::RelationByType() not any of the overides) I had to write something like

case foo:
{
  Relation rel;
  if (something(mContent))
    rel.Apend(blah);
  else if (some_otherthing())
    rel.Append(baz);

  rel.Append(otherthing);
  return rel;
}

which  to we what is an uncomfortable amount of extra stuff.  I don't think there's any way around all of this if we get rid of mLastIter which I think is worth to do, but I think for this particular function its worth to declare rel  before the switch statement since it will get rid of several lines that just declare a Relation object for each case.

Comment 51

[alexander :surkov (:asurkov)]

It's ok to keep rel variable declaration inside each case (and iirc I asked to do this in my review). It makes sense because scope of this variable is the case statement, it shouldn't be visible outside - "declare variable where it's used".

Comment 52

[Trevor Saunders (:tbsaunde)]

Attachment: patch v2

Comment 53

[Trevor Saunders (:tbsaunde)]

A couple notes, I didn't include removeing mLastIter or not using nsIAccessibleRelation::RELATION_FOO in this patch,  mostly because I want to do nsIAccessibleRelation::RELATION_FOO as the same time as some of the other big automatic rewrites that would be nice and I'd like to get that done sooner rather than later.  (one of those being nsAccessible *foo -> Accessible* foo which as we  discussed on irc fixes a lot of these nits)

I haven't yet changed the ms COM  AddRef / Release to be thread safe since I see we have a place in the tree already where it isn't, and I'm not convinced we need them to be thread safe.

Comment 54

[alexander :surkov (:asurkov)]

Comment on attachment 549639 [details] [diff] [review]
patch v2

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

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +944,5 @@
>  AtkRelationSet *
>  refRelationSetCB(AtkObject *aAtkObj)
>  {
> +  AtkRelationSet* relation_set =
> +    ATK_OBJECT_CLASS(parent_class)->ref_relation_set(aAtkObj);

I asked for new line, is there any specific reason that my comment wasn't addressed?

@@ +952,2 @@
>  
> +  PRUint32 relationType[] = {

relationType -> relationTypes

@@ +964,5 @@
> +  };
> +
> +  for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(relationType); i++) {
> +    AtkRelationType atkType = static_cast<AtkRelationType>(relationType[i]);
> +    AtkRelation* relation =

relation -> atkRelation

::: accessible/src/base/AccIterator.cpp
@@ +344,5 @@
> +    {
> +      nsAccessible* nextAcc = mAcc;
> +      mAcc = nsnull;
> +      return nextAcc;
> +    }

wrong indentation

::: accessible/src/base/AccIterator.h
@@ +53,5 @@
> +class AccIterable
> +{
> +public:
> +    virtual ~AccIterable() { }
> +    virtual nsAccessible* Next() = 0;

two spaces indentation

@@ +282,5 @@
> +
> +  // AccIterable
> +  virtual nsAccessible* Next();
> +
> +private:

add copy protection

@@ +303,5 @@
> +  virtual ~SingleAccIterator() { }
> +
> +  virtual nsAccessible* Next();
> +
> +private:

add copy protection

::: accessible/src/base/Relation.h
@@ +16,5 @@
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2___

fix year

@@ +43,5 @@
> +#include "AccIterator.h"
> +
> +/**
> + * copy constructing Relations doesn't work because the copyconstructor can't
> + * mutate the old relation, so we use this class as an intermediary

maybe: This is an instrumental class to allow Relation objects to be returned from functions. Copy constructor of Relation class doesn't work in this case because the it's not allowed mutate the copied relation.

@@ +45,5 @@
> +/**
> + * copy constructing Relations doesn't work because the copyconstructor can't
> + * mutate the old relation, so we use this class as an intermediary
> + */
> +struct RelationHelper

maybe RelationCopyHelper?

@@ +59,5 @@
> +};
> +
> +/**
> + * contains a set of accessibles in a list of iterators allowing them to be
> + * lazily enumerated

maybe: A collection or relation targets of certain type. Targets are computed lazily while iterated.

@@ +74,5 @@
> +
> +  Relation(nsAccessible* aAcc)
> +  {
> +    if (aAcc)
> +    mFirstIter = mLastIter = new SingleAccIterator(aAcc);

wrong indetation

@@ +76,5 @@
> +  {
> +    if (aAcc)
> +    mFirstIter = mLastIter = new SingleAccIterator(aAcc);
> +    else
> +      mFirstIter = mLastIter = nsnull;

it's null in either case, preferable to assign then when these members are initialized

btw, maybe reuse AppendTarget?

@@ +85,5 @@
> +  {
> +    AppendTarget(aContent);
> +  }
> +
> +  Relation& operator=(RelationHelper& aRH)

I didn't find anything in coding style, but it appears we wrap around the operator by spaces so Relation& operator = (RelationHelper& aRH);

@@ +129,5 @@
> +
> +  inline nsAccessible* Next()
> +  {
> +    nsAccessible* target = nsnull;
> +    while (mFirstIter && !(target = mFirstIter->Next()) && target->IsDefunct()) {

this means you try next iterator if target is defunct regardless the presence of other targets (that's not a case now but it makes sense to have universal code)

also, if SingleAccIterator is one who keeps the accessible alive then this code will crash since it release it on Next(). I recommend to hide IsDefunct() inside Next() method implementation.

@@ +135,5 @@
> +        mFirstIter = mLastIter = nsnull;
> +      else
> +        mFirstIter = mFirstIter->mNextIter;
> +    }
> +

maybe
while (bla) {
  mFirstIter = mFirstIter->mNextIter;
}

if (!mFirstIter)
  mLastIter = nsnull;

also please add a comment why mFirstIter = mFirstIter->mNextIter works here (comment #28)

::: accessible/src/msaa/ia2AccessibleRelation.cpp
@@ +95,5 @@
> +STDMETHODIMP
> +ia2AccessibleRelation::get_relationType(BSTR *aRelationType)
> +{
> +__try {
> +  *aRelationType = NULL;

null check it

@@ +157,5 @@
> +STDMETHODIMP
> +ia2AccessibleRelation::get_localizedRelationType(BSTR *aLocalizedRelationType)
> +{
> +__try {
> +  *aLocalizedRelationType = NULL;

null check it

@@ +192,5 @@
> +}
> +
> +STDMETHODIMP
> +ia2AccessibleRelation::get_targets(long aMaxTargets, IUnknown **aTarget,
> +                                 long *aNTargets)

wrong indentation

::: accessible/src/msaa/ia2AccessibleRelation.h
@@ +46,5 @@
> +#include "nsTArray.h"
> +
> +#include "AccessibleRelation.h"
> +
> +class ia2AccessibleRelation: public IAccessibleRelation

space before :

@@ +77,5 @@
> +      /* [length_is][size_is][out] */ IUnknown **target,
> +      /* [retval][out] */ long *nTargets);
> +
> +  inline bool HasTargets()
> +  { return mTargets.Length(); }

two spaces indentation for body

@@ +78,5 @@
> +      /* [retval][out] */ long *nTargets);
> +
> +  inline bool HasTargets()
> +  { return mTargets.Length(); }
> +

deny copy constructors

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +1058,2 @@
>  
> +      *aNRelations = 0;

wrong indentation

@@ +1060,5 @@
>  
> +  if (IsDefunct())
> +    return E_FAIL;
> +
> +  for (PRUint32 relType = nsIAccessibleRelation::RELATION_FIRST; relType < nsIAccessibleRelation::RELATION_LAST; relType++) {

align them properly

@@ +1085,3 @@
>      return E_FAIL;
>  
> +  PRUint32 relType = aRelationIndex + nsIAccessibleRelation::RELATION_FIRST;

not addressed from previous review, you can't get relation type from relation index

@@ +1117,5 @@
> +       relType < nsIAccessibleRelation::RELATION_LAST &&
> +       *aNRelations < aMaxRelations; relType++) {
> +    Relation rel = RelationByType(relType);
> +    nsRefPtr<ia2AccessibleRelation> relation = new ia2AccessibleRelation(relType,
> +                                                                     rel);

wrong indentation, prefer to put new on line line
relation -> ia2Rel or ia2Relation

@@ +1121,5 @@
> +                                                                     rel);
> +    if (!relation->HasTargets())
> +      continue;
> +
> +    relation.swap(aRelation+idx);

wrap + by spaces
forget() looks more reasonable here

::: accessible/src/xpcom/nsAccessibleRelation.cpp
@@ +80,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(aTarget);
> +  nsresult rv = NS_OK;
> +  nsCOMPtr<nsIAccessible> target = do_QueryElementAt(mTargets, aIndex, &rv);
> +  NS_ADDREF(*aTarget = target);

not addressed from previous review, use forget

::: accessible/src/xpcom/nsAccessibleRelation.h
@@ +64,5 @@
> +
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIACCESSIBLERELATION
> +
> +  NS_DECLARE_STATIC_IID_ACCESSOR(NS_ACCRELATION_IMPL_CID)

do you still need to nsAccessibleRelation refid for something? looks like artifact

Comment 55

[Trevor Saunders (:tbsaunde)]

(In reply to comment #54)
> Comment on attachment 549639 [details] [diff] [review] [diff] [details] [review]
> patch v2
> 
> Review of attachment 549639 [details] [diff] [review] [diff] [details] [review]:

> @@ +944,5 @@
> >  AtkRelationSet *
> >  refRelationSetCB(AtkObject *aAtkObj)
> >  {
> > +  AtkRelationSet* relation_set =
> > +    ATK_OBJECT_CLASS(parent_class)->ref_relation_set(aAtkObj);
> 
> I asked for new line, is there any specific reason that my comment wasn't
> addressed?

sorry, meant to poke you about this and the msaa thing but forgot to do it before attaching this patch.  Its not clear to me from the contet where exactly you want a  blank line.

> add copy protection

for  Relation as well as the ones you mention? especially since an actually copy there is bad.

> @@ +76,5 @@
> > +  {
> > +    if (aAcc)
> > +    mFirstIter = mLastIter = new SingleAccIterator(aAcc);
> > +    else
> > +      mFirstIter = mLastIter = nsnull;
> 
> it's null in either case, preferable to assign then when these members are
> initialized

huh? mind rephrasing?

> > +  Relation& operator=(RelationHelper& aRH)
> 
> I didn't find anything in coding style, but it appears we wrap around the
> operator by spaces so Relation& operator = (RelationHelper& aRH);

ok, nsAutoPtr.h seems to use operator=( blah and I  think I just used that but I'm fine with changing to operator = (blah)

> @@ +129,5 @@
> > +
> > +  inline nsAccessible* Next()
> > +  {
> > +    nsAccessible* target = nsnull;
> > +    while (mFirstIter && !(target = mFirstIter->Next()) && target->IsDefunct()) {
> 
> this means you try next iterator if target is defunct regardless the
> presence of other targets (that's not a case now but it makes sense to have
> universal code)
> 
> also, if SingleAccIterator is one who keeps the accessible alive then this
> code will crash since it release it on Next(). I recommend to hide
> IsDefunct() inside Next() method implementation.

sure, btw I think cycle collection would possibly save you here since there's always a cycle between an accessible and its parent  if has one.


> while (bla) {
>   mFirstIter = mFirstIter->mNextIter;
> }

I assume you don't actually want braces since it becomes a single statement?

> > +ia2AccessibleRelation::get_targets(long aMaxTargets, IUnknown **aTarget,
> > +                                 long *aNTargets)
> 
> wrong indentation

yeah, from find and replace CAccessibleRelation -> ia2AccessibleRelation, thanks for the catch

> @@ +1085,3 @@
> >      return E_FAIL;
> >  
> > +  PRUint32 relType = aRelationIndex + nsIAccessibleRelation::RELATION_FIRST;
> 
> not addressed from previous review, you can't get relation type from
> relation index

yeah, as I said meant to poke you, sorry.  Do you have a better idea on how to implement this than get relations until we've run out of times  or we've gotten enough relations with targets to  be at the index requested?

> > ::: accessible/src/xpcom/nsAccessibleRelation.h
> @@ +64,5 @@
> > +
> > +  NS_DECL_ISUPPORTS
> > +  NS_DECL_NSIACCESSIBLERELATION
> > +
> > +  NS_DECLARE_STATIC_IID_ACCESSOR(NS_ACCRELATION_IMPL_CID)
> 
> do you still need to nsAccessibleRelation refid for something? looks like
> artifact

It seems like an artifact, but I'm not sure what macros we want here, I don't seem to seem macros in nsISupportsImpl.h to declare   query interface that can only qi to nsAccessibleRelation

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #55)

> > @@ +944,5 @@
> > >  AtkRelationSet *
> > >  refRelationSetCB(AtkObject *aAtkObj)
> > >  {
> > > +  AtkRelationSet* relation_set =
> > > +    ATK_OBJECT_CLASS(parent_class)->ref_relation_set(aAtkObj);
> > 
> > I asked for new line, is there any specific reason that my comment wasn't
> > addressed?
> 
> sorry, meant to poke you about this and the msaa thing but forgot to do it
> before attaching this patch.  Its not clear to me from the contet where
> exactly you want a  blank line.

ok, I see, right after the mentioned line. It is in sources right now, your patch removes it.

> > add copy protection
> 
> for  Relation as well as the ones you mention? especially since an actually
> copy there is bad.

I don't think compiler will generate default copy constructors in case of Relation class but to be on safe side it make sense to deny them.

> > @@ +76,5 @@
> > > +  {
> > > +    if (aAcc)
> > > +    mFirstIter = mLastIter = new SingleAccIterator(aAcc);
> > > +    else
> > > +      mFirstIter = mLastIter = nsnull;
> > 
> > it's null in either case, preferable to assign then when these members are
> > initialized
> 
> huh? mind rephrasing?

I meant to have something like

Constructor(aAcc) : mFirstIter(nsnull), mLastIter(nsnull)
{
  if (aAcc)
    mFirstIter = mLastIter = new SingleAccIterator(aAcc);
}

but you can use AppendTarget

> > > +  Relation& operator=(RelationHelper& aRH)
> > 
> > I didn't find anything in coding style, but it appears we wrap around the
> > operator by spaces so Relation& operator = (RelationHelper& aRH);
> 
> ok, nsAutoPtr.h seems to use operator=( blah and I  think I just used that
> but I'm fine with changing to operator = (blah)

I meant a11y module

> > @@ +129,5 @@
> > > +
> > > +  inline nsAccessible* Next()
> > > +  {
> > > +    nsAccessible* target = nsnull;
> > > +    while (mFirstIter && !(target = mFirstIter->Next()) && target->IsDefunct()) {
> > 
> > this means you try next iterator if target is defunct regardless the
> > presence of other targets (that's not a case now but it makes sense to have
> > universal code)
> > 
> > also, if SingleAccIterator is one who keeps the accessible alive then this
> > code will crash since it release it on Next(). I recommend to hide
> > IsDefunct() inside Next() method implementation.
> 
> sure, btw I think cycle collection would possibly save you here since
> there's always a cycle between an accessible and its parent  if has one.

for defunct nodes there's no cycle between accessible and its parent

> > while (bla) {
> >   mFirstIter = mFirstIter->mNextIter;
> > }
> 
> I assume you don't actually want braces since it becomes a single statement?

sure
 
> > @@ +1085,3 @@
> > >      return E_FAIL;
> > >  
> > > +  PRUint32 relType = aRelationIndex + nsIAccessibleRelation::RELATION_FIRST;
> > 
> > not addressed from previous review, you can't get relation type from
> > relation index
> 
> yeah, as I said meant to poke you, sorry.  Do you have a better idea on how
> to implement this than get relations until we've run out of times  or we've
> gotten enough relations with targets to  be at the index requested?

no, this is idea, hope IA2 will come soon with better API based on iterations and relations by type

> 
> > > ::: accessible/src/xpcom/nsAccessibleRelation.h
> > @@ +64,5 @@
> > > +
> > > +  NS_DECL_ISUPPORTS
> > > +  NS_DECL_NSIACCESSIBLERELATION
> > > +
> > > +  NS_DECLARE_STATIC_IID_ACCESSOR(NS_ACCRELATION_IMPL_CID)
> > 
> > do you still need to nsAccessibleRelation refid for something? looks like
> > artifact
> 
> It seems like an artifact, but I'm not sure what macros we want here, I
> don't seem to seem macros in nsISupportsImpl.h to declare   query interface
> that can only qi to nsAccessibleRelation

if we don't need it then remove entries of NS_ACCRELATION_IMPL_CID and fix NS_IMPL_ISUPPORTS2.

btw, any specific reason that you create new nsAccessibleRelation.h/cpp and ia2AccessibleRelation.h/cpp files rather than copy existing ones? I think it makes sense to keep history since basically they are quite similar.

Comment 57

[Trevor Saunders (:tbsaunde)]

> ok, I see, right after the mentioned line. It is in sources right now, your
> patch removes it.

I see, seems like an odd place to me, but *shrug* :)

> I meant to have something like

ok, thanks

> btw, any specific reason that you create new nsAccessibleRelation.h/cpp and
> ia2AccessibleRelation.h/cpp files rather than copy existing ones? I think it
> makes sense to keep history since basically they are quite similar.

I think it happened because I actually rewrote both of them even if I could have done something different.  However from looking at some diffing it looks like the only thing diff really identifies as not having changed is the license boiler plate, so it doesn't really seem to matter much I think.

Comment 58

[alexander :surkov (:asurkov)]

(In reply to comment #57)

> I think it happened because I actually rewrote both of them even if I could
> have done something different.  However from looking at some diffing it
> looks like the only thing diff really identifies as not having changed is
> the license boiler plate, so it doesn't really seem to matter much I think.

for example, looking at old and new nsAccessibleRelation.h/cpp they should be mostly identical, the diff shouldn't be so stupid to not show this and if it is then I assume that can be fixed by tweaking the code. Can you show a version where you move the files rather than remove and create new ones?

Comment 59

[Trevor Saunders (:tbsaunde)]

Attachment: patch v3

I think I changed the xpcom stuff related to nsAccessibleRelation  correctly, but I'm not really sure...

Comment 60

[alexander :surkov (:asurkov)]

Comment on attachment 550030 [details] [diff] [review]
patch v3

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

r=me with comments fixed

::: accessible/src/base/AccIterator.cpp
@@ +343,5 @@
> +SingleAccIterator::Next()
> +{
> +  nsRefPtr<nsAccessible> nextAcc;
> +  mAcc.swap(nextAcc);
> +  return (nextAcc && !nextAcc->IsDefunct()) ? nextAcc : nsnull;

leak

maybe:
if (mAcc) {
  nsAccessible* acc = mAcc->IsDefunct() ? nsnull : mAcc;
  mAcc = nsnull;
  return acc;
}
return nsnull;

::: accessible/src/base/Relation.h
@@ +42,5 @@
> +
> +#include "AccIterator.h"
> +
> +/**
> + * this class is used to return relations from functions.  A copy constructor

this -> This
relations -> Relation object

@@ +52,5 @@
> +  RelationCopyHelper(AccIterable* aFirstIter, AccIterable* aLastIter)
> +  {
> +    mFirstIter = aFirstIter;
> +    mLastIter = aLastIter;
> +  }

RelationCopyHelper(AccIterable* aFirstIter, AccIterable* aLastIter) : mFirstIter(aFirstIter), mLastIter(aLastIter)
{
}

@@ +85,5 @@
> +    {
> +      mFirstIter = aRH.mFirstIter;
> +      mLastIter = aRH.mLastIter;
> +      return *this;
> +    }

no indentation of { } please

@@ +127,5 @@
> +    nsAccessible* target = nsnull;
> +
> +    // a trick nsAutoPtr deletes what it used to point to when assigned to
> +    while (mFirstIter && !(target = mFirstIter->Next()))
> +        mFirstIter = mFirstIter->mNextIter;

wrong indentation

@@ +133,5 @@
> +    if (!mFirstIter)
> +      mLastIter = nsnull;
> +
> +    return target;
> +  }

please comment AppendTarget and Next() methods

::: accessible/src/msaa/nsAccessibleRelationWrap.h
@@ +51,3 @@
>  {
>  public:
> +  ia2AccessibleRelation(PRUint32 aType, Relation& aRel);

Relation* aRel

@@ +77,4 @@
>        /* [length_is][size_is][out] */ IUnknown **target,
>        /* [retval][out] */ long *nTargets);
>  
> +  inline bool HasTargets()

const

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +1099,2 @@
>  
> +    relIdx++;

wrong indentation

@@ +1118,1 @@
>    *aRelation = NULL;

not what you introduced, but it doesn't make big sense to null out the first array item

@@ +1130,5 @@
> +    if (!relation->HasTargets())
> +      continue;
> +
> +    relation.forget(aRelation + idx);
> +    (*aNRelations)++;

I prefer to put these into if block
if (relation->HasTargets()) {
}

::: accessible/src/base/nsAccessibleRelation.h
@@ +60,1 @@
>  private:

add copy protection please

@@ +61,2 @@
>    nsCOMPtr<nsIMutableArray> mTargets;
> +  PRUint32 mType;

any objection to move mType?

Comment 61

[alexander :surkov (:asurkov)]

Comment on attachment 550030 [details] [diff] [review]
patch v3

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

::: accessible/src/base/AccIterator.cpp
@@ +343,5 @@
> +SingleAccIterator::Next()
> +{
> +  nsRefPtr<nsAccessible> nextAcc;
> +  mAcc.swap(nextAcc);
> +  return (nextAcc && !nextAcc->IsDefunct()) ? nextAcc : nsnull;

no leak, you're right, though we don't need extra addref/release

Comment 62

[Trevor Saunders (:tbsaunde)]

(In reply to comment #61)
> Comment on attachment 550030 [details] [diff] [review] [diff] [details] [review]
> patch v3
> 
> Review of attachment 550030 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/AccIterator.cpp
> @@ +343,5 @@
> > +SingleAccIterator::Next()
> > +{
> > +  nsRefPtr<nsAccessible> nextAcc;
> > +  mAcc.swap(nextAcc);
> > +  return (nextAcc && !nextAcc->IsDefunct()) ? nextAcc : nsnull;
> 
> no leak, you're right, though we don't need extra addref/release

uh, nsRefPtr.swap() doesn't AddRef / Release, not sure what you talk about here.

> @@ +1118,1 @@
> >    *aRelation = NULL;
> 
> not what you introduced, but it doesn't make big sense to null out the first
> array item

should we memset the whole array?

> @@ +61,2 @@
> >    nsCOMPtr<nsIMutableArray> mTargets;
> > +  PRUint32 mType;
> 
> any objection to move mType?

not really, just habit to put bigger things first for alignment.  (64 bit pointer), but in this case I don't think it atters.

Comment 63

[alexander :surkov (:asurkov)]

(In reply to comment #62)
> (In reply to comment #61)
> > Comment on attachment 550030 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch v3
> > 
> > Review of attachment 550030 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/src/base/AccIterator.cpp
> > @@ +343,5 @@
> > > +SingleAccIterator::Next()
> > > +{
> > > +  nsRefPtr<nsAccessible> nextAcc;
> > > +  mAcc.swap(nextAcc);
> > > +  return (nextAcc && !nextAcc->IsDefunct()) ? nextAcc : nsnull;
> > 
> > no leak, you're right, though we don't need extra addref/release

you're right, ignore it, though we maybe don't need to have extra object :)

> > not what you introduced, but it doesn't make big sense to null out the first
> > array item
> 
> should we memset the whole array?

I think just ignore it. They should rely on number value instead, anyway it leads to bad things if something was in array.

Comment 64

[[e-mail]]

I don't understand where the RelationCopyHelper fits in...

[I happened to notice at least one mis-indentation.]

Comment 65

[James Teh [:Jamie]]

(In reply to comment #48)
> > Sorry, I'm not clear on how COM works with threading, so I don't know on
> > which thread COM will try to access your object.
> Jamie, do you know?
Sorry, I somehow missed this in the email swarm. :( If the thread hosting the accessibles is a single threaded apartment (STA) thread (and I suspect it is), then methods will always be called in that thread by COM, so you don't need to worry about cross-thread calls. If it's an MTA, calls could happen in any thread.

It is an STA thread if you initialise COM with CoInitialize() or CoInitializeEx() with the COINIT_APARTMENTTHREADED flag.

Comment 66

[Trevor Saunders (:tbsaunde)]

(In reply to comment #64)
> I don't understand where the RelationCopyHelper fits in...

It's a bit of a trick see http://stackoverflow.com/questions/2121844/what-is-auto-ptr-ref-what-it-achieves-and-how-it-achieves-it
basically when a Relation is returned instead of  going through a copy constructor a RelationCopyhelper is created, and copied through it.

> [I happened to notice at least one mis-indentation.]

arg, other than what Alex already found?

Comment 67

[[e-mail]]

(In reply to comment #66)
> (In reply to comment #64)
> > I don't understand where the RelationCopyHelper fits in...
> 
> It's a bit of a trick see
> http://stackoverflow.com/questions/2121844/what-is-auto-ptr-ref-what-it-
> achieves-and-how-it-achieves-it
> basically when a Relation is returned instead of  going through a copy
> constructor a RelationCopyhelper is created, and copied through it.
Aha, very sneaky, so the compiler wants to construct a Relation from another Relation, but normally you can't do that if the source relation is const (usually because it's a temporary of some sort) but it can invoke the conversion operator on the source relation so by the time the temporary helper becomes const the pointer has already been forgotten by the source relation.

> > [I happened to notice at least one mis-indentation.]
> 
> arg, other than what Alex already found?

Oh, I think he found that one after all, I just didn't look back far enough.

Comment 68

[alexander :surkov (:asurkov)]

(In reply to comment #67)

> Aha, very sneaky, so the compiler wants to construct a Relation from another
> Relation, but normally you can't do that if the source relation is const
> (usually because it's a temporary of some sort) but it can invoke the
> conversion operator on the source relation so by the time the temporary
> helper becomes const the pointer has already been forgotten by the source
> relation.

That's right. STL is quite good for inspiration :)

Comment 69

[[e-mail]]

Comment on attachment 550030 [details] [diff] [review]
patch v3

Oh, some very minor queries:

>+  Relation(RelationCopyHelper aRelation) :
const RelationCopyHelper or maybe const RelationCopyHelper&?

>+  Relation& operator = (RelationCopyHelper& aRH)
const RelationCopyHelper&?

>+  inline void AppendIter(AccIterable* aIter)
Shouldn't need an explicit inline here, as you're providing the body.

By the way, I did think of a scheme which I think looks slightly less awkward than the RelationCopyHelper. It works something like this:

class RelationBuilder {
  nsAutoPtr<AccIterable> mFirstIter;
  AccIterable* mLastIter;
public:
  void AppendIter(AccIterable* aIter);
  /* other append methods */
  operator AccIterable*() { return mFirstIter.forget(); }
}

class Relation {
  nsAutoPtr<AccIterable> mIter;
public:
  Relation(AccIterable* aIter) : mIter(aIter) {}
  /* enumeration methods */
}

Relation rel = RelationByType(nsIAccessibleRelation::RELATION_LABEL_FOR);

virtual AccIterable* RelationByType(PRUint32 aType);

AccIterable* 
nsXULTreeItemAccessibleBase::RelationByType(PRUint32 aType)
{
  if (aType != nsIAccessibleRelation::RELATION_NODE_CHILD_OF)
    return nsAccessible::RelationByType(aType);

  RelationBuilder rel;
  if (!NS_SUCCEEDED(mTreeView->GetParentIndex(mRow, &parentIndex)))
    return rel;

  if (parentIndex == -1) {
    rel.AppendTarget(mParent);
    return rel;
  }
  nsRefPtr<nsXULTreeAccessible> treeAcc = do_QueryObject(mParent);

  rel.AppendTarget(treeAcc->GetTreeItemAccessible(parentIndex));
  return rel;
}

Comment 70

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #69)
> Comment on attachment 550030 [details] [diff] [review] [diff] [details] [review]
> patch v3
> 
> Oh, some very minor queries:
> 
> >+  Relation(RelationCopyHelper aRelation) :
> const RelationCopyHelper or maybe const RelationCopyHelper&?

sure

> >+  Relation& operator = (RelationCopyHelper& aRH)
> const RelationCopyHelper&?

sure

> >+  inline void AppendIter(AccIterable* aIter)
> Shouldn't need an explicit inline here, as you're providing the body.

yeah, I think Alex said he prefers explicit inline keyword in a comment somewhere here, but I'm fine either way.

> By the way, I did think of a scheme which I think looks slightly less
> awkward than the RelationCopyHelper. It works something like this:
> 
> class RelationBuilder {
>   nsAutoPtr<AccIterable> mFirstIter;
>   AccIterable* mLastIter;
> public:
>   void AppendIter(AccIterable* aIter);
>   /* other append methods */
>   operator AccIterable*() { return mFirstIter.forget(); }
> }
> 
> class Relation {
>   nsAutoPtr<AccIterable> mIter;
> public:
>   Relation(AccIterable* aIter) : mIter(aIter) {}
>   /* enumeration methods */
> }
> 
> Relation rel = RelationByType(nsIAccessibleRelation::RELATION_LABEL_FOR);
> 
> virtual AccIterable* RelationByType(PRUint32 aType);
> 
> AccIterable* 
> nsXULTreeItemAccessibleBase::RelationByType(PRUint32 aType)
> {
>   if (aType != nsIAccessibleRelation::RELATION_NODE_CHILD_OF)
>     return nsAccessible::RelationByType(aType);
> 
>   RelationBuilder rel;
>   if (!NS_SUCCEEDED(mTreeView->GetParentIndex(mRow, &parentIndex)))
>     return rel;
> 
>   if (parentIndex == -1) {
>     rel.AppendTarget(mParent);
>     return rel;
>   }
>   nsRefPtr<nsXULTreeAccessible> treeAcc = do_QueryObject(mParent);
> 
>   rel.AppendTarget(treeAcc->GetTreeItemAccessible(parentIndex));
>   return rel;
> }

It seems to me like this scheme is a little more complicated.  On the other hand it does  help with the fact that Relation is currently 2 pointers which means it can't be returned in a register.

How would you propose dealing with a situation like this using that scheme

foo::RelationByType(aType)
{
  Relation rel = RelationByType(aType);
  if (blah)
    rel.Append(something);

  return rel;
}  The trick here being that returning the relation seems to loose the pointer to the end of the list.


So, I think it makes sense to land what we have with these comments addressed  so that some other work blocked on this can go forward, and either leave this bug open or file a second to improve this and possibly get rid of mLastIter in Relation.

Comment 71

[[e-mail]]

(In reply to Trevor Saunders (:tbsaunde) from comment #70)
> I think Alex said he prefers explicit inline keyword
Fair enough.

> How would you propose dealing with a situation like this using that scheme
I skimmed the patch but I didn't notice you doing that. Sorry about that.
I'd need to see how often you do that to see whether it's even worth while.

Comment 72

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #71)
> > How would you propose dealing with a situation like this using that scheme
> I skimmed the patch but I didn't notice you doing that. Sorry about that.
> I'd need to see how often you do that to see whether it's even worth while.

np, probably no more than 5 or 6, but I'm not sure off hand how often those cases happen in actua use.

Comment 73

[alexander :surkov (:asurkov)]

(In reply to [e-mail] from comment #71)
> (In reply to Trevor Saunders (:tbsaunde) from comment #70)
> > I think Alex said he prefers explicit inline keyword
> Fair enough.
?

I like STL approach because it allows to hide the workaround for the problem completely from eyes, while RelationBuilder approach makes developer to wonder what is it for and gives a little bit more complicated Relation usage. If STL approach doesn't have hidden or potential problems then I would stay on it.

Comment 74

[alexander :surkov (:asurkov)]

inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/1140e223915f

Comment 75

[Trevor Saunders (:tbsaunde)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1140e223915f

Comment 76

[alexander :surkov (:asurkov)]

landed on central http://hg.mozilla.org/mozilla-central/rev/1140e223915f