648265 - provide dexcomed table interface version

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Add TableAccessible abstract class equivalent to nsIAccessibleTable interface and make table classes implement it.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: add a table accessible interface class

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: make xul list box inherit from TableAccessible

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: TableAccessible should have a virtual destructor

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: add a Caption() method to the accessible table interface and make xul list box implement it

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: add Summary() to table interface and implement on nsXULListboxAccessible

Comment 7

[Trevor Saunders (:tbsaunde)]

Attachment: RowCount()

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: CellIndexAt()

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: ColumnIndexAt()

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: RowIndexAt()

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: RowAndColAtIndex()

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: RowExtentAt() and ColExtentAt()

Comment 13

[Trevor Saunders (:tbsaunde)]

Attachment: Row / Col Description()

Comment 14

[Trevor Saunders (:tbsaunde)]

Attachment: Is{Cell,Col,Row}Selected()

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: IsProbablyForLayout()

Comment 16

[Trevor Saunders (:tbsaunde)]

Attachment: [Un]Select{Row,Col}()

Comment 17

[Trevor Saunders (:tbsaunde)]

Attachment: Selected{Cell,Col,Row}Count()

Comment 18

[Trevor Saunders (:tbsaunde)]

Attachment: kill GetSelectedCellIndicies(0 and move GetSelectedCells() to SelectedCells()

Comment 19

[Trevor Saunders (:tbsaunde)]

Attachment: GetSelected{Column,Row}Indices() -> SelectedFooIndices()

Comment 20

[Trevor Saunders (:tbsaunde)]

Attachment: cleanup IsMultyColumn() and make it inline

Comment 21

[Trevor Saunders (:tbsaunde)]

Attachment: bustage fix use GetChildAt(idx) instead of GetChildAt(idx, outAcc) in nsXULListboxAccessible::CellAt()

Comment 22

[Trevor Saunders (:tbsaunde)]

Attachment: improve assertions, some cleanup related to making every index unsigned still remains

Comment 23

[Trevor Saunders (:tbsaunde)]

Attachment: bustage fix always use ColCount() and SelectedColCOunt() not ColumnCount()

Comment 24

[Trevor Saunders (:tbsaunde)]

Attachment: always use unsigned integers for indices

Comment 25

[alexander :surkov (:asurkov)]

Looks ok (not digging into details) but I bet we will be ruined up by comments if I start review. I suggest organizing:
1) TableAccessible class, including downcasting stuff support in nsAccessible
2) TableAccessible implementation for
2.1) xul listbox
2.2) xul trees 
2.3) aria grids
2.4) html tables
3) replace nsIAccessibleTable usage on TableAccessible

sounds ok?

Comment 26

[alexander :surkov (:asurkov)]

If you're fine with approach then I can do review of all these patches to avoid extra work on your side.

Comment 27

[alexander :surkov (:asurkov)]

Comment on attachment 544694 [details] [diff] [review]
add a table accessible interface class

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

r=me with comments fixed

::: accessible/src/generic/TableAccessible.h
@@ +12,5 @@
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is __________________________________________.

mozilla.org code

@@ +15,5 @@
> + *
> + * The Original Code is __________________________________________.
> + *
> + * The Initial Developer of the Original Code is
> + * ____________________________________________.

Mozilla Foundation

@@ +16,5 @@
> + * The Original Code is __________________________________________.
> + *
> + * The Initial Developer of the Original Code is
> + * ____________________________________________.
> + * Portions created by the Initial Developer are Copyright (C) 2___

2011

@@ +19,5 @@
> + * ____________________________________________.
> + * Portions created by the Initial Developer are Copyright (C) 2___
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):

Your name <youremail> (original author)

@@ +42,5 @@
> +class TableAccessible
> +{
> +
> +  private:
> +    // only classes inheriting from TableAccessible should be instantiated

the comment is not necessary, it's going to be abstract class, you can't instantiate it
note: i'm fine to keep private constructors but comment doesn't make sense
maybe it doesn't matter because the class is pure abstract but I think I prefer to keep them protected rather than private

Comment 28

[alexander :surkov (:asurkov)]

Comment on attachment 544695 [details] [diff] [review]
make xul list box inherit from TableAccessible

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

::: accessible/src/xul/nsXULListboxAccessible.h
@@ +44,5 @@
>  
>  #include "nsCOMPtr.h"
>  #include "nsXULMenuAccessible.h"
>  #include "nsBaseWidgetAccessible.h"
> +#include "TableAccessible.h"

keep accessible files separately from others, put it after "nsIAccessibleTable.h" separated by empty line please

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 544696 [details] [diff] [review]
TableAccessible should have a virtual destructor

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

::: accessible/src/generic/TableAccessible.h
@@ +40,5 @@
>  #define TABLE_ACCESSIBLE_H
>  
>  class TableAccessible
>  {
> +  public:

no two spaces for public and for private btw

Comment 30

[alexander :surkov (:asurkov)]

Comment on attachment 544697 [details] [diff] [review]
add a Caption() method to the accessible table interface and make xul list box implement it

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

::: accessible/src/generic/TableAccessible.h
@@ +44,5 @@
>    public:
>    virtual ~TableAccessible() { };
>  
> +  /**
> +   * get the caption accessible if any for this table.

get -> Return

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 544698 [details] [diff] [review]
add Summary() to table interface and implement on nsXULListboxAccessible

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

::: accessible/src/generic/TableAccessible.h
@@ +47,5 @@
>    /**
>     * get the caption accessible if any for this table.
>     */
>    virtual nsAccessible* Caption() = 0;
> +  virtual void Summary(nsString& aSummary) = 0;

comment please

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +226,5 @@
>  
> +void
> +nsXULListboxAccessible::Summary(nsString& aSummary)
> +{
> +  aSummary.SetIsVoid();

Use Truncate instead. SetIsVoid is different from Truncate and used for specific proposes

@@ +244,5 @@
>  
>  NS_IMETHODIMP
>  nsXULListboxAccessible::GetSummary(nsAString &aSummary)
>  {
> +  nsString summary;

nsAutoString

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 544699 [details] [diff] [review]
RowCount()

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

::: accessible/src/generic/TableAccessible.h
@@ +48,5 @@
>     * get the caption accessible if any for this table.
>     */
>    virtual nsAccessible* Caption() = 0;
> +  virtual PRUint32 ColumnCount() = 0;
> +  virtual PRUint32 RowCount();

comment it please

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +234,5 @@
> +nsXULListboxAccessible::ColumnCount()
> +{
> +  nsIContent* headContent = nsnull;
> +  PRUint32 count = mContent->GetChildCount();
> +  // XXX why does this loop look at all the content children but only care about the last list one?

it makes sense to crawl by accessible tree looking for the first one, please make sure we have mochitest for this

@@ +264,5 @@
> +  nsCOMPtr<nsIDOMXULSelectControlElement> element(do_QueryInterface(mContent));
> +  NS_ENSURE_TRUE(element, 0);
> +
> +  PRUint32 itemCount = 0;
> +  return NS_FAILED(element->GetItemCount(&itemCount)) ? 0 : itemCount;

I'd avoid XBL usage where we can, maybe group attrbiutes can be used for that?

@@ +289,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsXULListboxAccessible::GetColumnCount(PRInt32* aColumnsCount)

aColumnsCount -> aColumnCount

@@ +294,3 @@
>  {
>    NS_ENSURE_ARG_POINTER(aColumnsCout);
>    *aColumnsCout = 0;

compilation error

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 544700 [details] [diff] [review]
CellIndexAt()

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

::: accessible/src/generic/TableAccessible.h
@@ +48,5 @@
>     * get the caption accessible if any for this table.
>     */
>    virtual nsAccessible* Caption() = 0;
> +  virtual nsAccessible* CellAt(PRUint32 aRow, PRUint32 aCol);
> +  virtual PRUint32 CellIndexAt(PRUint32 aRow, PRUint32 aCell);

comment please

I'd prefer to group them logically rather than by name, the order in nsIAccessibleTable is worth to follow

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +268,5 @@
>    return NS_FAILED(element->GetItemCount(&itemCount)) ? 0 : itemCount;
>  }
>  
> +nsAccessible*
> +nsXULListboxAccessible::CellAt(PRUint32 aROw, PRUint32 aCol)

aROw -> aRowIdx, aCol -> aColIdx

@@ -312,5 @@
> -    return NS_ERROR_FAILURE;
> -
> -  *aRowCount = RowCount();
> -  return NS_OK;
> -}

why did you remove all of them?

@@ -316,5 @@
> -}
> -
> -NS_IMETHODIMP
> -nsXULListboxAccessible::GetCellAt(PRInt32 aRow, PRInt32 aColumn,
> -                                  nsIAccessible **aAccessibleCell)

where's XPCOM implementation supposed to be?

Comment 34

[alexander :surkov (:asurkov)]

Comment on attachment 544714 [details] [diff] [review]
improve assertions, some cleanup related to making every index unsigned still remains

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

do we really need them? For outside usage we should have a check so assertions won't ever hit (not taking into account assertions are not expected by consumer). If these methods are used internally with wrong arguments then maybe it's ok, if you think it brings new level of protection.

Comment 35

[alexander :surkov (:asurkov)]

Comment on attachment 544712 [details] [diff] [review]
cleanup IsMultyColumn() and make it inline

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

::: accessible/src/xul/nsXULListboxAccessible.h
@@ +136,5 @@
>    virtual void UnSelectRow(PRUint32 aRow);
>  
>  protected:
> +  inline bool IsMulticolumn()
> +  { return ColumnCount() > 1; }

two space identation please

Comment 36

[alexander :surkov (:asurkov)]

Comment on attachment 544716 [details] [diff] [review]
always use unsigned integers for indices

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

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +275,5 @@
>    return row->GetChildAt(aColumn);
>  }
>  
>  PRUint32
> +nsXULListboxAccessible::CellIndexAt(PRUint32 aRow, PRUint32 aCol)

you need to change XPCOM interface too, right?

@@ +347,5 @@
>                 "Doesn't implement nsIDOMXULMultiSelectControlElement.");
>  
>    PRInt32 selectedrowCount = 0;
>    nsresult rv = control->GetSelectedCount(&selectedrowCount);
> +  NS_ENSURE_SUCCESS(rv, false);

PRUint32 is expected

Comment 37

[alexander :surkov (:asurkov)]

other patches remove XPCOM support and you don't get it back so I didn't review them. 

At this point it's ok to not touch implementation (and it's worth to do), so do not follow suggestions of this kind in comment #32, otherwise you'd should do that everywhere.

Comment 38

[Trevor Saunders (:tbsaunde)]

(In reply to comment #25)
> Looks ok (not digging into details) but I bet we will be ruined up by
> comments if I start review. I suggest organizing:
> 1) TableAccessible class, including downcasting stuff support in nsAccessible
> 2) TableAccessible implementation for
> 2.1) xul listbox
> 2.2) xul trees 
> 2.3) aria grids
> 2.4) html tables
> 3) replace nsIAccessibleTable usage on TableAccessible

I'd add 4 implement xpcom nsIAccessibleTable wrapper  on the cached xpcom accessible wrappers like the stuff in bug 665672

> sounds ok?

sure, would you prefer then for the followup 1 patch per part?  I don't think making these patches into 2 1 for 1 and 1 for 2.1 shouldn't be too hard.

> @@ -312,5 @@
> > -    return NS_ERROR_FAILURE;
> > -
> > -  *aRowCount = RowCount();
> > -  return NS_OK;
> > -}
> 
> why did you remove all of them?
> 
> @@ -316,5 @@
> > -}
> > -
> > -NS_IMETHODIMP
> > -nsXULListboxAccessible::GetCellAt(PRInt32 aRow, PRInt32 aColumn,
> > -                                  nsIAccessible **aAccessibleCell)
> 
> where's XPCOM implementation supposed to be?

I realized that it would probably easier to just skip to implementing one wrapper for all tables in xpcom/ that would use downcasting from the xpcom wrappers  nsAccessible to TableAccessible then call the internal method.  Rather than leave the wrappers in the table implementations then clean up later.(In reply to comment #34)
> Comment on attachment 544714 [details] [diff] [review] [review]
> improve assertions, some cleanup related to making every index unsigned
> still remains
> 
> Review of attachment 544714 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> do we really need them? For outside usage we should have a check so
> assertions won't ever hit (not taking into account assertions are not
> expected by consumer). If these methods are used internally with wrong
> arguments then maybe it's ok, if you think it brings new level of protection.

hmm, I was thinking it might be easier to put the checks there to reduce the number of places we need to check, but I'm not sure we actually get anything from these checks being where they are so, maybe we don't need to check the internal stuff.

(In reply to comment #36)
> Comment on attachment 544716 [details] [diff] [review] [review]
> always use unsigned integers for indices
> 
> Review of attachment 544716 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/xul/nsXULListboxAccessible.cpp
> @@ +275,5 @@
> >    return row->GetChildAt(aColumn);
> >  }
> >  
> >  PRUint32
> > +nsXULListboxAccessible::CellIndexAt(PRUint32 aRow, PRUint32 aCol)
> 
> you need to change XPCOM interface too, right?

or check the arguments to each method, but I think changing the interface probably makes sense to do in part 4.

(In reply to comment #37)
> other patches remove XPCOM support and you don't get it back so I didn't
> review them. 

ok, there are still a lot of changes coming, all of parts 2.2 2.3 2.4 3 and 4.  Where all the xpcom support will go away in part 2 and then I'll implement the wrapper stuff in part 4.

> At this point it's ok to not touch implementation (and it's worth to do), so
> do not follow suggestions of this kind in comment #32, otherwise you'd
> should do that everywhere.

