Closed Bug 795192 Opened 7 years ago Closed 7 years ago

don't use xpcom table interfaces in atk code

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
I should probably read over this again, especially since its bin a while since I did most of this, but I think the boardum will kill me.
Attachment #665733 - Flags: review?(surkov.alexander)
Comment on attachment 665733 [details] [diff] [review]
patch

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

::: accessible/src/atk/AccessibleWrap.cpp
@@ +405,5 @@
>      interfacesBits |= 1 << MAI_INTERFACE_HYPERLINK_IMPL;
>  
>    if (!nsAccUtils::MustPrune(this)) {  // These interfaces require children
>      // Table interface.
> +    if (AsTable())

why don't we have IsTable() for tables?

@@ +411,1 @@
>        

nit: remove those whitespaces?

::: accessible/src/atk/nsMaiInterfaceTable.cpp
@@ +17,5 @@
>  using namespace mozilla::a11y;
>  
>  extern "C" {
>  static AtkObject*
> +refAtCB(AtkTable *aTable, gint aRowIdx, gint aColIdx)

nit: type* name (here and below)

@@ +22,3 @@
>  {
>    AccessibleWrap* accWrap = GetAccessibleWrap(ATK_OBJECT(aTable));
> +  if (!accWrap || aRowIdx < 0 || aColIdx < 0)

nit: checking args firstly? (here and below)

@@ +25,3 @@
>      return nullptr;
>  
> +  Accessible* cell = accWrap->AsTable()->CellAt(aRowIdx, aColIdx);

(not looking through the code) I'm curious if there's a chance of failing AsTable().

@@ +35,1 @@
>      return cellAtkObj;

nit: align the whole block?

@@ +215,2 @@
>  {
> +      *aSelected = nullptr;

nit: wrong indent

@@ +223,5 @@
> +  accWrap->AsTable()->SelectedColIndices(&cols);
> +  if (cols.IsEmpty())
> +    return 0;
> +
> +  gint *atkColumns = g_new(gint, cols.Length());

nit: type* name

@@ +234,2 @@
>      *aSelected = atkColumns;
> +    return cols.Length();

nit: wrong offset for these two

@@ +283,5 @@
>    if (!accWrap)
>      return FALSE;
>  
> +  return static_cast<gboolean>(accWrap->AsTable()->
> +      IsCellSelected(aRowIdx, aColIdx));

two spaces offset relative previous line please
Attachment #665733 - Flags: review?(surkov.alexander) → review+
> ::: accessible/src/atk/AccessibleWrap.cpp
> @@ +405,5 @@
> >      interfacesBits |= 1 << MAI_INTERFACE_HYPERLINK_IMPL;
> >  
> >    if (!nsAccUtils::MustPrune(this)) {  // These interfaces require children
> >      // Table interface.
> > +    if (AsTable())
> 
> why don't we have IsTable() for tables?

well, it would use a bit in mFlags and I think that would be the only current use for it.  But no terribly good reason other than that.

> @@ +22,3 @@
> >  {
> >    AccessibleWrap* accWrap = GetAccessibleWrap(ATK_OBJECT(aTable));
> > +  if (!accWrap || aRowIdx < 0 || aColIdx < 0)
> 
> nit: checking args firstly? (here and below)

doesn't really seem like its worth the complication to make us slightly faster in the case caller did something really stupid

> > +  Accessible* cell = accWrap->AsTable()->CellAt(aRowIdx, aColIdx);
> 
> (not looking through the code) I'm curious if there's a chance of failing
> AsTable().
it shouldn't because we only expose the atk table interface if AsTable() succeeds when we create the atk object see the first bit of the patch.
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > >      // Table interface.
> > > +    if (AsTable())
> > 
> > why don't we have IsTable() for tables?
> 
> well, it would use a bit in mFlags and I think that would be the only
> current use for it.  But no terribly good reason other than that.

You could do inline boolean IsTable() { return AsTable(); }
https://hg.mozilla.org/mozilla-central/rev/6852e80e78c8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.