Comment on attachment 700783 [details] [diff] [review] remove nsIWinAccessNode and stuff from ia2AccessibleTableCell Review of attachment 700783 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/windows/ia2/ia2AccessibleTableCell.cpp @@ +46,5 @@ > ia2AccessibleTableCell::get_table(IUnknown** aTable) > { > A11Y_TRYBLOCK_BEGIN > > + *aTable = NULL; nit: wrong indent @@ +84,5 @@ > long* aNColumnHeaderCells) > { > A11Y_TRYBLOCK_BEGIN > > + *aCellAccessibles = NULL; nit: wrong indetn @@ +95,5 @@ > + > + *aNColumnHeaderCells = cells.Length(); > + *aCellAccessibles = > + static_cast<IUnknown**>(::CoTaskMemAlloc(sizeof(IUnknown*) * > + cells.Length())); null check? @@ +99,5 @@ > + cells.Length())); > + > + for (uint32_t i = 0; i < cells.Length(); i++) { > + AccessibleWrap* cell = static_cast<AccessibleWrap*>(cells[i]); > + (*aCellAccessibles[i] = static_cast<IAccessible*>(cell))->AddRef(); I prefer having AddRef() as separate statement (here and below) @@ +143,5 @@ > long* aNRowHeaderCells) > { > A11Y_TRYBLOCK_BEGIN > > + *aCellAccessibles = NULL; nit: wrong indent @@ +154,5 @@ > + > + *aNRowHeaderCells = cells.Length(); > + *aCellAccessibles = > + static_cast<IUnknown**>(::CoTaskMemAlloc(sizeof(IUnknown*) * > + cells.Length())); null check, right? @@ +189,5 @@ > boolean* aIsSelected) > { > A11Y_TRYBLOCK_BEGIN > > + *aRowIdx = *aColIdx = *aRowExtents = *aColExtents = 0; nit: wrong indent ::: accessible/src/windows/ia2/ia2AccessibleTableCell.h @@ +15,5 @@ > +namespace mozilla { > +namespace a11y { > +class TableCellAccessible; > +} > +} please put it under a11y namespace as well @@ +66,5 @@ > +protected: > + ia2AccessibleTableCell(mozilla::a11y::TableCellAccessible* aTableCell) : > + mTableCell(aTableCell) {} > + > + mozilla::a11y::TableCellAccessible* mTableCell; note, in that case you don't need these prefixes