I'm not sure what your trying to say here. :(

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)
> (In reply to comment #25)
> > Looks ok (not digging into details) but I bet we will be ruined up by
> > comments if I start review. I suggest organizing:
> > 1) TableAccessible class, including downcasting stuff support in nsAccessible
> > 2) TableAccessible implementation for
> > 2.1) xul listbox
> > 2.2) xul trees 
> > 2.3) aria grids
> > 2.4) html tables
> > 3) replace nsIAccessibleTable usage on TableAccessible
> 
> I'd add 4 implement xpcom nsIAccessibleTable wrapper  on the cached xpcom
> accessible wrappers like the stuff in bug 665672

maybe not part of this bug, at least bug summary doesn't say about that. Sure that could be required by the way you fix this bug but I would rather keep things simper as possible.

So
1) Add internal interface and implement it (this bug)
2) Start to use internally (maybe can be done in this bug)
3) Dexpcom objects (deal separately)

Otherwise I afraid we can end up with one huge patch that makes everything.

> > sounds ok?
> 
> sure, would you prefer then for the followup 1 patch per part?  I don't
> think making these patches into 2 1 for 1 and 1 for 2.1 shouldn't be too
> hard.

ok, fine with me

> (In reply to comment #34)
> > Comment on attachment 544714 [details] [diff] [review] [review] [review]
> > improve assertions, some cleanup related to making every index unsigned
> > still remains
> > 
> > Review of attachment 544714 [details] [diff] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > do we really need them? For outside usage we should have a check so
> > assertions won't ever hit (not taking into account assertions are not
> > expected by consumer). If these methods are used internally with wrong
> > arguments then maybe it's ok, if you think it brings new level of protection.
> 
> hmm, I was thinking it might be easier to put the checks there to reduce the
> number of places we need to check, but I'm not sure we actually get anything
> from these checks being where they are so, maybe we don't need to check the
> internal stuff.

technically any assertion can't harm us, but propose of these methods is to control our internal code, API impl code (like MSAA part) must have own checks because we should report correct error and we don't need to assert when 3d party does something wrong. If you want to keep these assertions then it's fine with me.

