377800 - Too many warnings in console for accessibility!

Status: RESOLVED WORKSFORME

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

When I run with Inspect or another accessibility tool on Windows, I get a lot of useless error spew.

Comment 1

[Aaron Leventhal]

Attachment: No doubt there is more to clean up, but this is a start

Comment 2

[Aaron Leventhal]

Lets leave this open and go through the remaing warnings before we ship Firefox 3.

We should also check gcc warnings.

Comment 3

[Aaron Leventhal]

Surkov, nsAccessible::GetAttributes() spews a lot of messages here:
  NS_ENSURE_TRUE(attributes, NS_ERROR_FAILURE);

Comment 4

[Ginn Chen]

gcc warnings of a11y module should be gone after checkin of bug 382296, bug 382293.

For debug console warnings,
There're a lot of NS_ENSURE_TRUE(accWrap, nsnull) should be changed to
if (!accWrap) return nsnull;
because accWrap could be null if it's shutdown

Comment 5

[Ginn Chen]

Attachment: patch

1) For MAI callbacks,
change NS_ENSURE_TRUE(accWrap, XXX) to if (!accWrap) return XXX;
We don't need these warnings

2) Merge GetAccessibleWrap and CheckMaiAtkObject
kill console warnings

3) Implement static method GetAtkObject(nsIAccessible *) as surkov's suggest, see bug 382567 comment 3

4) nsAccessibleWrap::FireAtkPropChangedEvent doesn't look correct to me.
I removed some code from it. Anyway, this method will never be called.

5) GetRowDescription is not implemented, so remove the code of getRowDescriptionCB
kill console warnings

6) small logic changes in getNameCB, getDescriptionCB, we don't care empty string here.

Comment 6

[Aaron Leventhal]

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

Having Surkov review should be enough.

Comment 7

[alexander :surkov (:asurkov)]

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

Sorry for delay in review. I have few comments.

> AtkRole
> getRoleCB(AtkObject *aAtkObj)
>+        NS_ASSERTION(nsAccessible::IsTextInterfaceSupportCorrect(accWrap), "Does not support nsIAccessibleText when it should");

nit: is there offset here?

> AtkObject *
> getParentCB(AtkObject *aAtkObj)
>+    if (!aAtkObj->accessible_parent) {

Is it possible accessible parent has been changed but aAtkObj->accessible_parent holds old value?

nsAccessibleWrap::FireAtkPropChangedEvent(nsIAccessibleEvent *aEvent,
>                                           AtkObject *aObject)

>+    //Perhaps need more cases in the future

I would suggest to do not remove the code. This is needed for ATK and IA2 awaits too something similar. We need to file foollowing up bug to support them in gecko.

>Index: nsMaiInterfaceTable.cpp
> const gchar*
> getRowDescriptionCB(AtkTable *aTable, gint aRow)
> {

I would suggest to do not remove this code because I guess it's ok to fail if method is not implementd. We should rather to implement this instead to remove ATK mapping of the method.

Comment 8

[Ginn Chen]

Patch committed.

> Is it possible accessible parent has been changed but
> aAtkObj->accessible_parent holds old value?

I don't think accessible parent could be changed.
I didn't change code logic here.

I leave FireAtkPropChangedEvent and getRowDescriptionCB alone.
Keep in mind, the code in FireAtkPropChangedEvent is probably wrong.
And we don't need this function for general properties, e.g. "accessible-name"
atk_object_set_name already does it for us.

Comment 9

[Marco Zehe (:MarcoZ)]

Closing WFM since we got rid of warnings as the module was developed further.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #9)
> Closing WFM since we got rid of warnings as the module was developed further.

we did that but I'm not sure we don't have them a lot anymore. Marco did you have a change to check the console?