Closed Bug 145864 Opened 22 years ago Closed 22 years ago

Implementation of Basic Object interface for MAI

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: yinbolian, Assigned: yinbolian)

References

Details

(Keywords: access, Whiteboard: need sr=)

Attachments

(2 files, 8 obsolete files)

The basic object interface necessary for AT application to navigate the
accessibility tree includes:

get_name, get_description, get_parent, get_n_children, ref_child,
get_index_in_parent.
Please do not make me the QA contact
QA Contact: jrgm → nobody
Status: NEW → ASSIGNED
Keywords: access
QA Contact: nobody → mindy.liu
Blocks: 145863
Attached patch patch (obsolete) — Splinter Review
The patch does more work than the bug requested:
1. the object interface mentioned in the bug
2. the MaiCache class used for caching later
3. houseclean some debug info, and add a dump virtual func for the purpose
4. The incomplete code for component interface (MaiIfComponent)
5. correct some errors of object ref/unref managment.
Whiteboard: need r=
The patch does more work than the bug requested:
1. the object interface mentioned in the bug
2. the MaiCache class used for caching later
3. houseclean some debug info, and add a dump virtual func for the purpose
4. The incomplete code for component interface (MaiIfComponent)
5. correct some errors of object ref/unref managment.
Summary: Implementation Basic Object interface for MAI → Implementation of Basic Object interface for MAI
Component: XP Toolkit/Widgets → Accessibility APIs
Comment on attachment 84381 [details] [diff] [review]
patch

in MaiIfComponent::getExtenexts has some space issues with args, make sure they
line up.
also, stay away from one letter variable names. - nsMaiTopLevel.cpp
r=jgaunt
Attachment #84381 - Flags: review+
Attachment #84381 - Attachment is obsolete: true
I'm reviewing this patch today.
To start with, I'm looking at what is already in MAI, before I look at this
patch. I wish to fully understand the MAI module.

My first comment is that there aren't enough comments! I wish there was a
comment in most .h and .idl files, saying what the class is for. I also wish
there were some comments describing what some of the methods do. Some of the
methods may not need comments, because it is obvious what they do. However,
there should be some comments on how things are initialized, for example.

I realize that Mozilla doesn't have many comments, but this is an opportunity to
show other people how to make things easy for other developers. :)

The same thing goes for John and myself. If you notice areas in our code that
need more comments or documentation, please tell us :)
I have never seen "If" to mean "Interface". Perhaps you could abbreviate it
"Iface" or something more obvious? Or perhaps no abbreviation at all.

If I am going to understand and review this module, it looks like I will need to
develop a picture of the class/interface hierarchy and brief description of how
it works. My poor little brain is frustrated, because I do not know how
ATK/GTK/Bonobo work.

If you were already planning to do this anyway, then perhaps I can wait until
you are done, and I will look at that first?
Ok, I will change "If" to "Interface" and more comments. Some new code will also
included.  The patch will come soon. :)
Yes, the document about how MAI works will be helpful.  Although I have no
immediate plan to do that, I will try to make a it as soon as possible. 
Attached patch patch v3 (obsolete) — Splinter Review
1. change file name, class name and even variable from "xxIfxx" to
"xxInterfacexx"
2. more comments added for class. It is still far less than enough, I need to
add more later.
3. provide cache for children
Attachment #84872 - Attachment is obsolete: true
In MaiInterfaceComponent::GetExtents(), how are you sure these GTK integer types
are going to be the same as Mozilla's NSPR? Would it be safer to do something like:
    PRInt32 x,y,w,h;
    a->AccGetBounds(&x,&y,&w,&h);
    *pxLeft = x;
    *pyTop = y;
    *pcxWidth = w;
    *pcyHeight = h;
    return S_OK;

What's the difference between methods that have CB at the end of their name, and
other methods? E.g. getExtentsCB() and GetExtents(). Does CB stand for CallBack?
That is another short name that people will not know, like If.

Forgive me, I don't understand how CORBA/Bonobo work. What is the difference
between nsMaiInterface and nsMaiInterfaceComponent. How do they work in the big
picture?

Can you write up something small how this works? Or is there something else you
suggest that I read online that will help?
Here is the type of docs that might be helpful:
http://www.mozilla.org/projects/ui/accessibility/accessible-architecture.html
For example, see the second half of the document that explains how the boot
strapping works, how IAccessible requests are dealt with, and events.

Some documentation to help me understand your patch would be great.

- How does initialization work?
- Where do the requests come in?
- Why are all of these classes necessary? (For example, why do we need both
nsMaiWidget and nsMaiObject?)
- What about events?
I'm still learning from your patch.
I realized that we use "root" to be equivalent to a document or window, and you
re usin root for the application.


howa about renaming to nsMaiAppRoot and nsMaiWindowRoot?
Attached patch add Aaron's comments (obsolete) — Splinter Review
what's changed:
   change the parameters to call nsIAccessible in