> (In reply to comment #36)

> > >  PRUint32
> > > +nsXULListboxAccessible::CellIndexAt(PRUint32 aRow, PRUint32 aCol)
> > 
> > you need to change XPCOM interface too, right?
> 
> or check the arguments to each method, but I think changing the interface
> probably makes sense to do in part 4.

Ok, though I'd prefer to keep part 4 as simple as possible, so rather to do that as 3.5 or 4.5.

> (In reply to comment #37)
> 
> > At this point it's ok to not touch implementation (and it's worth to do), so
> > do not follow suggestions of this kind in comment #32, otherwise you'd
> > should do that everywhere.
> 
> I'm not sure what your trying to say here. :(

in comment #32 I asked about implementation changes, then I said it's better to deal with it separately and don't change code logic at this point because changes are going to be big.

Comment 40

[Trevor Saunders (:tbsaunde)]

> 
> ok, fine with me
> 
> > (In reply to comment #34)
> > > Comment on attachment 544714 [details] [diff] [review] [review] [review] [review]
> > > improve assertions, some cleanup related to making every index unsigned
> > > still remains
> > > 
> > > Review of attachment 544714 [details] [diff] [review] [review] [review] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > do we really need them? For outside usage we should have a check so
> > > assertions won't ever hit (not taking into account assertions are not
> > > expected by consumer). If these methods are used internally with wrong
> > > arguments then maybe it's ok, if you think it brings new level of protection.
> > 
> > hmm, I was thinking it might be easier to put the checks there to reduce the
> > number of places we need to check, but I'm not sure we actually get anything
> > from these checks being where they are so, maybe we don't need to check the
> > internal stuff.
> 
> technically any assertion can't harm us, but propose of these methods is to
> control our internal code, API impl code (like MSAA part) must have own
> checks because we should report correct error and we don't need to assert
> when 3d party does something wrong. If you want to keep these assertions
> then it's fine with me.

Ok, I want to give this some thought while I do the other patches.  Do we actually need to check 3rd party callers arguments? that sounds like we could have some  bad perf issues if we need to do foo->RowCount() > aRowIdx && ColCount() > aColIdx before doing the method itself.  of course we should make sure we don't crash, but I'm not sure about making sure the  error code is corect.

> > (In reply to comment #36)
> 
> > > >  PRUint32
> > > > +nsXULListboxAccessible::CellIndexAt(PRUint32 aRow, PRUint32 aCol)
> > > 
> > > you need to change XPCOM interface too, right?
> > 
> > or check the arguments to each method, but I think changing the interface
> > probably makes sense to do in part 4.
> 
> Ok, though I'd prefer to keep part 4 as simple as possible, so rather to do
> that as 3.5 or 4.5.

fine

> > > At this point it's ok to not touch implementation (and it's worth to do), so
> > > do not follow suggestions of this kind in comment #32, otherwise you'd
> > > should do that everywhere.
> > 
> > I'm not sure what your trying to say here. :(
> 
> in comment #32 I asked about implementation changes, then I said it's better
> to deal with it separately and don't change code logic at this point because
> changes are going to be big.

yeah, ok I agree, I'll try to file some spin offs for  those if you agree.

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #40)

> Ok, I want to give this some thought while I do the other patches.  Do we
> actually need to check 3rd party callers arguments? that sounds like we
> could have some  bad perf issues if we need to do foo->RowCount() > aRowIdx
> && ColCount() > aColIdx before doing the method itself.  of course we should
> make sure we don't crash, but I'm not sure about making sure the  error code
> is corect.

ATK doesn't care, IA2 does because it wants to return proper error value. 

RowCount/ColCount() shouldn't big deal from perf point of view since these operations result in array length getting which is cached. Until we have profiling that shows it's a problem let's do no focus on it.

> yeah, ok I agree, I'll try to file some spin offs for  those if you agree.

sure, thank you, one per component since not high priority, I'm not aware of huge XUL listboxes where it can be a problem.

Comment 42

[Trevor Saunders (:tbsaunde)]

Attachment: bug 648265 - add a TableAccessible interface

Comment 43

[Trevor Saunders (:tbsaunde)]

Attachment: add TableAccessibleInterface

Comment 44

[alexander :surkov (:asurkov)]

Comment on attachment 546180 [details] [diff] [review]
add TableAccessibleInterface

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

::: accessible/src/generic/TableAccessible.h
@@ +20,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + * Trevor Saunders <[e-mail]> (original author)

nit: two space indentation relative Contributor(s), i.e. 3 spaces after *

@@ +39,5 @@
> +
> +#ifndef TABLE_ACCESSIBLE_H
> +#define TABLE_ACCESSIBLE_H
> +
> +class TableAccessible

I would love to see a comment what this interface is about

@@ +41,5 @@
> +#define TABLE_ACCESSIBLE_H
> +
> +class TableAccessible
> +{
> +  public:

nit: no indentation

@@ +45,5 @@
> +  public:
> +  virtual ~TableAccessible() { };
> +
> +  /**
> +   * return the caption accessible if any for this table.

nit: don't forget to uppercase first letter please everywhere

@@ +50,5 @@
> +   */
> +  virtual nsAccessible* Caption() = 0;
> +
> +  /**
> +   * get the summary for this table.

return?

@@ +65,5 @@
> +   */
> +  virtual PRUint32 RowCount() = 0;
> +
> +  /**
> +   * return the accessible for the cell at the given row and column.

at row and column indices? and everywhere below because in code row and column are used for accessible objects

@@ +67,5 @@
> +
> +  /**
> +   * return the accessible for the cell at the given row and column.
> +   */
> +  virtual nsAccessible* CellAt(PRUint32 aRow, PRUint32 aCol) = 0;

aRowIdx, aColIdx

@@ +72,5 @@
> +
> +  /**
> +   * return the index of the cell at the given row and column.
> +   */
> +  virtual PRUint32 CellIndexAt(PRUint32 aRowIdx, PRUint32 aColIdx) = 0;

btw, what we are going return for out of range row and col indexes? the same below

@@ +80,5 @@
> +   */
> +  virtual PRUint32 ColIndexAt(PRUint32 aCellIdx) = 0;
> +
> +  /**
> +   *return the row index of the cell with the given index.

space between * and return

@@ +105,5 @@
> +
> +  /**
> +   * get the description of the given column.
> +   */
> +  virtual void ColDescription(PRUint32 aCol, nsAString& aDescription) = 0;

nsString, be consistent

@@ +110,5 @@
> +
> +  /**
> +   * get the description for the given row.
> +   */
> +  virtual void RowDescription(PRUint32 aRow, nsAString& aDescription) = 0;

same

@@ +148,5 @@
> +   */
> +  virtual void SelectedCells(nsTArray<nsAccessible*>* aCells);
> +
> +  /**
> +   */

no comment ending and no text

@@ +177,5 @@
> +   */
> +  virtual void UnselectRow(PRUint32 aRowIdx) = 0;
> +
> +  /**
> +   * return a huristic gues if the table is for layout.

gues -> guess

I'd love to see more detailed comment describing what means for layout and what else can be

@@ +179,5 @@
> +
> +  /**
> +   * return a huristic gues if the table is for layout.
> +   */
> +  virtual bool IsProbablyForLayout();

should be pure virtual

Comment 45

[Trevor Saunders (:tbsaunde)]

Attachment: part 1 tableAccessible interface

Comment 46

[alexander :surkov (:asurkov)]

Comment on attachment 551961 [details] [diff] [review]
part 1 tableAccessible interface

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

::: accessible/src/generic/TableAccessible.h
@@ +41,5 @@
> +#define TABLE_ACCESSIBLE_H
> +
> +/**
> + * abstract accessible table interface for implementing platform specific table
> + * interfaces.

maybe simple: Accessible table interface.

@@ +55,5 @@
> +
> +  /**
> +   * Get the summary for this table.
> +   */
> +  virtual void Summary(nsString& aSummary) = 0;

does implementation use nsString methods? otherwise use nsAString

@@ +58,5 @@
> +   */
> +  virtual void Summary(nsString& aSummary) = 0;
> +
> +  /**
> +   * return the number of columns in the table

Return and dot in the end

@@ +63,5 @@
> +   */
> +  virtual PRUint32 ColCount() = 0;
> +
> +  /**
> +   * return the number of rows in the table

same

@@ +68,5 @@
> +   */
> +  virtual PRUint32 RowCount() = 0;
> +
> +  /**
> +   * return the accessible for the cell at the given row and column.

capital letter
row and column indices

@@ +70,5 @@
> +
> +  /**
> +   * return the accessible for the cell at the given row and column.
> +   */
> +  virtual nsAccessible* CellAt(PRUint32 aRow, PRUint32 aCol) = 0;

append Idx to names

@@ +73,5 @@
> +   */
> +  virtual nsAccessible* CellAt(PRUint32 aRow, PRUint32 aCol) = 0;
> +
> +  /**
> +   * return the index of the cell at the given row and column.

capital letter

@@ +78,5 @@
> +   */
> +  virtual PRUint32 CellIndexAt(PRUint32 aRowIdx, PRUint32 aColIdx) = 0;
> +
> +  /**
> +   * return the index of the column the cell with the given index is in.

capital letter

@@ +83,5 @@
> +   */
> +  virtual PRUint32 ColIndexAt(PRUint32 aCellIdx) = 0;
> +
> +  /**
> +   *return the row index of the cell with the given index.

capital letter and space after *, keep this description in sync of ColIndexAt

@@ +88,5 @@
> +   */
> +  virtual PRUint32 RowIndexAt(PRUint32 aCellIdx) = 0;
> +
> +  /**
> +   * get the row and column indices for the cell at the given index.

capital letter

@@ +95,5 @@
> +                                PRUint32* aColIdx) = 0;
> +
> +  /**
> +   * return the number of columns occupied by the cell at the given row and
> +   * column indices.

capital letter

@@ +101,5 @@
> +  virtual PRUint32 ColExtentAt(PRUint32 aRowIdx, PRUint32 aColIdx) = 0;
> +
> +  /**
> +   * return the number of rows occupied by the cell at the given row and column
> +   * indices.

capital letter

@@ +106,5 @@
> +   */
> +  virtual PRUint32 RowExtentAt(PRUint32 aRowIdx, PRUint32 aColIdx) = 0;
> +
> +  /**
> +   * get the description of the given column.

capital letter

@@ +108,5 @@
> +
> +  /**
> +   * get the description of the given column.
> +   */
> +  virtual void ColDescription(PRUint32 aCol, nsAString& aDescription) = 0;

aColIdx

@@ +111,5 @@
> +   */
> +  virtual void ColDescription(PRUint32 aCol, nsAString& aDescription) = 0;
> +
> +  /**
> +   * get the description for the given row.

capital letter

@@ +113,5 @@
> +
> +  /**
> +   * get the description for the given row.
> +   */
> +  virtual void RowDescription(PRUint32 aRow, nsAString& aDescription) = 0;

aRowIdx

@@ +116,5 @@
> +   */
> +  virtual void RowDescription(PRUint32 aRow, nsAString& aDescription) = 0;
> +
> +  /**
> +   * return if the given column is selected.

capital letter
return true?

@@ +121,5 @@
> +   */
> +  virtual bool IsColSelected(PRUint32 aColIdx) = 0;
> +
> +  /**
> +   * return if the given row is selected.

capital letter
return true?

@@ +126,5 @@
> +   */
> +  virtual bool IsRowSelected(PRUint32 aRowIdx) = 0;
> +
> +  /**
> +   * return if the given cell is selected.

capital letter

@@ +131,5 @@
> +   */
> +  virtual bool IsCellSelected(PRUint32 aRowIdx, PRUint32 aColIdx) = 0;
> +
> +  /**
> +   * return the number of selected cells.

capital letter

@@ +136,5 @@
> +   */
> +  virtual PRUint32 SelectedCellCount() = 0;
> +
> +  /**
> +   * return the number of selected columns.

capital letter

@@ +141,5 @@
> +   */
> +  virtual PRUint32 SelectedColCount() = 0;
> +
> +  /**
> +   * return the number of selected rows.

capital letter

@@ +146,5 @@
> +   */
> +  virtual PRUint32 SelectedRowCount() = 0;
> +
> +  /**
> +   * get the set of selected cells.

capital letter

@@ +151,5 @@
> +   */
> +  virtual void SelectedCells(nsTArray<nsAccessible*>* aCells) = 0;
> +
> +  /**
> +   */

no doc

@@ +155,5 @@
> +   */
> +  virtual void SelectedColIndices(nsTArray<PRUint32>* aCols) = 0;
> +
> +  /**
> +   * get the indices for the set of selected rows.

capital letter
Get the set of selected row indices?

@@ +160,5 @@
> +   */
> +  virtual void SelectedRowIndices(nsTArray<PRUint32>* aRows) = 0;
> +
> +  /**
> +   * select the given column unselecting any other selected columns.

capital letter

@@ +162,5 @@
> +
> +  /**
> +   * select the given column unselecting any other selected columns.
> +   */
> +  virtual void SelectCol(PRUint32 aCol) = 0;

aColIdx

@@ +165,5 @@
> +   */
> +  virtual void SelectCol(PRUint32 aCol) = 0;
> +
> +  /**
> +   * select the given row unselecting all other previously selected rows.

capital letter

@@ +167,5 @@
> +
> +  /**
> +   * select the given row unselecting all other previously selected rows.
> +   */
> +  virtual void SelectRow(PRUint32 aRow) = 0;

aRowIdx

@@ +170,5 @@
> +   */
> +  virtual void SelectRow(PRUint32 aRow) = 0;
> +
> +  /**
> +   * unselect the given column leaving other selected columns selected.

capital letter

@@ +175,5 @@
> +   */
> +  virtual void UnselectCol(PRUint32 aColIdx) = 0;
> +
> +  /**
> +   * unselect the given row leaving other selected rows selected.

capital letter

@@ +180,5 @@
> +   */
> +  virtual void UnselectRow(PRUint32 aRowIdx) = 0;
> +
> +  /**
> +   * return a huristic gues if the table is for layout.

capital letter
huristic -> heuristic
gues -> guess
if the table used for layout rather than for data?

Comment 47

[alexander :surkov (:asurkov)]

btw, what is return value for methods like 
PRUint32 CellIndexAt(PRUint32 aRowIdx, PRUint32 aColIdx)
when given arguments are out of range?

Comment 48

[alexander :surkov (:asurkov)]

why not RowAndColAtIndex -> RowAndColIndicesAt

Comment 49

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #48)
> why not RowAndColAtIndex -> RowAndColIndicesAt

I'll change if you'd like, there's no reason I can think of other than it being a char or two shorter.

(In reply to alexander surkov from comment #47)
> btw, what is return value for methods like 
> PRUint32 CellIndexAt(PRUint32 aRowIdx, PRUint32 aColIdx)
> when given arguments are out of range?

currently that particular method doesn't check the  args, and doesn't technically need to, it  can just get the number of rows and multiply that by the number of columsn, then add the col indexx to that.  of course if you try CellAt(row, col) with invalid args you get null.  another question worth asking here is if it makes sense to have CellIndexAt(row, col) when you can just get the accessible and call IndexInParent() on it.

another issue that I believe exists in the current impl to is we may have issues with index not being correct in tables with non uniform numbers of rows for example

<div role="table"
  <div role=row>
    <div role="cell">foo</div>
    <div role="cell"> bar </div>
  </div>
  <div role="row">
    <div role="cell"> cat </div>
  </div>
  <div role="row">
    <div role="cell> dog </div>
    <div role=?cell"> frog </div>
  </div>
</div>

cell 2,0 should have index 4 right, but I suspect atleast for an aria grid its given 5 , similar for 2,1.

Comment 50

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #49)
> (In reply to alexander surkov from comment #48)
> > why not RowAndColAtIndex -> RowAndColIndicesAt
> I'll change if you'd like, there's no reason I can think of other than it
> being a char or two shorter.

reason to keep naming consistent, for example, RowIndexAt and SelectedColIndices

> (In reply to alexander surkov from comment #47)
> > btw, what is return value for methods like 
> > PRUint32 CellIndexAt(PRUint32 aRowIdx, PRUint32 aColIdx)
> > when given arguments are out of range?
> 
> currently that particular method doesn't check the  args, and doesn't
> technically need to, it  can just get the number of rows and multiply that
> by the number of columsn, then add the col indexx to that.  of course if you
> try CellAt(row, col) with invalid args you get null.

keep in mind implementations of other table. I think it's safer to reserve -1 for wrong given arguments.

>  another question worth
> asking here is if it makes sense to have CellIndexAt(row, col) when you can
> just get the accessible and call IndexInParent() on it.

IndexInParent() called on cell return position in row accessible which value is not in relation of cell index within a table.

> <div role="table"
>   <div role=row>
>     <div role="cell">foo</div>
>     <div role="cell"> bar </div>
>   </div>
>   <div role="row">
>     <div role="cell"> cat </div>
>   </div>
>   <div role="row">
>     <div role="cell> dog </div>
>     <div role=?cell"> frog </div>
>   </div>
> </div>
> 
> cell 2,0 should have index 4 right, but I suspect atleast for an aria grid
> its given 5 , similar for 2,1.

I'd say it should return 3 but current impl is likely returns 4. Anyway makes sense to file a bug but not sure what author meant here.

Comment 51

[Trevor Saunders (:tbsaunde)]

> > (In reply to alexander surkov from comment #47)
> > > btw, what is return value for methods like 
> > > PRUint32 CellIndexAt(PRUint32 aRowIdx, PRUint32 aColIdx)
> > > when given arguments are out of range?
> > 
> > currently that particular method doesn't check the  args, and doesn't
> > technically need to, it  can just get the number of rows and multiply that
> > by the number of columsn, then add the col indexx to that.  of course if you
> > try CellAt(row, col) with invalid args you get null.
> 
> keep in mind implementations of other table. I think it's safer to reserve
> -1 for wrong given arguments.
why do you prefer that to requiring callers who require to get a correct index back to call rowCount() and colCount() themselves to check? actually, after checking with Joanie it appears atk should handle these case ore gracefully than just returning a bs  index, so perhaps there isn't a performance gain to be had for some platform layers by not having the table method do the check.

> >  another question worth
> > asking here is if it makes sense to have CellIndexAt(row, col) when you can
> > just get the accessible and call IndexInParent() on it.
> 
> IndexInParent() called on cell return position in row accessible which value
> is not in relation of cell index within a table.

fair point

Comment 52

[alexander :surkov (:asurkov)]

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

> > keep in mind implementations of other table. I think it's safer to reserve
> > -1 for wrong given arguments.
> why do you prefer that to requiring callers who require to get a correct
> index back to call rowCount() and colCount() themselves to check? actually,
> after checking with Joanie it appears atk should handle these case ore
> gracefully than just returning a bs  index, so perhaps there isn't a
> performance gain to be had for some platform layers by not having the table
> method do the check.

If anything goes wrong (like table has bad markup and/or caller was wrong about cells count) then we are going to return correct 0 index. How much is it ok from point of view of interface robustness? Also keep in mind other methods ColIndexAt and RowIndexAt.

Comment 53

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #52)
> (In reply to Trevor Saunders (:tbsaunde) from comment #51)
> 
> > > keep in mind implementations of other table. I think it's safer to reserve
> > > -1 for wrong given arguments.
> > why do you prefer that to requiring callers who require to get a correct
> > index back to call rowCount() and colCount() themselves to check? actually,
> > after checking with Joanie it appears atk should handle these case ore
> > gracefully than just returning a bs  index, so perhaps there isn't a
> > performance gain to be had for some platform layers by not having the table
> > method do the check.
> 
> If anything goes wrong (like table has bad markup and/or caller was wrong
> about cells count) then we are going to return correct 0 index. How much is

correct 0 index? I don't follow

> it ok from point of view of interface robustness? Also keep in mind other
> methods ColIndexAt and RowIndexAt.

well, I gues returning -1 is a bit better, though it makes me a little sad to need to use signed ints here.

Comment 54

[alexander :surkov (:asurkov)]

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

> > If anything goes wrong (like table has bad markup and/or caller was wrong
> > about cells count) then we are going to return correct 0 index. How much is
> 
> correct 0 index? I don't follow

I mean are you going to return 0 (which can be valid cell index) when there's no cell at given row and col indices?

> > it ok from point of view of interface robustness? Also keep in mind other
> > methods ColIndexAt and RowIndexAt.
> 
> well, I gues returning -1 is a bit better, though it makes me a little sad
> to need to use signed ints here.

I'm not happy with -1 either (because we need to check this value when method returns) but otherwise we should guarantee that in arguments are always valid. Maybe we could do that for internal part, but obviously we can't for AT.

Comment 55

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #54)
> (In reply to Trevor Saunders (:tbsaunde) from comment #53)
> 
> > > If anything goes wrong (like table has bad markup and/or caller was wrong
> > > about cells count) then we are going to return correct 0 index. How much is
> > 
> > correct 0 index? I don't follow
> 
> I mean are you going to return 0 (which can be valid cell index) when
> there's no cell at given row and col indices?

welll, I might, but that would be by accident not design, I was going to return whatever the computation would return under the assumption the row and column were valid.  So trying to remember the different translation schemes, I think html tables would probably return (uint32) -1, and other tables would return some  "random"  value that was the product of there particular translation scheme.

> > > it ok from point of view of interface robustness? Also keep in mind other
> > > methods ColIndexAt and RowIndexAt.
> > 
> > well, I gues returning -1 is a bit better, though it makes me a little sad
> > to need to use signed ints here.
> 
> I'm not happy with -1 either (because we need to check this value when
> method returns) but otherwise we should guarantee that in arguments are
> always valid. Maybe we could do that for internal part, but obviously we
> can't for AT.

yeah, that's why I proposed having the platform wrappers deal with  checking  the row and column or index were valid.  although, I'm not sure they actually can do that for a badly markd up table, since I think they'd need to make assumptions that wouldn't necessarily hold.

Comment 56

[alexander :surkov (:asurkov)]

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

> yeah, that's why I proposed having the platform wrappers deal with  checking
> the row and column or index were valid.  although, I'm not sure they
> actually can do that for a badly markd up table, since I think they'd need
> to make assumptions that wouldn't necessarily hold.

HTML tables can give you example of bad markuped tables when there's no cell for valid row and column. I don't know about ATK but IA2 expects 0 value and E_INVALIDARG.

Comment 57

[Trevor Saunders (:tbsaunde)]

Attachment: add TableAccessible interface with nits fixed

Comment 58

[Trevor Saunders (:tbsaunde)]

Attachment: table accessible downcasting.

Simple patch make nsAccessible define a vfunc to type safetly cast to TableAccessible.  Since nsAccessibles aren't tables the nsAccessible impl returns NULL, and table accessible classes can override the function with an impl that returns this.

Comment 59

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/xul/nsXULListboxAccessible.cpp
> @@ +234,5 @@
> > +nsXULListboxAccessible::ColumnCount()
> > +{
> > +  nsIContent* headContent = nsnull;
> > +  PRUint32 count = mContent->GetChildCount();
> > +  // XXX why does this loop look at all the content children but only care about the last list one?
> 
> it makes sense to crawl by accessible tree looking for the first one, please
> make sure we have mochitest for this

What you mean here is we should 1 change to crawling the accessible tree instead of the content one, and 2 make sure there is a test making sure this functions properly right?  Wouldn't any xul list box in the tests  give us 2? accept the part of  2 xul list boxes as  children of one content node, but I'm not really sure if that's possible.

Comment 60

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #32)
> Comment on attachment 544699 [details] [diff] [review]
> RowCount()
> 
> Review of attachment 544699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/generic/TableAccessible.h
> @@ +48,5 @@
> >     * get the caption accessible if any for this table.
> >     */
> >    virtual nsAccessible* Caption() = 0;
> > +  virtual PRUint32 ColumnCount() = 0;
> > +  virtual PRUint32 RowCount();
> 
> comment it please
> 
> ::: accessible/src/xul/nsXULListboxAccessible.cpp
> @@ +234,5 @@
> > +nsXULListboxAccessible::ColumnCount()
> > +{
> > +  nsIContent* headContent = nsnull;
> > +  PRUint32 count = mContent->GetChildCount();
> > +  // XXX why does this loop look at all the content children but only care about the last list one?
> 
> it makes sense to crawl by accessible tree looking for the first one, please
> make sure we have mochitest for this
> 
> @@ +264,5 @@
> > +  nsCOMPtr<nsIDOMXULSelectControlElement> element(do_QueryInterface(mContent));
> > +  NS_ENSURE_TRUE(element, 0);
> > +
> > +  PRUint32 itemCount = 0;
> > +  return NS_FAILED(element->GetItemCount(&itemCount)) ? 0 : itemCount;
> 
> I'd avoid XBL usage where we can, maybe group attrbiutes can be used for
> that?

I'm not sure exactly what you want, but I think  element might actually e implemented in js, I can't find any concrete class in the tree that inherits from it.number of

Comment 61

[Trevor Saunders (:tbsaunde)]

Attachment: TableAccessible interface

This header requires nsTArray.h and a forward declaration of nsAccessible

Comment 62

[Trevor Saunders (:tbsaunde)]

Attachment: xul listbox table Accessible impl

Comment 63

[Trevor Saunders (:tbsaunde)]

Attachment: xul tree grid table accessible impl

Comment 64

[Trevor Saunders (:tbsaunde)]

Attachment: replace atk usage of nsIAccessibleTable with TableAccessible

Comment 65

[Trevor Saunders (:tbsaunde)]

Attachment: xpcom wrappers

I've put the xpcom wrappers for all 4 table classes in xpcom/xpAccessibleTable.cpp.  While they aren't on a true xpcom only class I think this makes sense to keep them out of our way and to keep track of what we've completely seperated the xpcom and impl for and what we haven't.  It also means that we won't have to move these wrappers in the future, just remove 3 copies and rename the other to xpAccessible::GetFoo().

Let me know if you have issues with this, I'd rather not but putting the wrappers in the file for there respective classes isn't hard.

Comment 66

[alexander :surkov (:asurkov)]

Comment on attachment 553671 [details] [diff] [review]
table accessible downcasting.

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

::: accessible/src/base/nsAccessible.h
@@ +418,5 @@
>  
>    inline bool IsRoot() const { return mFlags & eRootAccessible; }
>    nsRootAccessible* AsRoot();
>  
> +  virtual TableAccessible* AsTableAcc() { return nsnull; }

name it AsTable()

Comment 67

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #65)
> Created attachment 553784 [details] [diff] [review]
> xpcom wrappers
> 
> I've put the xpcom wrappers for all 4 table classes in
> xpcom/xpAccessibleTable.cpp.  While they aren't on a true xpcom only class I
> think this makes sense to keep them out of our way and to keep track of what
> we've completely seperated the xpcom and impl for and what we haven't.  It
> also means that we won't have to move these wrappers in the future, just
> remove 3 copies and rename the other to xpAccessible::GetFoo().
> 
> Let me know if you have issues with this, I'd rather not but putting the
> wrappers in the file for there respective classes isn't hard.

Maybe go with tearoff instead? That should be nicer and closer in the end to what we move to.

Comment 68

[alexander :surkov (:asurkov)]

Comment on attachment 553736 [details] [diff] [review]
xul listbox table Accessible impl

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

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +265,5 @@
>      do_QueryInterface(mContent);
>  
>    nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
> +  control->GetItemAtIndex(aRowIdx, getter_AddRefs(item));
> +  NS_ENSURE_TRUE(item, nsnull);

nit: we don't need to warn on wrong index

@@ +271,5 @@
>    nsCOMPtr<nsIContent> itemContent(do_QueryInterface(item));
>  
>    nsAccessible *row =
>      GetAccService()->GetAccessibleInWeakShell(itemContent, mWeakShell);
> +  NS_ENSURE_TRUE(row, nsnull);

same

@@ +321,4 @@
>  }
>  
> +PRUint32
> +nsXULListboxAccessible::ColExtentAt(PRUint32 aRow, PRUint32 aCol)

aRowIdx, aColIdx

@@ +327,4 @@
>  }
>  
> +PRUint32
> +nsXULListboxAccessible::RowExtentAt(PRUint32 aRow, PRUint32 aCol)

aRowIdx, aColIdx

@@ +333,4 @@
>  }
>  
> +void
> +nsXULListboxAccessible::ColDescription(PRUint32 aCol, nsAString& aDescription)

aColIdx

@@ +339,4 @@
>  }
>  
> +void
> +nsXULListboxAccessible::RowDescription(PRUint32 aRow, nsAString& aDescription)

aRowIdx

@@ +345,4 @@
>  }
>  
> +bool
> +nsXULListboxAccessible::IsColSelected(PRUint32 aCol)

aColIdx

@@ +360,4 @@
>  }
>  
> +bool
> +nsXULListboxAccessible::IsRowSelected(PRUint32 aRow)

aRowIdx

@@ +377,4 @@
>  }
>  
> +bool
> +nsXULListboxAccessible::IsCellSelected(PRUint32 aRow, PRUint32 aCol)

aRow, aCol -> aRowIdx, aColIdx

@@ +387,3 @@
>  {
> +  // if the number of selected rows is 0 we don't need to know how many columns
> +  // there are

nit: Uppercase, dot in the end

@@ +411,4 @@
>  }
>  
> +void
> +nsXULListboxAccessible::SelectedCells(nsTArray<nsAccessible*>* cells)

cells -> aCells

@@ +425,4 @@
>  
>    PRUint32 selectedItemsCount = 0;
> +  if (NS_FAILED(selectedItems->GetLength(&selectedItemsCount)))
> +      return;

nit: wrong indentation of 'if' body
nit: it's ok to keep warning here

@@ +446,4 @@
>  }
>  
> +void
> +nsXULListboxAccessible::SelectedColIndices(nsTArray<PRUint32>* aCols)

methods like this should make sure the array is clear before appending elements
nit: aCols -> aColIndices

@@ +455,4 @@
>  }
>  
> +void
> +nsXULListboxAccessible::SelectedRowIndices(nsTArray<PRUint32>* aRows)

nit: aRows -> aRowIndices

@@ +479,5 @@
>      if (item) {
>        PRInt32 itemIdx = -1;
>        control->GetIndexOfItem(item, &itemIdx);
> +      if (itemIdx != -1) {
> +        NS_ASSERTION(itemIdx >= 0, "negative item index!");

always true at this point

@@ +488,4 @@
>  }
>  
> +void
> +nsXULListboxAccessible::SelectRow(PRUint32 aRow)

aRow -> aRowIdx

@@ +497,5 @@
>  
>    nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
>    control->GetItemAtIndex(aRow, getter_AddRefs(item));
> +  NS_ASSERTION(item,
> +               "item doesn't implement nsIDOMXULControlSelectItemElement");

it means wrong index and no assertion for this case

@@ +504,4 @@
>  }
>  
> +void
> +nsXULListboxAccessible::SelectCol(PRUint32 aCol)

aColIdx

@@ +510,4 @@
>  }
>  
> +void
> +nsXULListboxAccessible::UnselectRow(PRUint32 aRow)

aRowIdx

@@ +519,5 @@
>  
>    nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
>    control->GetItemAtIndex(aRow, getter_AddRefs(item));
> +  NS_ASSERTION(item,
> +               "item doesn't implement nsIDOMXULControlSelectItemElement");

no assertion

@@ +526,4 @@
>  }
>  
> +void
> +nsXULListboxAccessible::UnselectCol(PRUint32 aCol)

aColIdx

::: accessible/src/xul/nsXULListboxAccessible.h
@@ +108,5 @@
>    virtual PRUint32 NativeRole();
>    virtual PRUint64 NativeState();
>  
> +  // TableAccessible
> +  virtual TableAccessible* AsTableAcc() { return this; }

it's not TableAccessible

@@ +125,5 @@
> +  virtual void ColDescription(PRUint32 aCol, nsAString& aDescription);
> +  virtual void RowDescription(PRUint32 aRow, nsAString& aDescription);
> +  virtual bool IsColSelected(PRUint32 aCol);
> +  virtual bool IsRowSelected(PRUint32 aRow);
> +  virtual bool IsCellSelected(PRUint32 aRow, PRUint32 aCol);

aRowIdx, aColIdx for these 7 methods

@@ +131,5 @@
> +  virtual PRUint32 SelectedColCount();
> +  virtual PRUint32 SelectedRowCount();
> +  virtual void SelectedCells(nsTArray<nsAccessible*>* aCells);
> +  virtual void SelectedColIndices(nsTArray<PRUint32>* aCols);
> +  virtual void SelectedRowIndices(nsTArray<PRUint32>* aRows);

aRowIndices/aColIndices

@@ +135,5 @@
> +  virtual void SelectedRowIndices(nsTArray<PRUint32>* aRows);
> +  virtual void SelectCol(PRUint32 aColIdx);
> +  virtual void SelectRow(PRUint32 aRow);
> +  virtual void UnselectCol(PRUint32 aCol);
> +  virtual void UnselectRow(PRUint32 aRow);

aRowIdx, aColIdx for these 3 methods

@@ +142,3 @@
>  protected:
> +  inline bool IsMulticolumn()
> +  { return ColCount() > 1; }

two spaces indentation for function body please

Comment 69

[alexander :surkov (:asurkov)]

Comment on attachment 553735 [details] [diff] [review]
TableAccessible interface

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

::: accessible/src/generic/TableAccessible.h
@@ +77,5 @@
> +   */
> +  virtual nsAccessible* CellAt(PRUint32 aRowIdx, PRUint32 aColIdx) = 0;
> +
> +  /**
> +   * Return the index of the cell at the given row and column.

nit: "row and column indices" like above

@@ +99,5 @@
> +                                  PRInt32* aColIdx) = 0;
> +
> +  /**
> +   * return the number of columns occupied by the cell at the given row and
> +   * Column indices.

nit: return -> Return, Column -> column

@@ +115,5 @@
> +   */
> +  virtual void ColDescription(PRUint32 aColIdx, nsAString& aDescription) = 0;
> +
> +  /**
> +   * Get the description for the given row.

please make keep comments consistent: description of/description for

@@ +120,5 @@
> +   */
> +  virtual void RowDescription(PRUint32 aRowIdx, nsAString& aDescription) = 0;
> +
> +  /**
> +   * Return if the given column is selected.

Return -> Return true

@@ +125,5 @@
> +   */
> +  virtual bool IsColSelected(PRUint32 aColIdx) = 0;
> +
> +  /**
> +   * Return if the given row is selected.

Return -> Return true

@@ +130,5 @@
> +   */
> +  virtual bool IsRowSelected(PRUint32 aRowIdx) = 0;
> +
> +  /**
> +   * Return if the given cell is selected.

Return -> Return true

Comment 70

[alexander :surkov (:asurkov)]

Comment on attachment 553740 [details] [diff] [review]
xul tree grid table accessible impl

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

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +93,4 @@
>  {
> +  PRInt32 rowCount = 0;
> +  mTreeView->GetRowCount(&rowCount);
> +  return rowCount;

wouldn't you need to protect from negative result?

@@ +168,2 @@
>  {
> +  nsAccessible *rowAccessible = GetTreeItemAccessible(aRowIdx);

nit: rename it to 'row' while you are here please

@@ +241,1 @@
>                                                nsAString& aDescription)

nit: fix indentation

@@ +241,4 @@
>                                                nsAString& aDescription)
>  {
>    aDescription.Truncate();
> +  nsAccessible* treeColumns = GetChildAt(0);

nit: FirstChild()

@@ +254,1 @@
>                                             nsAString& aDescription)

nit: fix indentation

@@ +288,4 @@
>  {
>    nsCOMPtr<nsITreeSelection> selection;
>    mTreeView->GetSelection(getter_AddRefs(selection));
> +  NS_ASSERTION(selection, "no selection!");

nit: be consistent, you didn't assert in method above, I think warning is enough here

@@ +294,2 @@
>  
> +  selection->Select(aRowIdx);

I prefer
if (selection)
  selection->Select(aRowIdx);

@@ +305,4 @@
>  {
>    nsCOMPtr<nsITreeSelection> selection;
>    mTreeView->GetSelection(getter_AddRefs(selection));
> +  NS_ASSERTION(selection, "no selection!");

no assertion please, just warning

@@ +311,2 @@
>  
> +  selection->ClearRange(aRowIdx, aRowIdx);

same if (selection)

::: accessible/src/xul/nsXULTreeGridAccessible.h
@@ +61,5 @@
>    // nsIAccessibleTable
>    NS_DECL_NSIACCESSIBLETABLE
>  
> +  // TableAccessible
> +  virtual TableAccessible* AsTableAcc() { return this; }

not a part of TableAccessible

@@ +83,5 @@
> +  virtual PRUint32 SelectedColCount();
> +  virtual PRUint32 SelectedRowCount();
> +  virtual void SelectedColIndices(nsTArray<PRUint32>* aCols);
> +  virtual void SelectedRowIndices(nsTArray<PRUint32>* aRows);
> +  virtual void SelectedCells(nsTArray<nsAccessible*>* aCells);

aRows/aCols to aRowIndices/aColIndices? here and in cpp

Comment 71

[alexander :surkov (:asurkov)]

Comment on attachment 553671 [details] [diff] [review]
table accessible downcasting.

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

::: accessible/src/base/nsAccessible.h
@@ +419,5 @@
>    inline bool IsRoot() const { return mFlags & eRootAccessible; }
>    nsRootAccessible* AsRoot();
>  
> +  virtual TableAccessible* AsTableAcc() { return nsnull; }
> +

maybe add inline IsTable() const { return AsTable(); } to be consistent to rest part and have a nice naming.

Comment 72

[alexander :surkov (:asurkov)]

Comment on attachment 553759 [details] [diff] [review]
replace atk usage of nsIAccessibleTable with TableAccessible

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

::: accessible/src/atk/nsMaiInterfaceTable.cpp
@@ +86,5 @@
> +  nsAccessible* cell = accTable->CellAt(aRowIdx, aColIdx);
> +  if (!cell)
> +    return nsnull;
> +
> +  AtkObject *cellAtkObj = nsAccessibleWrap::GetAtkObject(cell);

nit: AtkObject* cellAtkObj

@@ +200,4 @@
>  }
>  
>  const gchar*
> +getColumnDescriptionCB(AtkTable *aTable, gint aColIdx)

AtkTable*

@@ +215,4 @@
>  }
>  
>  AtkObject*
> +getColumnHeaderCB(AtkTable *aTable, gint aColIdx)

AtkTable*

@@ +220,5 @@
> +  nsAccessibleWrap *accWrap = GetAccessibleWrap(ATK_OBJECT(aTable));
> +  if (!accWrap)
> +    return nsnull;
> +
> +  TableAccessible* accTable = accWrap->AsTableAcc();

while you're here, maybe change accTable/accCell to table/cell?

@@ +254,4 @@
>  }
>  
>  const gchar*
> +getRowDescriptionCB(AtkTable *aTable, gint aRowIdx)

AtkTable*

@@ +269,4 @@
>  }
>  
>  AtkObject*
> +getRowHeaderCB(AtkTable *aTable, gint aRowIdx)

AtkTable*

@@ +328,5 @@
> +  NS_ENSURE_TRUE(accTable, 0);
> +
> +  nsTArray<PRUint32> selectedCols;
> +  accTable->SelectedColIndices(&selectedCols);
> +  gint *atkColumns = g_new(gint, selectedCols.Length());

gint*
what does it give when length is 0?
why don't you want to operate on aSelected?

@@ +368,4 @@
>  }
>  
>  gboolean
> +isColumnSelectedCB(AtkTable *aTable, gint aColIdx)

AtkTable*

@@ +381,4 @@
>  }
>  
>  gboolean
> +isRowSelectedCB(AtkTable *aTable, gint aRowIdx)

AtkTable*

@@ +394,4 @@
>  }
>  
>  gboolean
> +isCellSelectedCB(AtkTable *aTable, gint aRowIdx, gint aColIdx)

AtkTable*

Comment 73

[alexander :surkov (:asurkov)]

Comment on attachment 553735 [details] [diff] [review]
TableAccessible interface

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

::: accessible/src/generic/TableAccessible.h
@@ +152,5 @@
> +
> +  /**
> +   * Get the set of selected cells.
> +   */
> +  virtual void SelectedCells(nsTArray<nsAccessible*>* aCells) = 0;

why did you kill GetSelectedCellIndices, they are used in MSAA part

Comment 74

[alexander :surkov (:asurkov)]

Trevor, that'd be really great if you finish this ;)

Comment 75

[Trevor Saunders (:tbsaunde)]

Attachment: interface inheritance makefiles and downcasting

This should set things up, I'll do the first patch to convert methods and we can make a bunch of good first bugs for the rest.

Comment 76

[alexander :surkov (:asurkov)]

Comment on attachment 600848 [details] [diff] [review]
interface inheritance makefiles and downcasting

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

r=me

::: accessible/src/base/nsARIAGridAccessible.h
@@ +60,5 @@
>    // nsIAccessibleTable
>    NS_DECL_NSIACCESSIBLETABLE
>  
> +  // nsAccessible
> +  virtual TableAccessible* AsTable() { return this; }

I'd say it makes sense to keep it together with other AsXXX methods

::: accessible/src/generic/TableAccessible.h
@@ +14,5 @@
> +
> +/**
> + * Abstract accessible table interface.
> + */
> +class TableAccessible

let's keep it out of namespace for now?

@@ +26,5 @@
> +
> +  /**
> +   * Get the summary for this table.
> +   */
> +  virtual void Summary(nsAString& aSummary) {}

should we use nsString? in other methods too

@@ +66,5 @@
> +                                  PRInt32* aColIdx) {}
> +
> +  /**
> +   * return the number of columns occupied by the cell at the given row and
> +   * Column indices.

return -> return
Column -> column

@@ +79,5 @@
> +
> +  /**
> +   * Get the description of the given column.
> +   */
> +  virtual void ColDescription(PRUint32 aColIdx, nsAString& aDescription) {}

shouldn't you truncate strings?

Comment 77

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/base/nsARIAGridAccessible.h
> @@ +60,5 @@
> >    // nsIAccessibleTable
> >    NS_DECL_NSIACCESSIBLETABLE
> >  
> > +  // nsAccessible
> > +  virtual TableAccessible* AsTable() { return this; }
> 
> I'd say it makes sense to keep it together with other AsXXX methods

since the rest of them aren't virtual I'm not sure what you mean.

> ::: accessible/src/generic/TableAccessible.h
> @@ +14,5 @@
> > +
> > +/**
> > + * Abstract accessible table interface.
> > + */
> > +class TableAccessible
> 
> let's keep it out of namespace for now?

well, the patch currently doesn't namespace it at all,  did you want to change that?

> @@ +26,5 @@
> > +
> > +  /**
> > +   * Get the summary for this table.
> > +   */
> > +  virtual void Summary(nsAString& aSummary) {}
> 
> should we use nsString? in other methods too

I think that's probably a good idea.

> @@ +79,5 @@
> > +
> > +  /**
> > +   * Get the description of the given column.
> > +   */
> > +  virtual void ColDescription(PRUint32 aColIdx, nsAString& aDescription) {}
> 
> shouldn't you truncate strings?

I'm not sure that's needed for internal apis, but maybe its fast so why not?

Comment 78

[alexander :surkov (:asurkov)]

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

> > > +  virtual TableAccessible* AsTable() { return this; }
> > 
> > I'd say it makes sense to keep it together with other AsXXX methods
> 
> since the rest of them aren't virtual I'm not sure what you mean.

idea is just keeping the casting methods all together (like AsDoc and etc), virtual is just implementation details

> well, the patch currently doesn't namespace it at all,  did you want to
> change that?

I think it makes sense since it seems we do that for everything. Just to be consistent.

> > should we use nsString? in other methods too
> 
> I think that's probably a good idea.

yeah, you'd need to check implementation to see if it makes sense, otherwise it means extra copy on xpcom level

> > shouldn't you truncate strings?
> 
> I'm not sure that's needed for internal apis, but maybe its fast so why not?

this makes sense for error protection, internally if somebody reuse the string then it's a problem.

it should be fast, it's just release data but doesn't have early zero length check though

Comment 79

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #78)
> (In reply to Trevor Saunders (:tbsaunde) from comment #77)
> 
> > > > +  virtual TableAccessible* AsTable() { return this; }
> > > 
> > > I'd say it makes sense to keep it together with other AsXXX methods
> > 
> > since the rest of them aren't virtual I'm not sure what you mean.
> 
> idea is just keeping the casting methods all together (like AsDoc and etc),
> virtual is just implementation details

yes, but iirc non of the other ones are implemented in a class other than nsAccessible.

> > well, the patch currently doesn't namespace it at all,  did you want to
> > change that?
> 
> I think it makes sense since it seems we do that for everything. Just to be
> consistent.

ok, mozilla::a11y not just mozilla right?

> > > should we use nsString? in other methods too
> > 
> > I think that's probably a good idea.
> 
> yeah, you'd need to check implementation to see if it makes sense, otherwise
> it means extra copy on xpcom level

it looks like the answer is complicated, but atleast sometimes this wont result in extra copies.  Either way this should make the platform impls better which we may care about more for now, and its possible we can work on optimizing the xpcom case if we need to.

> > > shouldn't you truncate strings?
> > 
> > I'm not sure that's needed for internal apis, but maybe its fast so why not?
> 
> this makes sense for error protection, internally if somebody reuse the
> string then it's a problem.
> 
> it should be fast, it's just release data but doesn't have early zero length
> check though

seems so.

Comment 80

[Trevor Saunders (:tbsaunde)]

Attachment: interface inheritance casting and makefiles

Comment 81

[alexander :surkov (:asurkov)]

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

> > idea is just keeping the casting methods all together (like AsDoc and etc),
> > virtual is just implementation details
> 
> yes, but iirc non of the other ones are implemented in a class other than
> nsAccessible.

the question here is virtual stuff means implementation detail or design stuff. I tend to consider this as purely implementation stuff, thus it makes sense to keep with other casting methods like AsDoc which return document interface.

> > > well, the patch currently doesn't namespace it at all,  did you want to
> > > change that?
> > 
> > I think it makes sense since it seems we do that for everything. Just to be
> > consistent.
> 
> ok, mozilla::a11y not just mozilla right?

sure

Comment 82

[Trevor Saunders (:tbsaunde)]

Attachment: wip

interface down casting and dexpcom first few methods

Comment 83

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 84

[alexander :surkov (:asurkov)]

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

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

I'm not sure I really like these template things but if we're going to get rid them eventually then it's fine with me

::: accessible/src/atk/nsMaiInterfaceTable.cpp
@@ +245,3 @@
>  
> +  nsAccessible* caption = table->Caption();
> +    return caption ? nsAccessibleWrap::GetAtkObject(caption) : nsnull;

wrong indentation

::: accessible/src/base/nsARIAGridAccessible.cpp
@@ +663,5 @@
>    return NS_OK;
>  }
>  
> +void
> +nsARIAGridAccessible::UnselectRow(PRUint32 aRow)

aRow -> aRowIdx

