Trevor Saunders (:tbsaunde) Reporter
	Description • 9 years ago

the methods in nsMaiInterfacetable.cpp need to use the table methods on ProxyAccessible when the accessible is a proxy.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 1 • 9 years ago

The file has some functions returning AtkObject* without g_object_ref, yet some do call that.
It is not clear to me what the refcnt model is here.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 2 • 9 years ago

This is missing 3 functions, since those return AtkObject* without g_object_ref.
I think those are buggy, but perhaps there is some odd special case here.
Anyhow, those 3 functions could be fixed in a followup patch.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 3 • 9 years ago

or does "ref" in a method name in gtk world mean that g_object_ref should be called, and otherwise just raw pointers can be returned?

	Trevor Saunders (:tbsaunde) Reporter
	Comment 5 • 9 years ago

Comment on attachment 8669609 [details] [diff] [review]
v2

> getSelectedRowsCB(AtkTable *aTable, gint **aSelected)
> {
>+  nsAutoTArray<uint32_t, 10> rows;
>   AccessibleWrap* accWrap = GetAccessibleWrap(ATK_OBJECT(aTable));
>-  if (!accWrap)
>+  if (accWrap) {
>+    accWrap->AsTable()->SelectedRowIndices(&rows);
>+  } else if (ProxyAccessible* proxy = GetProxy(ATK_OBJECT(aTable))) {
>+    proxy->TableSelectedRowIndices(&rows);
>+  } else {
>     return 0;
>-
>-  nsAutoTArray<uint32_t, 10> rows;
>-  accWrap->AsTable()->SelectedRowIndices(&rows);
>+  }

this is missing the check we found a selected row before trying to allocate that is in the column version.

> isColumnSelectedCB(AtkTable *aTable, gint aColIdx)
> {
>   AccessibleWrap* accWrap = GetAccessibleWrap(ATK_OBJECT(aTable));
>-  if (!accWrap)
>-    return FALSE;
>+  if (accWrap) {
>+    return static_cast<gboolean>(accWrap->AsTable()->IsColSelected(aColIdx));
>+  } else if (ProxyAccessible* proxy = GetProxy(ATK_OBJECT(aTable))) {
>+    return proxy->TableColumnSelected(aColIdx);
>+  }
> 
>-  return static_cast<gboolean>(accWrap->AsTable()->IsColSelected(aColIdx));
>+  return FALSE;

I'd say if we need the cast in one case we need it in the rest, but I'm not really convinced they are needed at all.

> static gboolean
> isCellSelectedCB(AtkTable *aTable, gint aRowIdx, gint aColIdx)
> {
>   AccessibleWrap* accWrap = GetAccessibleWrap(ATK_OBJECT(aTable));
>-  if (!accWrap)
>-    return FALSE;
>-
>+  if (accWrap) {
>     return static_cast<gboolean>(accWrap->AsTable()->
>       IsCellSelected(aRowIdx, aColIdx));
>+  } else if (ProxyAccessible* proxy = GetProxy(ATK_OBJECT(aTable))) {
>+    return proxy->TableCellSelected(aRowIdx, aColIdx);

same, and below.

>+DocAccessibleChild::RecvAtkTableColumnHeader(const uint64_t& aID,
>+                                             const int32_t& aCol,
>+                                             uint64_t* aHeader,
>+                                             bool* aOk)
>+{
>+  *aHeader = 0;
>+  *aOk = false;
>+
>+#ifdef MOZ_ACCESSIBILITY_ATK
>+  TableAccessible* acc = IdToTableAccessible(aID);
>+  if (acc) {
>+    Accessible* header = AccessibleWrap::GetColumnHeader(acc, aCol);
>+    if (header) {
>+      *aHeader = reinterpret_cast<uint64_t>(header->UniqueID());
>+      *aOk = true;
>+    }
>+  }
>+#endif

There's really nothing atk specific about this, and it wouldn't be the first time that a generic interface was only used on one platform, so I'd prefer making these be methods on TableAccessible and not ifdefing, but meh I'm feeling nice today so whatever

>   prio(high) sync TableUnselectRow(uint64_t aID, uint32_t aRow);
>   prio(high) sync TableIsProbablyForLayout(uint64_t aID) returns(bool aForLayout);
>+  prio(high) sync AtkTableColumnHeader(uint64_t aID, int32_t aCol) returns(uint64_t aHeaderID, bool aOk);
>+  prio(high) sync AtkTableRowHeader(uint64_t aID, int32_t aRow) returns(uint64_t aHeaderID, bool aOk);

over long lines?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 6 • 9 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> this is missing the check we found a selected row before trying to allocate
> that is in the column version.
I don't understand this comment



> I'd say if we need the cast in one case we need it in the rest, but I'm not
> really convinced they are needed at all.
Oh, sure, I'll add the static_cast for consistency

> 
> There's really nothing atk specific about this,
Other platforms don't have this behavior, so I definitely want to keep this in atk land.
Any reason why not? Why should the code be compiled on all the platforms but used only in atk?

	Pulsebot
	Comment 8 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a319d7355228

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 9 • 9 years ago

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