Closed
Bug 145864
Opened 22 years ago
Closed 22 years ago
Implementation of Basic Object interface for MAI
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: yinbolian, Assigned: yinbolian)
References
Details
(Keywords: access, Whiteboard: need sr=)
Attachments
(2 files, 8 obsolete files)
83.01 KB,
patch
|
Details | Diff | Splinter Review | |
522 bytes,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•22 years ago
|
Assignee | ||
Comment 2•22 years ago
|
||
Assignee | ||
Comment 3•22 years ago
|
||
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=
Assignee | ||
Comment 4•22 years ago
|
||
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
Updated•22 years ago
|
Component: XP Toolkit/Widgets → Accessibility APIs
Comment 5•22 years ago
|
||
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+
Assignee | ||
Comment 6•22 years ago
|
||
Attachment #84381 -
Attachment is obsolete: true
Comment 7•22 years ago
|
||
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 :)
Comment 8•22 years ago
|
||
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?
Assignee | ||
Comment 9•22 years ago
|
||
Ok, I will change "If" to "Interface" and more comments. Some new code will also included. The patch will come soon. :)
Assignee | ||
Comment 10•22 years ago
|
||
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.
Assignee | ||
Comment 11•22 years ago
|
||
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
Comment 12•22 years ago
|
||
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?
Comment 13•22 years ago
|
||
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?
Comment 14•22 years ago
|
||
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?
Assignee | ||
Comment 15•22 years ago
|
||
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
Comment 16•22 years ago
|
||
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?
Comment 17•22 years ago
|
||
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?
Comment 18•22 years ago
|
||
+ /* 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.
Comment 19•22 years ago
|
||
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?
Comment 20•22 years ago
|
||
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.
Assignee | ||
Comment 21•22 years ago
|
||
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
Assignee | ||
Comment 22•22 years ago
|
||
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.
Assignee | ||
Comment 23•22 years ago
|
||
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
Comment 24•22 years ago
|
||
I looked up the string stuff. All you need is: atk_object_set_name(atkObject, NS_CONST_CAST(char*, NS_ConvertUCS2toUTF8(uniName).get()));
Comment 25•22 years ago
|
||
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
Assignee | ||
Comment 26•22 years ago
|
||
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.
Assignee | ||
Comment 27•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #85570 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #85752 -
Flags: review+
Assignee | ||
Comment 28•22 years ago
|
||
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 29•22 years ago
|
||
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 30•22 years ago
|
||
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+
Comment 31•22 years ago
|
||
it seems that I am over stricted:-), thanks, Aaron. Chris, can you spend some time to review this patch? thanks
Assignee | ||
Comment 32•22 years ago
|
||
Attachment #85752 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #85998 -
Flags: review+
Assignee | ||
Updated•22 years ago
|
Whiteboard: need r= → need sr=
Comment 33•22 years ago
|
||
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+
Comment 34•22 years ago
|
||
+#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.
Assignee | ||
Comment 35•22 years ago
|
||
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?
Assignee | ||
Comment 36•22 years ago
|
||
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
Assignee | ||
Comment 37•22 years ago
|
||
Attachment #88422 -
Attachment is obsolete: true
Assignee | ||
Comment 38•22 years ago
|
||
has been checked in. Thanks everybody.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 39•22 years ago
|
||
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 ?
Comment 40•22 years ago
|
||
oops, forget to check in the modification the mai's glue code: nsMaiHook.h. thanks, Christophe. will checkin soon.
Assignee | ||
Comment 41•22 years ago
|
||
Comment 42•22 years ago
|
||
checked in nsMaiHook.h and I verified the building gtk2 with MAI, it is OK now.
Comment 43•22 years ago
|
||
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.
Description
•