::: accessible/src/base/nsARIAGridAccessible.h
@@ +81,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedCellIndices(PRUint32 *cellsArraySize NS_OUTPARAM, PRInt32 **cellsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedColumnIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedRowIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD SelectRow(PRInt32 rowIndex);
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \

excess \

::: accessible/src/generic/TableAccessible.h
@@ +13,5 @@
> +
> +class nsAccessible;
> +
> +  namespace mozilla {
> +  namespace a11y {

wrong indentation

@@ +16,5 @@
> +  namespace mozilla {
> +  namespace a11y {
> +
> +/**
> + * Abstract accessible table interface.

it's not really abstract

@@ +48,5 @@
> +   */
> +  virtual nsAccessible* CellAt(PRUint32 aRowIdx, PRUint32 aColIdx) { return nsnull; }
> +
> +  /**
> +   * Return the index of the cell at the given row and column.

indices

@@ +93,5 @@
> +  virtual void RowDescription(PRUint32 aRowIdx, nsString& aDescription)
> +    { aDescription.Truncate(); }
> +
> +  /**
> +   * Return if the given column is selected.

Return true

@@ +98,5 @@
> +   */
> +  virtual bool IsColSelected(PRUint32 aColIdx) { return false; }
> +
> +  /**
> +   * Return if the given row is selected.

same

@@ +103,5 @@
> +   */
> +  virtual bool IsRowSelected(PRUint32 aRowIdx) { return false; }
> +
> +  /**
> +   * Return if the given cell is selected.

same

@@ +160,5 @@
> +};
> +
> +}
> +}
> +

} // namespace a11y
} namespace mozilla

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1140,5 @@
>                                     nsISelectionPrivate::TABLESELECTION_COLUMN);
>  }
>  
> +void
> +nsHTMLTableAccessible::UnselectRow(PRUint32 aRow)

aRow -> aRowIdx

::: accessible/src/html/nsHTMLTableAccessible.h
@@ +155,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedCellIndices(PRUint32 *cellsArraySize NS_OUTPARAM, PRInt32 **cellsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedColumnIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedRowIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD SelectRow(PRInt32 rowIndex);
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \

excess \

@@ +161,5 @@
> +
> +  // TableAccessible
> +  virtual nsAccessible* Caption();
> +  virtual void UnselectRow(PRUint32 aRow);
> +  bool IsProbablyLayoutTable();

virtual bool

::: accessible/src/xpcom/xpcAccessibleTable.cpp
@@ +35,5 @@
> +}
> +
> +template<class derived>
> +NS_IMETHODIMP
> +xpcAccessibleTable<derived>::UnselectRow(PRInt32 aRow)

aRow -> aRowIdx

@@ +38,5 @@
> +NS_IMETHODIMP
> +xpcAccessibleTable<derived>::UnselectRow(PRInt32 aRow)
> +{
> +  if (aRow < 0)
> +    return NS_ERROR_INVALID_ARG;

shouldn't you check aRow > rowCount?

@@ +66,5 @@
> +  *aResult = false;
> +
> +  nsHTMLTableAccessible* realThis = static_cast<nsHTMLTableAccessible*>(this);
> +  if (realThis->IsDefunct())
> +    return NS_ERROR_FAILURE;

it makes sense to have common implementation, why would you need to specialize it?

::: accessible/src/xpcom/xpcAccessibleTable.h
@@ +11,5 @@
> +
> +class nsIAccessible;
> +
> +template<class derived>
> +class xpcAccessibleTable: public nsIAccessibleTable

space after :?

@@ +13,5 @@
> +
> +template<class derived>
> +class xpcAccessibleTable: public nsIAccessibleTable
> +{
> +  public:

no indent for public

@@ +18,5 @@
> +    xpcAccessibleTable();
> +    virtual ~xpcAccessibleTable() { }
> +
> +    NS_IMETHOD GetCaption(nsIAccessible** aCaption);
> +    NS_IMETHOD UnselectRow(PRInt32 aRow);

aRow -> aRowIdx

@@ +19,5 @@
> +    virtual ~xpcAccessibleTable() { }
> +
> +    NS_IMETHOD GetCaption(nsIAccessible** aCaption);
> +    NS_IMETHOD UnselectRow(PRInt32 aRow);
> +    NS_IMETHOD IsProbablyForLayout(bool* aIsForLayout);

these definitions are different from definitions used in nsARIAGridAccessible for example, can we be consistent

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +786,5 @@
>    return NS_OK;
>  }
>  
> +void
> +nsXULListboxAccessible::UnselectRow(PRUint32 aRow)

aRow -> aRowIdx

::: accessible/src/xul/nsXULListboxAccessible.h
@@ +122,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedCellIndices(PRUint32 *cellsArraySize NS_OUTPARAM, PRInt32 **cellsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedColumnIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedRowIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD SelectRow(PRInt32 rowIndex);
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \

excess \

::: accessible/src/xul/nsXULTreeGridAccessible.h
@@ +79,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedCellIndices(PRUint32 *cellsArraySize NS_OUTPARAM, PRInt32 **cellsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedColumnIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedRowIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD SelectRow(PRInt32 rowIndex);
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \

excess \

@@ +83,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \
> +  NS_SCRIPTABLE NS_IMETHOD UnselectColumn(PRInt32 columnIndex);
> +
> +  // TableAccessible
> +  virtual void UnselectRow(PRUint32 aRow);

aRow -> aRowIdx

Comment 85

[alexander :surkov (:asurkov)]

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

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

I'm not sure I really like these template things but if we're going to get rid them eventually then it's fine with me

::: accessible/src/atk/nsMaiInterfaceTable.cpp
@@ +245,3 @@
>  
> +  nsAccessible* caption = table->Caption();
> +    return caption ? nsAccessibleWrap::GetAtkObject(caption) : nsnull;

wrong indentation

::: accessible/src/base/nsARIAGridAccessible.cpp
@@ +663,5 @@
>    return NS_OK;
>  }
>  
> +void
> +nsARIAGridAccessible::UnselectRow(PRUint32 aRow)

aRow -> aRowIdx

::: accessible/src/base/nsARIAGridAccessible.h
@@ +81,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedCellIndices(PRUint32 *cellsArraySize NS_OUTPARAM, PRInt32 **cellsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedColumnIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedRowIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD SelectRow(PRInt32 rowIndex);
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \

excess \

::: accessible/src/generic/TableAccessible.h
@@ +13,5 @@
> +
> +class nsAccessible;
> +
> +  namespace mozilla {
> +  namespace a11y {

wrong indentation

@@ +16,5 @@
> +  namespace mozilla {
> +  namespace a11y {
> +
> +/**
> + * Abstract accessible table interface.

it's not really abstract

@@ +48,5 @@
> +   */
> +  virtual nsAccessible* CellAt(PRUint32 aRowIdx, PRUint32 aColIdx) { return nsnull; }
> +
> +  /**
> +   * Return the index of the cell at the given row and column.

indices

@@ +93,5 @@
> +  virtual void RowDescription(PRUint32 aRowIdx, nsString& aDescription)
> +    { aDescription.Truncate(); }
> +
> +  /**
> +   * Return if the given column is selected.

Return true

@@ +98,5 @@
> +   */
> +  virtual bool IsColSelected(PRUint32 aColIdx) { return false; }
> +
> +  /**
> +   * Return if the given row is selected.

same

@@ +103,5 @@
> +   */
> +  virtual bool IsRowSelected(PRUint32 aRowIdx) { return false; }
> +
> +  /**
> +   * Return if the given cell is selected.

same

@@ +160,5 @@
> +};
> +
> +}
> +}
> +

} // namespace a11y
} namespace mozilla

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +1140,5 @@
>                                     nsISelectionPrivate::TABLESELECTION_COLUMN);
>  }
>  
> +void
> +nsHTMLTableAccessible::UnselectRow(PRUint32 aRow)

aRow -> aRowIdx

::: accessible/src/html/nsHTMLTableAccessible.h
@@ +155,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedCellIndices(PRUint32 *cellsArraySize NS_OUTPARAM, PRInt32 **cellsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedColumnIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedRowIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD SelectRow(PRInt32 rowIndex);
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \

excess \

@@ +161,5 @@
> +
> +  // TableAccessible
> +  virtual nsAccessible* Caption();
> +  virtual void UnselectRow(PRUint32 aRow);
> +  bool IsProbablyLayoutTable();

virtual bool

::: accessible/src/xpcom/xpcAccessibleTable.cpp
@@ +16,5 @@
> +// the same time it produces vtables, which will be for this translation unit.
> +template<> xpcAccessibleTable<nsARIAGridAccessible>::xpcAccessibleTable() { }
> +template<> xpcAccessibleTable<nsHTMLTableAccessible>::xpcAccessibleTable() { }
> +template<> xpcAccessibleTable<nsXULListboxAccessible>::xpcAccessibleTable() { }
> +template<> xpcAccessibleTable<nsXULTreeGridAccessible>::xpcAccessibleTable() { }

wouldn't 
template<> xpAccessibleTable<TableAccessible> would do a trick too?

@@ +35,5 @@
> +}
> +
> +template<class derived>
> +NS_IMETHODIMP
> +xpcAccessibleTable<derived>::UnselectRow(PRInt32 aRow)

aRow -> aRowIdx

@@ +38,5 @@
> +NS_IMETHODIMP
> +xpcAccessibleTable<derived>::UnselectRow(PRInt32 aRow)
> +{
> +  if (aRow < 0)
> +    return NS_ERROR_INVALID_ARG;

shouldn't you check aRow > rowCount?

@@ +66,5 @@
> +  *aResult = false;
> +
> +  nsHTMLTableAccessible* realThis = static_cast<nsHTMLTableAccessible*>(this);
> +  if (realThis->IsDefunct())
> +    return NS_ERROR_FAILURE;

it makes sense to have common implementation, why would you need to specialize it?

::: accessible/src/xpcom/xpcAccessibleTable.h
@@ +11,5 @@
> +
> +class nsIAccessible;
> +
> +template<class derived>
> +class xpcAccessibleTable: public nsIAccessibleTable

space after :?

@@ +13,5 @@
> +
> +template<class derived>
> +class xpcAccessibleTable: public nsIAccessibleTable
> +{
> +  public:

no indent for public

@@ +18,5 @@
> +    xpcAccessibleTable();
> +    virtual ~xpcAccessibleTable() { }
> +
> +    NS_IMETHOD GetCaption(nsIAccessible** aCaption);
> +    NS_IMETHOD UnselectRow(PRInt32 aRow);

aRow -> aRowIdx

@@ +19,5 @@
> +    virtual ~xpcAccessibleTable() { }
> +
> +    NS_IMETHOD GetCaption(nsIAccessible** aCaption);
> +    NS_IMETHOD UnselectRow(PRInt32 aRow);
> +    NS_IMETHOD IsProbablyForLayout(bool* aIsForLayout);

these definitions are different from definitions used in nsARIAGridAccessible for example, can we be consistent

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +786,5 @@
>    return NS_OK;
>  }
>  
> +void
> +nsXULListboxAccessible::UnselectRow(PRUint32 aRow)

aRow -> aRowIdx

::: accessible/src/xul/nsXULListboxAccessible.h
@@ +122,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedCellIndices(PRUint32 *cellsArraySize NS_OUTPARAM, PRInt32 **cellsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedColumnIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedRowIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD SelectRow(PRInt32 rowIndex);
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \

excess \

::: accessible/src/xul/nsXULTreeGridAccessible.h
@@ +79,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedCellIndices(PRUint32 *cellsArraySize NS_OUTPARAM, PRInt32 **cellsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedColumnIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD GetSelectedRowIndices(PRUint32 *rowsArraySize NS_OUTPARAM, PRInt32 **rowsArray NS_OUTPARAM);
> +  NS_SCRIPTABLE NS_IMETHOD SelectRow(PRInt32 rowIndex);
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \

excess \

@@ +83,5 @@
> +  NS_SCRIPTABLE NS_IMETHOD SelectColumn(PRInt32 columnIndex); \
> +  NS_SCRIPTABLE NS_IMETHOD UnselectColumn(PRInt32 columnIndex);
> +
> +  // TableAccessible
> +  virtual void UnselectRow(PRUint32 aRow);

aRow -> aRowIdx

Comment 86

[Trevor Saunders (:tbsaunde)]

> @@ +48,5 @@
> > +   */
> > +  virtual nsAccessible* CellAt(PRUint32 aRowIdx, PRUint32 aColIdx) { return nsnull; }
> > +
> > +  /**
> > +   * Return the index of the cell at the given row and column.
> 
> indices

actually, it only gets one, so the correct english is index :)

> @@ +161,5 @@
> > +
> > +  // TableAccessible
> > +  virtual nsAccessible* Caption();
> > +  virtual void UnselectRow(PRUint32 aRow);
> > +  bool IsProbablyLayoutTable();
> 
> virtual bool

I cna but its kind of silly when we don't need it see my later comment on the explicit instantiation stuff.

> 
> ::: accessible/src/xpcom/xpcAccessibleTable.cpp
> @@ +16,5 @@
> > +// the same time it produces vtables, which will be for this translation unit.
> > +template<> xpcAccessibleTable<nsARIAGridAccessible>::xpcAccessibleTable() { }
> > +template<> xpcAccessibleTable<nsHTMLTableAccessible>::xpcAccessibleTable() { }
> > +template<> xpcAccessibleTable<nsXULListboxAccessible>::xpcAccessibleTable() { }
> > +template<> xpcAccessibleTable<nsXULTreeGridAccessible>::xpcAccessibleTable() { }
> 
> wouldn't 
> template<> xpAccessibleTable<TableAccessible> would do a trick too?

I'll try it

> @@ +38,5 @@
> > +NS_IMETHODIMP
> > +xpcAccessibleTable<derived>::UnselectRow(PRInt32 aRow)
> > +{
> > +  if (aRow < 0)
> > +    return NS_ERROR_INVALID_ARG;
> 
> shouldn't you check aRow > rowCount?

I wasn't sure we wanted the perf effect, but if you like sure

> @@ +66,5 @@
> > +  *aResult = false;
> > +
> > +  nsHTMLTableAccessible* realThis = static_cast<nsHTMLTableAccessible*>(this);
> > +  if (realThis->IsDefunct())
> > +    return NS_ERROR_FAILURE;
> 
> it makes sense to have common implementation, why would you need to
> specialize it?

I don't need to, but it makes the call non virtual in the case that the table might actually be for layout and if we know its a type that's  never a layout table then we make no calls.  So, perf win but not really needed I could remove and make IsProbablyLayoutTable() virtual again if want.

> @@ +19,5 @@
> > +    virtual ~xpcAccessibleTable() { }
> > +
> > +    NS_IMETHOD GetCaption(nsIAccessible** aCaption);
> > +    NS_IMETHOD UnselectRow(PRInt32 aRow);
> > +    NS_IMETHOD IsProbablyForLayout(bool* aIsForLayout);
> 
> these definitions are different from definitions used in
> nsARIAGridAccessible for example, can we be consistent

I'm not sure what you mean, xpcom methods should be on one or the other of xpcAccessibleTable or the concrete table object classes but not both.  If you mean the extra macro I gues I could try and remove them, but that sounds like anoying work, and when dexpcom is done they all have gone away anyway.

>

Comment 87

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #86)
> > @@ +48,5 @@
> > > +   */
> > > +  virtual nsAccessible* CellAt(PRUint32 aRowIdx, PRUint32 aColIdx) { return nsnull; }
> > > +
> > > +  /**
> > > +   * Return the index of the cell at the given row and column.
> > 
> > indices
> 
> actually, it only gets one, so the correct english is index :)

row and column is two so I said indices. Ok, english rules might be different.

> > > +  bool IsProbablyLayoutTable();
> > 
> > virtual bool
> 
> I cna but its kind of silly when we don't need it see my later comment on
> the explicit instantiation stuff.

TableAccessible* table = acc->AsTable();
table->IsProbablyLayoutTable() will call TableAccessible version if I don't miss anything. What's silly here?

> > shouldn't you check aRow > rowCount?
> 
> I wasn't sure we wanted the perf effect, but if you like sure

Ok, maybe we should do nothing rather than firing an exception. That's XPCOM part and we can decide what to do. Anyway we need to be consistent. It means either check aRow < 0 and aRow > rowCount or nothing.

> > it makes sense to have common implementation, why would you need to
> > specialize it?
> 
> I don't need to, but it makes the call non virtual in the case that the
> table might actually be for layout and if we know its a type that's  never a
> layout table then we make no calls.  So, perf win but not really needed I
> could remove and make IsProbablyLayoutTable() virtual again if want.

I see

> > these definitions are different from definitions used in
> > nsARIAGridAccessible for example, can we be consistent
> 
> I'm not sure what you mean, xpcom methods should be on one or the other of
> xpcAccessibleTable or the concrete table object classes but not both.  If
> you mean the extra macro I gues I could try and remove them, but that sounds
> like anoying work, and when dexpcom is done they all have gone away anyway.

ok, fair enough

Comment 88

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #87)
> (In reply to Trevor Saunders (:tbsaunde) from comment #86)
> > > @@ +48,5 @@
> > > > +   */
> > > > +  virtual nsAccessible* CellAt(PRUint32 aRowIdx, PRUint32 aColIdx) { return nsnull; }
> > > > +
> > > > +  /**
> > > > +   * Return the index of the cell at the given row and column.
> > > 
> > > indices
> > 
> > actually, it only gets one, so the correct english is index :)
> 
> row and column is two so I said indices. Ok, english rules might be
> different.

I think the better way to say this is return the index of the cell at the given row and column indices.

> > > shouldn't you check aRow > rowCount?
> > 
> > I wasn't sure we wanted the perf effect, but if you like sure
> 
> Ok, maybe we should do nothing rather than firing an exception. That's XPCOM
> part and we can decide what to do. Anyway we need to be consistent. It means
> either check aRow < 0 and aRow > rowCount or nothing.

lets check both.

> > > it makes sense to have common implementation, why would you need to
> > > specialize it?
> > 
> > I don't need to, but it makes the call non virtual in the case that the
> > table might actually be for layout and if we know its a type that's  never a
> > layout table then we make no calls.  So, perf win but not really needed I
> > could remove and make IsProbablyLayoutTable() virtual again if want.
> 
> I see

so your ok with the way it is or I should change back?

Comment 89

[alexander :surkov (:asurkov)]

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

> > > > it makes sense to have common implementation, why would you need to
> > > > specialize it?
> > > 
> > > I don't need to, but it makes the call non virtual in the case that the
> > > table might actually be for layout and if we know its a type that's  never a
> > > layout table then we make no calls.  So, perf win but not really needed I
> > > could remove and make IsProbablyLayoutTable() virtual again if want.
> > 
> > I see
> 
> so your ok with the way it is or I should change back?

keep it as virtual

when you upload a new patch please ask Neil for superreview

Comment 90

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 91

[[e-mail]]

So, I'm trying and failing to see what the benefit of these changes are (for instance diffstat unhelpfully tells me you're adding three times as much code as you're deleting)...

Comment 92

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #91)
> So, I'm trying and failing to see what the benefit of these changes are (for
> instance diffstat unhelpfully tells me you're adding three times as much
> code as you're deleting)...

well, thinking of that patch as a whole change on its on is probably not the best idea, I'd probably classify as the infrastructure and beginning of a change.  There will be a set of patches to follow that will deal with the rest of the methods on the interface.  I'm not sure about exact numbers, but one of the sources of additions is the one time infrastructure for the changes.  Another big addition was temporarily inlining the NS_DECL_NSIACCESSIBLETABLE macros (or tleast most of them) but by the end of the set of patches to come those will all go away.

Comment 93

[[e-mail]]

So are you going to tell me what this change is going to achieve or should I just wait until you make a roll up of all the patches?

Comment 94

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #93)
> So are you going to tell me what this change is going to achieve or should I
> just wait until you make a roll up of all the patches?

well, I believe the plan is to farm the rest of the patches out as good first bugs, so I'll try to explain and hopefully Alex will add if I miss anything.  The main result should be that we have an internal api for dealing with accessible tables that works well for implementing platform apis and can be wrapped to provide the xpcom interface.  So The internal api should be a bit better in that it has useful return types, and provides default implementations since a lot of the accessible table implementations have a bit of boilerplate for methods that particular table doesn't really support.  I'd expect its also a little more performant since it won't require changing ref counts.

Comment 95

[alexander :surkov (:asurkov)]

what we want to get in the end:

introduce TableAccessible interface (dexpcomed nsIAccessibleTable version) and make table classes like nsARIAGridAccessible, nsHTMLTableAccessible etc to implement it.

nsIAccessibleTable XPCOM interface is implemented by xpcAccessibleTable class. Currently we inherit table classes from this class. Later we remove it from inheritance chain and put it as tear off.

As Trevor said we want to finish this work as good first bugs and thus we want to provide a base for it.

Comment 96

[[e-mail]]

(In reply to alexander :surkov from comment #95)
> nsIAccessibleTable XPCOM interface is implemented by xpcAccessibleTable
> class. Currently we inherit table classes from this class. Later we remove
> it from inheritance chain and put it as tear off.
Now that sounds like a plan!

So, to avoid another rewrite when you change it to a tear off, could I suggest adding a TableAccessible* member to xpcAccessibleTable instead of curiously recursive template static casts, so that when you do change it to a tear off all you have to do is to change it to an nsRefPtr<TableAccessible>.

Intermediate code:
class nsARIAGridAccessible : public nsAccessibleWrap,
                             public nsIAccessibleTable
                             public mozilla::a11y::TableAccessible,
                             public xpcAccessibleTable /* will be removed */

nsARIAGridAccessible::
  nsARIAGridAccessible(nsIContent *aContent, nsIWeakReference *aShell) :
  nsAccessibleWrap(aContent, aShell),
  xpcAccessibleTable((TableAccessible*)this) /* will be removed */
{
}

NS_IMPL_ISUPPORTS_INHERITED1(nsARIAGridAccessible,
                             nsAccessible,
                             nsIAccessibleTable) /* will be removed */

class xpcAccessibleTable : public nsISupports
{
  public:
    xpcAccessibleTable(TableAccessible *acc)
      : mAcc(acc)
    {}

  private:
    TableAccessible* mAcc; /* will be changed */
}

Final code:
class nsARIAGridAccessible : public nsAccessibleWrap,
                             public nsIAccessibleTable
                             public mozilla::a11y::TableAccessible

nsARIAGridAccessible::
  nsARIAGridAccessible(nsIContent *aContent, nsIWeakReference *aShell) :
  nsAccessibleWrap(aContent, aShell)
{
}

NS_IMPL_ADDREF_INHERITED(nsARIAGridAccessible, nsAccessible)
NS_IMPL_RELEASE_INHERITED(nsARIAGridAccessible, nsAccessible)
NS_INTERFACE_MAP_BEGIN(nsARIAGridAccessible)
NS_INTERFACE_MAP_ENTRY_TEAROFF(nsIAccessibleTable, new xpcAccessibleTable(this))
NS_INTERFACE_MAP_END_INHERITING(nsAccessible)

class xpcAccessibleTable : public nsISupports
{
  public:
    xpcAccessibleTable(TableAccessible *acc)
      : mAcc(acc)
    {}

  private:
    nsRefPtr<TableAccessible> mAcc;
}

Comment 97

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #96)
> (In reply to alexander :surkov from comment #95)
> > nsIAccessibleTable XPCOM interface is implemented by xpcAccessibleTable
> > class. Currently we inherit table classes from this class. Later we remove
> > it from inheritance chain and put it as tear off.
> Now that sounds like a plan!

well, not really tearoff, I think a better to explain would be to say we have one internal accessible object and one "external" one to implement xpcom interfaces using  internal object.  I'm not sure yet if that means that each of the accessible table classes can use the same class to make they're xpcom accessible or if we'll need one class for each that inherits from a class implementing nsIAccessibleTable.

> So, to avoid another rewrite when you change it to a tear off, could I
> suggest adding a TableAccessible* member to xpcAccessibleTable instead of
> curiously recursive template static casts, so that when you do change it to
> a tear off all you have to do is to change it to an
> nsRefPtr<TableAccessible>.

well, two non-major things we need to know its an accessible as in nsAccessible too for IsDefunct() and TableAccessible doesn't require that the implementor be refcounted (which I gues could change).

> Intermediate code:
> class nsARIAGridAccessible : public nsAccessibleWrap,
>                              public nsIAccessibleTable
>                              public mozilla::a11y::TableAccessible,
>                              public xpcAccessibleTable /* will be removed */
> 
> nsARIAGridAccessible::
>   nsARIAGridAccessible(nsIContent *aContent, nsIWeakReference *aShell) :
>   nsAccessibleWrap(aContent, aShell),
>   xpcAccessibleTable((TableAccessible*)this) /* will be removed */
> {
> }
> 
> NS_IMPL_ISUPPORTS_INHERITED1(nsARIAGridAccessible,
>                              nsAccessible,
>                              nsIAccessibleTable) /* will be removed */
> 
> class xpcAccessibleTable : public nsISupports
> {
>   public:
>     xpcAccessibleTable(TableAccessible *acc)
>       : mAcc(acc)
>     {}
> 
>   private:
>     TableAccessible* mAcc; /* will be changed */
> }
> 
> Final code:
> class nsARIAGridAccessible : public nsAccessibleWrap,
>                              public nsIAccessibleTable

btw I assume you don't actually mean to leave that there right?

Comment 98

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #97)
> (In reply to [e-mail] from comment #96)
> > (In reply to alexander :surkov from comment #95)
> > > nsIAccessibleTable XPCOM interface is implemented by xpcAccessibleTable
> > > class. Currently we inherit table classes from this class. Later we remove
> > > it from inheritance chain and put it as tear off.
> > Now that sounds like a plan!
> 
> well, not really tearoff,

hm, it's better to get an agreement between us before getting Neil into stuffs :) I thought we were agreed to expose XPCOM impls for interfaces like nsIAccessibleTable as a real tear-offs.

Comment 99

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #98)
> (In reply to Trevor Saunders (:tbsaunde) from comment #97)
> > (In reply to [e-mail] from comment #96)
> > > (In reply to alexander :surkov from comment #95)
> > > > nsIAccessibleTable XPCOM interface is implemented by xpcAccessibleTable
> > > > class. Currently we inherit table classes from this class. Later we remove
> > > > it from inheritance chain and put it as tear off.
> > > Now that sounds like a plan!
> > 
> > well, not really tearoff,
> 
> hm, it's better to get an agreement between us before getting Neil into
> stuffs :) I thought we were agreed to expose XPCOM impls for interfaces like
> nsIAccessibleTable as a real tear-offs.

benefits are lesser memory per accessible object and we don't need to wait for nsIAccessible dexcomination before we can do that for nsIAccessibleTable.

Comment 100

[[e-mail]]

(In reply to Trevor Saunders from comment #97)
> well, not really tearoff, I think a better to explain would be to say we
> have one internal accessible object and one "external" one to implement
> xpcom interfaces using internal object.
Fair enough, but that works in a similar manner.

> I'm not sure yet if that means
> that each of the accessible table classes can use the same class to make
> they're xpcom accessible or if we'll need one class for each that inherits
> from a class implementing nsIAccessibleTable.
Well it wouldn't work so well if they all needed separate classes!

> > So, to avoid another rewrite when you change it to a tear off, could I
> > suggest adding a TableAccessible* member to xpcAccessibleTable instead of
> > curiously recursive template static casts, so that when you do change it to
> > a tear off all you have to do is to change it to an
> > nsRefPtr<TableAccessible>.
> well, two non-major things we need to know its an accessible as in
> nsAccessible too for IsDefunct() and TableAccessible doesn't require that
> the implementor be refcounted (which I gues could change).
OK, so there are two approaches here.
The first is to treat the relationship like a weak reference. When the accessible becomes defunct it clears the reference from the xpcAccessibleTable which then starts throwing exceptions on its methods. However to do this the accessible needs to keep track of its xpcAccessibleTable tearoff, which consumes an extra pointer. Also, it would always hand out the same xpcAccessibleTable each time, which may or may not be an advantage.
The second is to treat the relationship like a tearoff. The tearoff needs to be able to access all the methods on the original object that it needs, but this could be done by declaring dummy methods on TableAccessible which are the same signature as the existing methods on the other interfaces, which the actual nsAccessible object already implements, or they could be declared on an mozilla::a11y::Accessible base interface which other xpcAccessible object could also use in the future.

> >                              public nsIAccessibleTable
> btw I assume you don't actually mean to leave that there right?
Yeah, when I copied those lines from your patch I hadn't noticed the "-" at the beginning of the line ;-)

Comment 101

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #100)
> (In reply to Trevor Saunders from comment #97)
> > well, not really tearoff, I think a better to explain would be to say we
> > have one internal accessible object and one "external" one to implement
> > xpcom interfaces using internal object.
> Fair enough, but that works in a similar manner.

true

> > I'm not sure yet if that means
> > that each of the accessible table classes can use the same class to make
> > they're xpcom accessible or if we'll need one class for each that inherits
> > from a class implementing nsIAccessibleTable.
> Well it wouldn't work so well if they all needed separate classes!

I'm not sure I follow this, what problem do you see with different xpcom wrapper classes like these

class xpcARIAGrid : public xpcTableAccessible,
public nsIAccessibleFoo
{
  ....
};

class xpcHTMLTableAccessible : public xpcAccessibleTable,
public nsIAccessibleBar
{
  ...
};

what isn't clear to me yet is if we'll need that, r if we can have xpcAccessibleTable be the wrapper for each.  In the second case I think we'd probalby end up needing something like this template stuff, but if we can use one wrapper for all thre it might well be worth not using templates.

> > > So, to avoid another rewrite when you change it to a tear off, could I
> > > suggest adding a TableAccessible* member to xpcAccessibleTable instead of
> > > curiously recursive template static casts, so that when you do change it to
> > > a tear off all you have to do is to change it to an
> > > nsRefPtr<TableAccessible>.
> > well, two non-major things we need to know its an accessible as in
> > nsAccessible too for IsDefunct() and TableAccessible doesn't require that
> > the implementor be refcounted (which I gues could change).
> OK, so there are two approaches here.
> The first is to treat the relationship like a weak reference. When the
> accessible becomes defunct it clears the reference from the
> xpcAccessibleTable which then starts throwing exceptions on its methods.
> However to do this the accessible needs to keep track of its
> xpcAccessibleTable tearoff, which consumes an extra pointer. Also, it would
> always hand out the same xpcAccessibleTable each time, which may or may not
> be an advantage.

well, I believe me and Alex have agreed we want to cache the wrappers in a hash table anyway so yes we'll do this to take care of the pointer the wrapper has.  Using this approach now though means that we'd have a class that holds a weak pointer to iself and a cache of mappings from things to themselves which is kind of silly but infrastructure we need anyway, so if Alex is fine with adding it now it works for me.

> The second is to treat the relationship like a tearoff. The tearoff needs to
> be able to access all the methods on the original object that it needs, but
> this could be done by declaring dummy methods on TableAccessible which are
> the same signature as the existing methods on the other interfaces, which
> the actual nsAccessible object already implements, or they could be declared
> on an mozilla::a11y::Accessible base interface which other xpcAccessible
> object could also use in the future.

that would lead to diamond inheritance of the mozilla::a11y::Accessible interface no? and I'd just assume avoid that.

Comment 102

[[e-mail]]

(In reply to Trevor Saunders from comment #101)
> I'm not sure I follow this, what problem do you see with different xpcom
> wrapper classes like these
> 
> class xpcARIAGrid : public xpcTableAccessible,
> public nsIAccessibleFoo
> {
>   ....
> };
> 
> class xpcHTMLTableAccessible : public xpcAccessibleTable,
> public nsIAccessibleBar
> {
>   ...
> };
> 
> what isn't clear to me yet is if we'll need that, r if we can have
> xpcAccessibleTable be the wrapper for each.
As long as the implementation of xpcAccessibleTable only depends on the methods in AccessibleTable then I think this should work. If you only need to construct subclasses of xpcAccessibleTable then you can actually go one step further and make the xpcAccessibleTable methods non-virtual and use NS_FORWARD_NSIACCESSIBLETABLE(xpcAccessibleTable::) in the subclasses.

> that would lead to diamond inheritance of the mozilla::a11y::Accessible
> interface no? and I'd just assume avoid that.
So what you're saying is that there are existing classes that implement multiple nsIAccessible interfaces in a way that cannot be converted to a heirarchy?

Comment 103

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #102)
> (In reply to Trevor Saunders from comment #101)
> > I'm not sure I follow this, what problem do you see with different xpcom
> > wrapper classes like these
> > 
> > class xpcARIAGrid : public xpcTableAccessible,
> > public nsIAccessibleFoo
> > {
> >   ....
> > };
> > 
> > class xpcHTMLTableAccessible : public xpcAccessibleTable,
> > public nsIAccessibleBar
> > {
> >   ...
> > };
> > 
> > what isn't clear to me yet is if we'll need that, r if we can have
> > xpcAccessibleTable be the wrapper for each.
> As long as the implementation of xpcAccessibleTable only depends on the
> methods in AccessibleTable then I think this should work. If you only need
> to construct subclasses of xpcAccessibleTable then you can actually go one
> step further and make the xpcAccessibleTable methods non-virtual and use
> NS_FORWARD_NSIACCESSIBLETABLE(xpcAccessibleTable::) in the subclasses.

would that actually win anything? it seems that would just add some indirection, instead of inheriting the virtual function you have a virtual function that calls an inherited non virtual function so you still have the virtual function overhead...

> > that would lead to diamond inheritance of the mozilla::a11y::Accessible
> > interface no? and I'd just assume avoid that.
> So what you're saying is that there are existing classes that implement
> multiple nsIAccessible interfaces in a way that cannot be converted to a
> heirarchy?

well, for the case of tables we have things like
class nsARIAGrdiAccessible : public nsAccessible,
public nsIAccessibleTable
{
};

renaming nsAccessible to mozilla::a11y::Accessible and usng mozilla::a11y::TableAccessible instead of nsIAccessible table we get
class nsARIAGridAccessible : public Accessible,
public TableAccessible
{
};

then if TableAccessible inherits Accessible you have a diamond right?
if we

Comment 104

[[e-mail]]

(In reply to Trevor Saunders from comment #103)
> (In reply to comment #102)
> > As long as the implementation of xpcAccessibleTable only depends on the
> > methods in AccessibleTable then I think this should work. If you only need
> > to construct subclasses of xpcAccessibleTable then you can actually go one
> > step further and make the xpcAccessibleTable methods non-virtual and use
> > NS_FORWARD_NSIACCESSIBLETABLE(xpcAccessibleTable::) in the subclasses.
> would that actually win anything? it seems that would just add some
> indirection, instead of inheriting the virtual function you have a virtual
> function that calls an inherited non virtual function so you still have the
> virtual function overhead...
Ah, but the point is that xpcAccessibleTable now has no virtual methods, so no vtable overhead. Also, a smart enough compiler will just generate a thunk rather than a separate call.

> class nsARIAGridAccessible : public Accessible,
> public TableAccessible
> {
> };
> 
> then if TableAccessible inherits Accessible you have a diamond right?
Oh, that sort of diamond. Sorry, I didn't realise what you meant before.

Comment 105

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #104)
> (In reply to Trevor Saunders from comment #103)
> > (In reply to comment #102)
> > > As long as the implementation of xpcAccessibleTable only depends on the
> > > methods in AccessibleTable then I think this should work. If you only need
> > > to construct subclasses of xpcAccessibleTable then you can actually go one
> > > step further and make the xpcAccessibleTable methods non-virtual and use
> > > NS_FORWARD_NSIACCESSIBLETABLE(xpcAccessibleTable::) in the subclasses.
> > would that actually win anything? it seems that would just add some
> > indirection, instead of inheriting the virtual function you have a virtual
> > function that calls an inherited non virtual function so you still have the
> > virtual function overhead...
> Ah, but the point is that xpcAccessibleTable now has no virtual methods, so
> no vtable overhead. Also, a smart enough compiler will just generate a thunk
> rather than a separate call.

oh, so your thinking it saves you a vtable in the object inheriting xpcAccessibleTable?

Comment 106

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #104)
> (In reply to Trevor Saunders from comment #103)
> > (In reply to comment #102)
> > > As long as the implementation of xpcAccessibleTable only depends on the
> > > methods in AccessibleTable then I think this should work. If you only need
> > > to construct subclasses of xpcAccessibleTable then you can actually go one
> > > step further and make the xpcAccessibleTable methods non-virtual and use
> > > NS_FORWARD_NSIACCESSIBLETABLE(xpcAccessibleTable::) in the subclasses.
> > would that actually win anything? it seems that would just add some
> > indirection, instead of inheriting the virtual function you have a virtual
> > function that calls an inherited non virtual function so you still have the
> > virtual function overhead...
> Ah, but the point is that xpcAccessibleTable now has no virtual methods, so
> no vtable overhead. Also, a smart enough compiler will just generate a thunk
> rather than a separate call.

since we wont do it all at once we'll sort of have to inline the macro bit by bit s methods are converted, but that should be fine. if a bit ugly in the short term.

Comment 107

[alexander :surkov (:asurkov)]

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

> well, I believe me and Alex have agreed we want to cache the wrappers in a
> hash table anyway so yes we'll do this to take care of the pointer the
> wrapper has.  Using this approach now though means that we'd have a class
> that holds a weak pointer to iself and a cache of mappings from things to
> themselves which is kind of silly but infrastructure we need anyway, so if
> Alex is fine with adding it now it works for me.

yeah, we did. But making nsIAccessibleTable impl a tear-off doesn't require us to introduce all stuffs right now. Following tear-off idea we can dexpcom most of interfaces and then we can do final separation XPCOM impl from internal objects as you say.

Comment 108

[[e-mail]]

(In reply to Trevor Saunders from comment #105)
> oh, so your thinking it saves you a vtable in the object inheriting
> xpcAccessibleTable?
Indeed.

(In reply to Trevor Saunders from comment #106)
> since we wont do it all at once we'll sort of have to inline the macro bit
> by bit s methods are converted, but that should be fine. if a bit ugly in
> the short term.
You could probably do it with a bunch of using statements (not quite so ugly).

Comment 109

[Trevor Saunders (:tbsaunde)]

Attachment: patch

I want to read over this again before asking for reviews, but it should be fairly close to done so feel free to review if you like :)

Comment 110

[[e-mail]]

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

> LOCAL_INCLUDES = \
>   -I$(srcdir)/../base \
>+  -I$(srcdir)/../generic \
>+  -I$(srcdir)/../html \
>+  -I$(srcdir)/../xul \
>   $(NULL)
Do you still need these?

>+#include "nsARIAGridAccessible.h"
>+#include "nsHTMLTableAccessible.h"
>+#include "nsXULListboxAccessible.h"
>+#include "nsXULTreeGridAccessible.h"
Or these?

>+class xpcAccessibleTable : public nsIAccessibleTable
This shouldn't inherit from (or #include) nsIAccessibleTable.

>+    virtual ~xpcAccessibleTable() { }
No virtual methods please!

Comment 111

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #110)
> Comment on attachment 607817 [details] [diff] [review]
> patch
> 
> > LOCAL_INCLUDES = \
> >   -I$(srcdir)/../base \
> >+  -I$(srcdir)/../generic \
> >+  -I$(srcdir)/../html \
> >+  -I$(srcdir)/../xul \
> >   $(NULL)
> Do you still need these?

without knowing which makefile its hard to say

> >+class xpcAccessibleTable : public nsIAccessibleTable
> This shouldn't inherit from (or #include) nsIAccessibleTable.

so, one "interesting' result of this is that there is a forced ordering of what order methods are converted in because some methods call others and you can only convert a method once every thing it calls has been converted.

Comment 112

[[e-mail]]

(In reply to Trevor Saunders from comment #111)
> (In reply to comment #110)
> > (From update of attachment 607817 [details] [diff] [review])
> > > LOCAL_INCLUDES = \
> > >   -I$(srcdir)/../base \
> > >+  -I$(srcdir)/../generic \
> > >+  -I$(srcdir)/../html \
> > >+  -I$(srcdir)/../xul \
> > >   $(NULL)
> > Do you still need these?
> without knowing which makefile its hard to say
Only the xpcom makefile has the ../xul addition ;-)

> > >+class xpcAccessibleTable : public nsIAccessibleTable
> > This shouldn't inherit from (or #include) nsIAccessibleTable.
> so, one "interesting' result of this is that there is a forced ordering of
> what order methods are converted in because some methods call others and you
> can only convert a method once every thing it calls has been converted.
Actually I was surprised that you were considering converting a method that needed to call an unconverted method.

Comment 113

[Trevor Saunders (:tbsaunde)]

> > > >+class xpcAccessibleTable : public nsIAccessibleTable
> > > This shouldn't inherit from (or #include) nsIAccessibleTable.
> > so, one "interesting' result of this is that there is a forced ordering of
> > what order methods are converted in because some methods call others and you
> > can only convert a method once every thing it calls has been converted.
> Actually I was surprised that you were considering converting a method that
> needed to call an unconverted method.

you expected I thought deeply about that decision ;)  However I ended up undoing that for now to avoid this problem.

Comment 114

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 115

[Trevor Saunders (:tbsaunde)]

Attachment: reduce includes of nsXULTreeAccessible.h and nsHTMLTableAccessible.h

they don't seem to be needed so lets just stop including them.

Comment 116

[alexander :surkov (:asurkov)]

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

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

looks cool, happy those weird template has gone :)

::: accessible/src/generic/TableAccessible.h
@@ +12,5 @@
> +#include "prtypes.h"
> +
> +class nsAccessible;
> +
> +  namespace mozilla {

nit: fix indent

::: accessible/src/xpcom/xpcAccessibleTable.h
@@ +24,5 @@
> +    nsresult GetCaption(nsIAccessible** aCaption);
> +    nsresult IsProbablyForLayout(bool* aIsForLayout);
> +
> +  protected:
> +    mozilla::a11y::TableAccessible* mTable;

wrong indentation of whole block

Comment 117

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 118

[[e-mail]]

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

> LOCAL_INCLUDES += \
>   -I$(srcdir) \
>-  -I$(srcdir)/../xpcom \
>+  -I$(srcdir)/../generic \
>   -I$(srcdir)/../html \
>+  -I$(srcdir)/../xpcom \
>   -I$(srcdir)/../xul \
Alphabetical order, I take it?

(Technically this patch is missing a line containing a single space at the end, although patch ignores it as part of its automatic fuzz.)

Comment 119

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #118)
> Comment on attachment 609270 [details] [diff] [review]
> patch
> 
> > LOCAL_INCLUDES += \
> >   -I$(srcdir) \
> >-  -I$(srcdir)/../xpcom \
> >+  -I$(srcdir)/../generic \
> >   -I$(srcdir)/../html \
> >+  -I$(srcdir)/../xpcom \
> >   -I$(srcdir)/../xul \
> Alphabetical order, I take it?

yeah, figured I'd put them in order while there, not trying to do magic.

> (Technically this patch is missing a line containing a single space at the
> end, although patch ignores it as part of its automatic fuzz.)

weird, I'll run it through git rebase --whitespace=fix

Comment 120

[Trevor Saunders (:tbsaunde)]

landed
https://hg.mozilla.org/integration/mozilla-central/rev/6e03c7c669b7
https://hg.mozilla.org/integration/mozilla-central/rev/10642cc4b2b3

Comment 121

[Trevor Saunders (:tbsaunde)]

Alex do you want to call this fixed now or wait for the bugs to convert the rest of the methods are fixed too?

Comment 122

[Trevor Saunders (:tbsaunde)]

and because it appears I suck at pushing to try I backed this out as
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7af1c8b8f83

Comment 123

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #121)
> Alex do you want to call this fixed now or wait for the bugs to convert the
> rest of the methods are fixed too?

I'd say fit summary, say it's fixed, file meta and blockings.

Comment 124

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #123)
> (In reply to Trevor Saunders (:tbsaunde) from comment #121)
> > Alex do you want to call this fixed now or wait for the bugs to convert the
> > rest of the methods are fixed too?
> 
> I'd say fit summary, say it's fixed, file meta and blockings.

ok

Comment 125

[Trevor Saunders (:tbsaunde)]

was green on try with includes added so relanded
https://hg.mozilla.org/integration/mozilla-inbound/rev/d224959f26ad
fwiw try run was https://tbpl.mozilla.org/?tree=Try&rev=6b7d4a7b7f18

Comment 126

[Ed Morley [:emorley]]

(In reply to Trevor Saunders (:tbsaunde) from comment #120)
> landed
> https://hg.mozilla.org/integration/mozilla-central/rev/10642cc4b2b3

https://hg.mozilla.org/mozilla-central/rev/10642cc4b2b3

Comment 127

[Ed Morley [:emorley]]

(In reply to Trevor Saunders (:tbsaunde) from comment #125)
> was green on try with includes added so relanded
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d224959f26ad

https://hg.mozilla.org/mozilla-central/rev/d224959f26ad