nsInterfaceComponent::GetExtents

Aaron, I did not change nsMaiRoot to nsMaiAppRoot as we talked in IRC after a
second consideration. My reason is:
   1. if use nsMaiAppRoot, it seems there is some other things like
nsMaiAppXXX, or nsMaiXXRoot, but there is none of that (there is nsMaiTopLevel,
not nsMaiWindowRoot)

   2. nsMaiRoot instance is root for all other nsMaiXXX instances. Although
nsMaiRoot is related to Application now, but relation is not mandatory. So I
think in concept, nsMaiRoot is better than nsMaiAppRoot

   3. nsMaiRoot is simpler.

Do you think it makes sense? 

Aaron, I will sent the current code of MAI, not only this patch to Bill Haneman
for comments, and cc you. After that, a new patch, and I think a final one,
will come.

thanks,
bolian
Attachment #85222 - Attachment is obsolete: true
I have a question about the string usage in ::GetName() and ::GetDescription()

It seems like there is 1 extra heap allocation that is not needed.
nsCAutoString uses the stack, and then the heap in cases where numChars >64.
This would save a lot of allocations.

Perhaps you could use 
nsCAutoString cName(uniName) 
and then 
atk_object_set_name(atkObject, cName.get()) 
or something like that.

Anything to save the extra heap allocations.
However, I am surprised that ATK does not take unicode names - is the accessible
name never unicode?
Forgive me for a dumb question:
Why is GetName() and others on the class itself, while GetExtents() on the
interface class? Shouldn't all of the ATK methods be on an interface class?
+    /* nsIAccessibles for Different toplevel Windows may have same
+     * Unique ID (why so?), we need to expose only one of them
+     * "ref" here is used to trace that.

We get the unique ID from the DOM Node pointer. The DOM node's pointer should
always be different for each document open in Mozilla.
In MaiWidget::RefChild(gint i)
+    childIndex++;
+    while (childIndex <= (i+1) && accChild) {
+        accChild->GetAccNextSibling(getter_AddRefs(accTmpChild));
+        accChild = accTmpChild;
+        childIndex++;
+    }
can be simplified to:
+    while (childIndex++ <= i && accChild) {
+        accChild->GetAccNextSibling(getter_AddRefs(accTmpChild));
+        accChild = accTmpChild;
+    }
Also, as John Gaunt says, please do not use single letter variable names. They
are much harder to do finds and global replaces on. Use something like |count|
or |index| in loops. For arguments to methods use something descriptive. Also,
in Mozilla we prefix arguments to methods with the letter a, for example
aChildNumber. Please change all of your instances of |gint i| to something more
descriptive?
I think nsMaiRoot is simple, but everywhere else we use "root" in accessibility,
we mean top level. Therefore, in Mai you are different from mozilla/accessible
and mozilla/widget/src/windows. By itself, you make sense. However, it
contradicts our other accessibility work. People who have seen the other
accessibility code will think, as I did, that there is one nsMaiRoot per window.
Aaron, thanks for your comments.

for comments 16: it is good, I will change the code.

for comments 17: In atk, the most important functions, that must be implemented,
are in the atk object itself. All the other interfaces, component, action, etc
are  optional for an accessible object.  So that is whay getExtents is seperate.

for comments 18: for all the nsIAccessible, I use unique ID to check if they are
same.

for comments 19: I will go through all the code to change the one letter variables

for comments 20: I will change nsMaiRoot to nsMaiAppRoot.

Thanks,
Bolian
Aaronl, about your Comment #16, cName.get() can not get a char* type pointer,
which is needed by atk_object_set_name()
so I have to alloc the buffer myself.
Attached patch patch v5 (obsolete) — Splinter Review
changes from the previous patch:
1. file/class  nsMaiRoot ==> nsMaiAppRoot, MaiRoot ==> MaiAppRoot
2. change arguments of funtion to Mozilla style (begin with "a")
3. one letter variable changed to meaningful word (I think it is clean this
time)

Thanks Aaorn for these changes.
Attachment #85388 - Attachment is obsolete: true
I looked up the string stuff. All you need is:
atk_object_set_name(atkObject, NS_CONST_CAST(char*,
NS_ConvertUCS2toUTF8(uniName).get()));
Bolian, why do ATK names use char* instead of unicode?

Anyway, if you fix the string usage to use NS_ConvertUCS2toUTF8, then you can
have r=aaronl

Aaronl, I do not know why it is no unicode in atk, but glib and gtk can support
UTF8, so I think atk can support UTF8. I will make sure this problem anyway.
Attachment #85570 - Attachment is obsolete: true
Attachment #85752 - Flags: review+
Aaronl, below is Bill Haneman comments about unicode support in atk, all other
unicode string should be converted into UTF-8
----------------------------------------------------------

The "char *" in ATK is UTF-8, the API documentation indicates that in a
few places (if this information is missing from some places in the ATK
API docs please let me know so I can add it).

Since UTF-8 uses a multibyte encoding based on 8-bit units, you can
safely treat UTF-8 text as null-terminated char* strings.  Of course you
should use UTF-8 routines for parsing, etc, and these are provided in
glib.

So yes, you can pass any UTF-8 string to ATK as a char*, ATK, Pango, and
GNOME consider all "char*" strings to be UTF-8.  If you have some other
encoding such as UCS-2, you must convert it to UTF-8 before passing it
to ATK.

best regards,

Bill

Comment on attachment 85752 [details] [diff] [review]
patch v6 (fix the string usage to use NS_ConvertUCS2toUTF8)

Bolian, although Aaron promised to give you r= after the minor modification,
you can not give r= to your own patch, it is rule.:(
Aaron: can you give r= to this patch again? thanks
Attachment #85752 - Flags: review+
Comment on attachment 85752 [details] [diff] [review]
patch v6 (fix the string usage to use NS_ConvertUCS2toUTF8)

r=aaronl

(If someone says, "Do this and you have my r=", I think it is okay to mark the
r= yourself after you have done what they asked)
Attachment #85752 - Flags: review+
it seems that I am over stricted:-), thanks, Aaron.
Chris, can you spend some time to review this patch? thanks
Attachment #85752 - Attachment is obsolete: true
Attachment #85998 - Flags: review+
Whiteboard: need r= → need sr=
Comment on attachment 85998 [details] [diff] [review]
(Sorry, this is the fix of string usage to use NS_ConvertUCS2toUTF8)

sr=jst
Attachment #85998 - Flags: superreview+
+#ifdef DEBUG_MAI
+    g_print("Accessibility Shut down\n");
+#endif

Instead of using DEBUG_MAI why don't you use the PR_LOG() logging mechanism?  It
makes debuging a lot easier in the long term.  There's lots of great examples in
the gtk2 code for doing this.

Why aren't you using a hash table for the MaiCache?  Looks like you're walking
arrays by hand.  Hash tables should be a lot faster.  Maybe you're doing this
because then the cache is bounded, but I'm not sure.

+    aClass->get_name = MaiObject::getNameCB;
+    aClass->get_description = MaiObject::getDescriptionCB;
+    aClass->get_parent = MaiObject::getParentCB;
+    aClass->get_n_children = MaiObject::getChildCountCB;
+    aClass->ref_child = MaiObject::refChildCB;
+    aClass->get_index_in_parent = MaiObject::getIndexInParentCB;
+    //  aClass->ref_state_set = MaiObject::refStateSetCB;
+    aClass->initialize = MaiObject::initializeCB;

The classes in this case have C calling conventions.  They are function
pointers.  You're calling into functions that have C++ calling conventions.  On
some platforms with some compilers, C and C++ have different calling
conventions.  This means that the callback that you assign to those classes
needs to have a C convention, not a C++ one.  That's why all of the code in the
gtk2 directory has C type callbacks.

You need to do something like:

foo_cb(GtkFoo *foo, gpointer data)
{
   MaiFoo *bar = (MaiFoo *)data;
   bar->foo();
}

class->foo = foo_cb;

Other than that I don't see any big problems.
blizzard, Thanks for your comments.

1. PR_LOG is great, I will use it.

2. about why I am not using hash table in MaiCache. Hash table do come to me at
first, but I want to control the size of the cache, and I did not find easy ways
to do that with hash table. So changed to Array, can you suggest how to use hash
table with that?

3. I noticed you are using global static functions in widget/src/gtk2 code as
callback function pointers, But since they are not enclose in |extern "C"|, I
think they are almost same with my usage, which is the static member function,
in both cases the C++ linkage will be used. Am I wrong?
move the callback functions into |extern "C"| namespace, they were C++ static
member functions before. This can avoid problem caused by different calling
conventions of C and C++ in some compilers (which complier ???)

Use PR_LOG instead of DEBUG_MAI to log the debug information
Attachment #85998 - Attachment is obsolete: true
Attachment #88422 - Attachment is obsolete: true
has been checked in. Thanks everybody.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
When trying to compile mozilla with gtk2 support and this patch applied, I get
the following error :
nsMaiUtil.cpp: In function `gboolean mai_init()':
nsMaiUtil.cpp:282: `struct MaiHook' has no member named `MaiShutdown'
nsMaiUtil.cpp:283: `struct MaiHook' has no member named `MaiStartup'

Were some modified files forgotten in the patch ?
oops, forget to check in the modification the mai's glue code: nsMaiHook.h.
thanks, Christophe. will checkin soon. 
checked in nsMaiHook.h
and I verified the building gtk2 with MAI, it is OK now.
OK, I turned it back on in the Makefile.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: