I think we should use the template trick unless someone proposes something better than what I've thought of, objects loadly :) basically we'd do the following CAccessibleTable would be templated on the name of a class so we'd have template<typename derived> class CAccessibleTable { ... }; and each method would be template<typename derived> HRESULT STDCALLTYPE CAccessibleTable::get_Foo() { derived* thisObj = static_cast<derived*>(this); thisObj->GetFoo(); ... } the each class that inherits from CAccessibleTable would be class HTMLTableAccessibleWrap : public HTMLTableAccessible, public CAccessibleTable<HTMLTableAccessibleWrap> { ... }; that way the static cast above just ends up casting to the right typ. The lternative that would come to mind is doing the same thing as we do with xpcAccessisbleTable and stash a pointer to ourselves, but of the right type. The biggest downside to that approach is that we increase the size of each accessible table by a word. I could live with this approach if people would really prefer it, but it doesn't seem like the best of ideas.

the first approach seems a little bit crazy but it benefits from memory consumption point of view. I'd like to get feedback from c++ gurus. Neil, do you have thoughts?

(In reply to alexander :surkov from comment #2) > the first approach seems a little bit crazy but it benefits from memory > consumption point of view. I'd like to get feedback from c++ gurus. So, the reason I went with the extra member approach was basically for the same reason we did with xpcAccessibletable.

(In reply to Trevor Saunders (:tbsaunde) from comment #3) > (In reply to alexander :surkov from comment #2) > > the first approach seems a little bit crazy but it benefits from memory > > consumption point of view. I'd like to get feedback from c++ gurus. > > So, the reason I went with the extra member approach was basically for the > same reason we did with xpcAccessibletable. ok, what was a reason btw would you remind me?

Comment on attachment 700760 [details] [diff] [review] don't use nsIAccessibleTable type stuff in ia2AccessibleTable Review of attachment 700760 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/msaa/XULTreeGridAccessibleWrap.cpp @@ +17,5 @@ > IMPL_IUNKNOWN_INHERITED1(XULTreeGridAccessibleWrap, > AccessibleWrap, > ia2AccessibleTable) > > +void nit: extra space ::: accessible/src/windows/ia2/ia2AccessibleTable.cpp @@ +51,1 @@ > IUnknown **aAccessible) nit: please align it @@ +66,1 @@ > return E_FAIL; i'm curious whether this should be E_INVALIDARG @@ +117,5 @@ > A11Y_TRYBLOCK_BEGIN > > *aDescription = NULL; > + if (aColIdx < 0) > + return E_INVALIDARG; if you do invalid arg for < 0 then it'd be reasonable if you do it for > colcount. basically it's applicable to all methods here. Note, xpcTableAccessible (which was used here) has those checks @@ +342,5 @@ > + *aNChildren = 0; > + if (!mTable) > + return CO_E_OBJNOTCONNECTED; > + > + nsAutoTArray<uint32_t, 30> cellIndices; nit: two spaces @@ +344,5 @@ > + return CO_E_OBJNOTCONNECTED; > + > + nsAutoTArray<uint32_t, 30> cellIndices; > + mTable->SelectedCellIndices(&cellIndices); > + nit: whitespace on empty line @@ +346,5 @@ > + nsAutoTArray<uint32_t, 30> cellIndices; > + mTable->SelectedCellIndices(&cellIndices); > + > + uint32_t maxCells = cellIndices.Length(); > + if (aMaxChildren > 0 && uint32_t(aMaxChildren) < maxCells) I prefer c++ casting style @@ +454,4 @@ > { > A11Y_TRYBLOCK_BEGIN > > + *aIsSelected = false; nit: wrong indent @@ +623,4 @@ > > + *aCells = > + static_cast<IUnknown**>(::CoTaskMemAlloc(sizeof(IUnknown*) * > + cells.Length())); does it never fail? ::: accessible/src/windows/ia2/ia2AccessibleTable.h @@ +167,5 @@ > /* [out, size_is(,*nRows)] */ long **selectedRows, > /* [out, retval] */ long *nRows); > > // nsISupports > NS_IMETHOD QueryInterface(const nsIID& uuid, void** result) = 0; it seems you don't need QueryInterface anymore @@ +170,5 @@ > // nsISupports > NS_IMETHOD QueryInterface(const nsIID& uuid, void** result) = 0; > > +protected: > + ia2AccessibleTable(mozilla::a11y::TableAccessible* aTable) : mTable(aTable) {} what about to put the class under mozilla::a11y namespace? sdn and uia classes and one ia2 class are namespacified

(In reply to alexander :surkov from comment #4) > (In reply to Trevor Saunders (:tbsaunde) from comment #3) > > (In reply to alexander :surkov from comment #2) > > > the first approach seems a little bit crazy but it benefits from memory > > > consumption point of view. I'd like to get feedback from c++ gurus. > > > > So, the reason I went with the extra member approach was basically for the > > same reason we did with xpcAccessibletable. > > ok, what was a reason btw would you remind me? I believe it was that it would be easier to move to seperate objects for xpcom interfaces in the future. > @@ +66,1 @@ > > return E_FAIL; > > i'm curious whether this should be E_INVALIDARG maybe, or maybe S_FALSE, for case of table with different number of cells in each row. > @@ +117,5 @@ > > A11Y_TRYBLOCK_BEGIN > > > > *aDescription = NULL; > > + if (aColIdx < 0) > > + return E_INVALIDARG; > > if you do invalid arg for < 0 then it'd be reasonable if you do it for > > colcount. basically it's applicable to all methods here. Note, > xpcTableAccessible (which was used here) has those checks I was trying to balance making sure that was correct with perf, but if you think its really important I guess we can explicitly check instead of letting the method we call on the table return and probably fail that way. > @@ +346,5 @@ > > + nsAutoTArray<uint32_t, 30> cellIndices; > > + mTable->SelectedCellIndices(&cellIndices); > > + > > + uint32_t maxCells = cellIndices.Length(); > > + if (aMaxChildren > 0 && uint32_t(aMaxChildren) < maxCells) > > I prefer c++ casting style well, that certainly isn't a C cast so I'm not sure what you call it ;) I sort of prefer it to static cast since its shorter. > @@ +623,4 @@ > > > > + *aCells = > > + static_cast<IUnknown**>(::CoTaskMemAlloc(sizeof(IUnknown*) * > > + cells.Length())); > > does it never fail? no, I forgot to add the null chekc > @@ +170,5 @@ > > // nsISupports > > NS_IMETHOD QueryInterface(const nsIID& uuid, void** result) = 0; > > > > +protected: > > + ia2AccessibleTable(mozilla::a11y::TableAccessible* aTable) : mTable(aTable) {} > > what about to put the class under mozilla::a11y namespace? sdn and uia > classes and one ia2 class are namespacified I guess that's fine with me

(In reply to Trevor Saunders (:tbsaunde) from comment #6) > > @@ +66,1 @@ > > > return E_FAIL; > > > > i'm curious whether this should be E_INVALIDARG > > maybe, or maybe S_FALSE, for case of table with different number of cells > in each row. it sounds reasonable but IA2 spec says nothing about S_FALSE for this method. > > @@ +117,5 @@ > > > A11Y_TRYBLOCK_BEGIN > > > > > > *aDescription = NULL; > > > + if (aColIdx < 0) > > > + return E_INVALIDARG; > > > > if you do invalid arg for < 0 then it'd be reasonable if you do it for > > > colcount. basically it's applicable to all methods here. Note, > > xpcTableAccessible (which was used here) has those checks > > I was trying to balance making sure that was correct with perf, but if you > think its really important I guess we can explicitly check instead of > letting the method we call on the table return and probably fail that way. that's how I read IA2 spec. From the perf point of view, you could check boundaries *after* you received data > > @@ +346,5 @@ > > > + nsAutoTArray<uint32_t, 30> cellIndices; > > > + mTable->SelectedCellIndices(&cellIndices); > > > + > > > + uint32_t maxCells = cellIndices.Length(); > > > + if (aMaxChildren > 0 && uint32_t(aMaxChildren) < maxCells) > > > > I prefer c++ casting style > > well, that certainly isn't a C cast so I'm not sure what you call it ;) I > sort of prefer it to static cast since its shorter. yeah, I always stop myself from using implicit casting because it can be used as static and reinterpret casting what makes it not really safe. Also it seems in our code we use static_cast more often (and not because of me :) ).

Comment on attachment 703756 [details] [diff] [review] bug 767272 - don't use nsIAccessibleTable type stuff in ia2AccessibleTable Review of attachment 703756 [details] [diff] [review]: ----------------------------------------------------------------- I think I'd need one more run ::: accessible/src/windows/ia2/ia2AccessibleTable.cpp @@ +47,5 @@ > // IAccessibleTable > > STDMETHODIMP > +ia2AccessibleTable::get_accessibleAt(long aRowIdx, long aColIdx, > + IUnknown **aAccessible) nit: type** @@ +341,5 @@ > { > A11Y_TRYBLOCK_BEGIN > > + *aChildren = NULL; > + *aNChildren = 0; invalid arg if those are nulls? (the same below) @@ +345,5 @@ > + *aNChildren = 0; > + if (!mTable) > + return CO_E_OBJNOTCONNECTED; > + > + nsAutoTArray<uint32_t, 30> cellIndices; nit: extra space after "30>" @@ +347,5 @@ > + return CO_E_OBJNOTCONNECTED; > + > + nsAutoTArray<uint32_t, 30> cellIndices; > + mTable->SelectedCellIndices(&cellIndices); > + nit: whitespaces on empty line @@ +350,5 @@ > + mTable->SelectedCellIndices(&cellIndices); > + > + uint32_t maxCells = cellIndices.Length(); > + if (aMaxChildren > 0 && static_cast<uint32_t>(aMaxChildren) < maxCells) > + maxCells = aMaxChildren; btw, per spec max arguments are ignored (see for example http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/interface_i_accessible_table.html#41743ec3a786776ebb1c227d3bac7859), sorry for the late notice @@ +457,4 @@ > { > A11Y_TRYBLOCK_BEGIN > > + *aIsSelected = false; nit: wrong indent @@ +518,2 @@ > > + if (aColIdx < 0 || static_cast<uint32_t>(aRowIdx) >= mTable->RowCount()) copy/paste issue, replace to aColIdx/ColCount @@ +552,2 @@ > > + if (aColIdx < 0 || static_cast<uint32_t>(aColIdx) >= mtable->ColCount()) mtable -> mTable @@ +578,2 @@ > > + if (aCellIdx < 0 || static_cast<uint32_t>(aColIdx)) second statement is wrong @@ +648,5 @@ > ia2AccessibleTable::get_selectedColumns(long** aColumns, long* aNColumns) > { > A11Y_TRYBLOCK_BEGIN > > + return get_selectedColumns(0, aColumns, aNColumns); I'd prefer to implement these IAccessibleTable2 methods right here rather than implement them as obsolete IAccessibleTable methods virtual calls. ::: accessible/src/windows/ia2/ia2AccessibleTable.h @@ +11,5 @@ > #include "AccessibleTable.h" > #include "AccessibleTable2.h" > > +namespace mozilla { > +namespace a11y { nit: new line should be nice @@ +163,5 @@ > /* [out, size_is(,*nRows)] */ long **selectedRows, > /* [out, retval] */ long *nRows); > > +protected: > + ia2AccessibleTable(mozilla::a11y::TableAccessible* aTable) : mTable(aTable) {} nit: mozilla::a11y is not needed @@ +168,2 @@ > > + mozilla::a11y::TableAccessible* mTable; same

> @@ +341,5 @@ > > { > > A11Y_TRYBLOCK_BEGIN > > > > + *aChildren = NULL; > > + *aNChildren = 0; > > invalid arg if those are nulls? (the same below) well, afaik we don't do that other places so I figured I'd leave that for bug 429xxx to be consistant. > @@ +350,5 @@ > > + mTable->SelectedCellIndices(&cellIndices); > > + > > + uint32_t maxCells = cellIndices.Length(); > > + if (aMaxChildren > 0 && static_cast<uint32_t>(aMaxChildren) < maxCells) > > + maxCells = aMaxChildren; > > btw, per spec max arguments are ignored (see for example > http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/ > interface_i_accessible_table.html#41743ec3a786776ebb1c227d3bac7859), sorry > for the late notice that's ... weird

(In reply to Trevor Saunders (:tbsaunde) from comment #10) > > > + *aChildren = NULL; > > > + *aNChildren = 0; > > > > invalid arg if those are nulls? (the same below) > > well, afaik we don't do that other places so I figured I'd leave that for > bug 429xxx to be consistant. ok, up to you

Comment on attachment 706218 [details] [diff] [review] bug 767272 - don't use nsIAccessibleTable type stuff in ia2AccessibleTable Review of attachment 706218 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/windows/ia2/ia2AccessibleTable.cpp @@ +52,2 @@ > { > + return get_accessibleAt(aRowIdx, aColIdx, aAccessible); I must missing something, how does it work?

(In reply to alexander :surkov from comment #13) > Comment on attachment 706218 [details] [diff] [review] > bug 767272 - don't use nsIAccessibleTable type stuff in ia2AccessibleTable > > Review of attachment 706218 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/src/windows/ia2/ia2AccessibleTable.cpp > @@ +52,2 @@ > > { SS> > + return get_accessibleAt(aRowIdx, aColIdx, aAccessible); > > I must missing something, how does it work? no, I just forgot to replace get_accessibleAt() with get_cellAt() when I copied pasted stuff around to make the AccessibleTable2 methods the ones that didn't call other AccessibleTable methods.

Comment on attachment 706218 [details] [diff] [review] bug 767272 - don't use nsIAccessibleTable type stuff in ia2AccessibleTable Review of attachment 706218 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f0653eede02f - https://tbpl.mozilla.org/php/getParsedLog.php?id=19225385&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=19225985&tree=Mozilla-Inbound say that attributes/test_obj.html was not pleased by your patch.

Comment on attachment 706218 [details] [diff] [review] bug 767272 - don't use nsIAccessibleTable type stuff in ia2AccessibleTable Review of attachment 706218 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: accessible/src/msaa/XULTreeGridAccessibleWrap.cpp @@ +21,5 @@ > +void > +XULTreeGridAccessibleWrap::Shutdown() > +{ > + ia2AccessibleTable::mTable = nullptr; > + XULTreeGridAccessibleWrap::Shutdown(); you do a recursion, that's why tests failed.