86517 - Checkin accessibility fixes so we're ready for release

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Aaron Leventhal]

We need to check in accessibility branch Accessible_052901_Branch4

This branch contains interfaces for exposing DOM nodes and documents via
Microsoft COM, so that marshalling can be used to get at data out of process.
Documentation is at www.mozilla.org/projects/ui/accessibility/vendors-win.html.
Has already gone through several layers of API review and approval.

Branch lso contains fixes for HTML selects and combo boxes.

After this branch is checked in, we'll be ready for serious vendor testing.

Comment 1

[Aaron Leventhal]

r= for Eric and John Gaunt's changes in the accessible directory and in
widget\src\windows\Accessible.cpp

Comment 2

[John Gaunt (redfive)]

r=jgaunt for aaronl's accessible changes

adding dependancies on other bugs in the branch, which have their own r=

Comment 3

[Aaron Leventhal]

Attachment: Contains dif of branch tip to branch base

Comment 4

[Chris Waterson]

. Why are you checking in accessible/public/nsIAccessibleDocumentInternal.h?
  It's generated from IDL.

. In nsAccessibilityService::CreateHTMLSelectAccessible(), you have an else-
  after-return here:

   if (*_retval) {
     NS_ADDREF(*_retval);
     return NS_OK;
   } else 
     return NS_ERROR_OUT_OF_MEMORY;

  as well as an unreachable return immediately afterwards!

. Same in CreateHTMLSelectOptionAccessible(). Looks to happen in the rest of
  the file, as well.

. Don't you want to check |d| in nsAccessibilityService::CreateAccessible()?

+  nsCOMPtr<nsIDocument> d (do_QueryInterface(document));
+  if (!document)
+    return NS_ERROR_FAILURE;

. Same in CreateHTMLBlockAccessible().

. You could probably just get rid of this:

+ //printf("################################## CreateHTMLIFrameAccessible\n");
+

. Use modern cast syntax here:

+              *_retval = (nsIAccessible*)frameAcc;

. Ick! Don't copy a bad pattern (returning addref'd pointers). Make it an
  out-parameter to avoid any leaks. Or, don't addref it if you're going to
  return it as the result of a function. (The latter is probably preferrable
  for a helper function like this one.)

+ nsIContent* 
+ nsAccessibilityService::FindContentForDocShell(nsIPresShell* aPresShell,
+                                             nsIContent*   aContent,
+                                             nsIDocShell*  aDocShell)

. See, you leak it right here:

+  nsIContent *tempContent;
+  tempContent = FindContentForDocShell(parentPresShell, rootContent, docShell);

. This is a bit kooky, because it reads like ``if aNode is not an nsIContent'',
  which won't _ever_ be the case in our world...

+  nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));
+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(aNode));
+  if (!content && doc) {

  Either do the test on |aNode|, or add some commentary as to why doing this
  is clever.

. Uh, does this actually compile? If it does, I seriously doubt it does
  what you meant it to do.

+  nsIFrame* frame = nsnull;
+  shell->GetPrimaryFrameFor(content, &&frame);

. In nsAccessible.cpp, don't use ``ns'' as the first letters of a variable
  name; makes it look like a concrete class.

+  nsCOMPtr<nsIAccessible> nsAcc;
+  mAccService->GetAccessibleFor(mPresShell, mDOMNode, getter_AddRefs(nsAcc));

  Why do you need a temporary, anyway? Can't you just read this directly into
  mAccessible? The very next line is...

+  mAccessible = nsAcc;

  If you tell me that |mAccessible| is a bare pointer, I'm going to be very
  scared, because you've just released it, and may well have a dangling
  reference.

. In nsAccessible::AppendFlatStringFromContentNode(), I don't think you want
  to be walking the content model here. You'll miss any content that's been
  generated with pseudo-rules (e.g., :before and :after), and if the catenation
  is important, I'm not sure just testing for a block frame is sufficient to
  properly separate words (e.g., what happens with list items?)

. Urp!

+  return NS_OK;;

. Not sure NS_ERROR_NOT_IMPLEMENTED is the right error code for
  nsLinkableAccessible::GetAccActionName() et. al.? (This usually means,
  ``I haven't gotten around to writing the code yet,'' I thought.)

. What's magic about zero? Need a comment here?

+NS_IMETHODIMP nsLinkableAccessible::AccDoAction(PRUint8 index)
+{
+  if (index == 0) {


That's about all I can do today. I'll pick up with nsGenericAccessible.cpp later.

Comment 5

[Sean Su]

r=,mo=ssu for patch id=39020 for only the part that affects xpinstall

Comment 6

[Aaron Leventhal]

Chris, thanks for the comments.

Here's a point by point reply:

- nsIAccessibleDocumentInternal.h was generated from the .idl, and the .idl was
deleted on purpose. We don't one script writers to use it -- it's only for use
from another .dll that needs to know what nsIDocument is associated with an iframe.

- The else after return, and extra return were indeed naughty, and have been
removed. 
  All similar cases have been changed to:
  if (*_retval) {
    NS_ADDREF(*_retval);
    return NS_OK;
  }    
  return NS_ERROR_OUT_OF_MEMORY;

- Check for |d| fixed, commented printf's removed, modern cast used

- Comment added for if (!content && doc) 
  // This happens when we're on the document node, which will not QI to an
nsIContent, 
  // When that happens, we try to get the outer, parent document node that
contains the document
  // For example, a <browser> or <iframe> element

- AddRef on pointer return removed

- GetPrimaryFrameFor(content, &&frame)  -- Amazingly, this is only in the diff
attachment.  In my source, it's a single ampersand!

- Unnecessary temporary nsAcc removed.

- In nsAccessible::AppendFlatStringFromContentNode(), we're only using it within
specific places, such as an HTML button, that can have arbitrary HTML. The
:before and :after rules will generally work, because we use the frames to get
at most things. It is true that inside a button, if there are :before and :after
rules, we will miss them, but I think we can live with that.

- Added some comments to explain action 0 - It's just that only 1 action had
been implemented for that object - we haven't needed to implement any others yet.

- I thought NS_ERROR_NOT_IMPLEMENTED meant - this method is not implemented -
perhaps on purpose. Is there an error code that means this method is purposely
not implemented on this class?

Aaron

Comment 7

[Chris Waterson]

> nsIAccessibleDocumentInternal.h was generated from the .idl, and the .idl was
> deleted on purpose. We don't one script writers to use it -- it's only for use
> from another .dll that needs to know what nsIDocument is associated with an
> iframe.

So, it's an old-sk00l C++-only XPCOM interface which was generated from IDL to
save some typing. That's fine. Clean out all the XPIDL-generated cruft so that
people like me don't wander around looking for the IDL file, or wondering why a
generated file got checked into the tree.

I'll continue on my accessibility odyssey later today.

Comment 8

[Chris Waterson]

aaronl: would it be possible for you to provide a description of what these
changes accomplish? (e.g., ``Got rid of nsDOMAccessible because its ability to
fluble the outer-most flemson valve is subsumed by nsGenericAccessible'') That
would make it a lot easier to review them.

Comment 9

[Chris Waterson]

1. Hmm. Why doesn't nsIAccessible use attributes? e.g.,

    readonly attribute nsIAccessible accParent;

  instead of

    nsIAccessible getAccParent();

  Ugh, and you've got readers _and_ writers in some places, too! For example,
  this:

    wstring getAccName();
    wstring getAccValue();
    void setAccName(in wstring name);
    void setAccValue(in wstring value);

  Should just be:

    attribute wstring accName;
    attribute wstring accValue;

  Is this a public interface? If so, I _really_ think this should be fixed.
  (FWIW, fixing this should have almost no effect on the C++ code you've
  written.)

2. I've seen pres-shell checks in a number of places; e.g., this, from
   nsHTMLFormControlAccessible.cpp:

@@ -413,9 +416,13 @@
 
   // Get current selection and find out if current node is in it
   nsCOMPtr<nsIPresShell> shell(do_QueryReferent(mPresShell));
+  if (!shell) {
+     return NS_ERROR_FAILURE;  
+  }
+

  What exactly _is_ an nsHTMLFormControlAccessible, and why would it be present
  on an element that has no presentation? FWIW, you later seem to require that
  a pres-shell be present in nsHTMLImageAccessible:

+  nsCOMPtr<nsIPresShell> shell(do_QueryReferent(mPresShell));
+  NS_ASSERTION(shell,"Shell is gone!!! What are we doing here?");
+
+  shell->GetDocument(getter_AddRefs(doc));

  What's up?

[begin perfunctory nags]

3. Have you verified that this stuff doesn't introduce any leaks? Could you
   briefly describe the ownership model of these objects?

4. Have you verified that this stuff has no impact on page load performance
   (as measured by jrgm's harness and iBench)?

[end perfunctory nags]

5. Small stylistic nit. Instead of this:

-              *aAcc = CreateNewAccessible(acc, node, wr);
+              *aAcc = acc;
               NS_IF_ADDREF(*aAcc);

  you can do:

               NS_IF_ADDREF(*aAcc = acc);

  I prefer the latter, but either way is fine and it's not a big deal.

6. I think you want to use NS_DECL_NSISUPPORTS_INHERITED here, in
   nsHTMLIFrameRootAccessible.h:

+class nsHTMLIFrameAccessible : public nsHTMLBlockAccessible, 
+                               public nsIAccessibleDocument,
+                               public nsIAccessibleDocumentInternal,
+                               public nsDocAccessible
 {
+  NS_DECL_ISUPPORTS
+  NS_DECL_NSIACCESSIBLEDOCUMENT
+  NS_DECL_NSIACCESSIBLEDOCUMENTINTERNAL
+

  It's not obvious that nsDocAccessible is a mixin, maybe rename to
  nsDocAccessibleMixin? I was looking for diamond inheritance to
  nsAccessible, and was relieved when I didn't find it! ;-)

  Also, since you derive from nsDocAccessible here, why do you need to
  re-derive from nsIAccessibleDocument and nsIAccessibleDocumentInternal?
  It's probably harmless to do so (except that it might make your vtable
  bigger), but it's also probably unnecessary.

  Which makes me think...

7. Have you compiled your branch on some of our other, more petulant
   compilers? (e.g., AIX, Irix, gcc-2.7.2.3, ...)

8. Use |nsnull| instead of |NULL| here, in nsHTMLImageAccessible.cpp:

+  nsCOMPtr<nsIAccessibilityService>
accService(do_GetService("@mozilla.org/accessibilityService;1"));
+  if (!accService)
+    return NULL;

9. Please change nsHTMLImageAccessible::CreateAreaAccessible() to
   return its result as an out parameter (instead of as a return value)?

10. Should nsMenuListenerAccessible be in its own file? It doesn't seem to make
    a lot of sense to have it in nsHTMLSelectAccessible.cpp, AFAICT. (Or is
    it really an nsSelectMenuListenerAccessible, i.e. specific to the
    select element?)

11. Fast n' loose with the heap, eh? :-) 

+NS_IMETHODIMP nsHTMLSelectAccessible::GetAccLastChild(nsIAccessible **_retval)
+{
+  // create a window accessible
+  *_retval = new nsHTMLSelectWindowAccessible(this, mDOMNode, mPresShell);
+  NS_ADDREF(*_retval);

    Might want to check these, |new| can fail. There are several places
    where this is done.

12. You should assert that you got the frames you think you should've, and
    you should also check that you got a frame at all. This is going to break
    the first time someone makes a major change to the select frame setup,
    if not before (e.g., somebody does a |display: none| on the optgroup or
    something insane).

+NS_IMETHODIMP nsHTMLSelectTextFieldAccessible::GetAccValue(PRUnichar **_retval)
+{
+  nsIFrame* frame = nsAccessible::GetBoundsFrame();
+  nsCOMPtr<nsIPresContext> context;
+  GetPresContext(context);
+  // gets a block frame
+  frame->FirstChild(context, nsnull, &frame);
+
+  // gets the text
+  frame->FirstChild(context, nsnull, &frame);
+
+  // get the texts node
+  nsCOMPtr<nsIContent> content;
+  frame->GetContent(getter_AddRefs(content));
+

    Use the frame-type atoms to determine you've got the right kind of
    frame.

13. Same in nsHTMLSelectTextFieldAccessible::GetBounds(). Don't be walkin'
    the frame hierarchy without checking!

14. #ifdef DEBUG_aaronl this, or just remove it:

+NS_IMETHODIMP nsMenuListenerAccessible::Create(nsIDOMEvent* aEvent)
+{ 
+  mOpen = PR_TRUE;
+#ifdef DEBUG
+  printf("Open\n");
+#endif

    Same in Destroy(), Close()

15. More unchecked frame traversal in:

      nsHTMLSelectButtonAccessible::AccDoAction()
      nsHTMLSelectButtonAccessible::GetBounds()
      nsHTMLSelectWindowAccessible::GetBounds()

    Might want to just move this stuff into a helper method.

16. This should be NS_DECL_NSISUPPORTS_INHERITED, I think.

+class nsHTMLSelectAccessible : public nsAccessible,
+                               public nsIAccessibleSelectable  
+{
+public:
+
+  NS_DECL_ISUPPORTS

    Have you verified that your <select> code doesn't introduce any leaks?

I'll pick back up at accessible/src/nsHTMLTableAccessible.cpp shortly...

Comment 10

[Aaron Leventhal]

Chris,

More responses:

- Many of the changes in the accessible/src directory were necessary as 
we removed delegation and instead used inheritence, which cleaned up our code
considerably. Inheritence also makes it possible for us to provide Sun
Microsystems with different interfaces specialized for each widget type, rather
than just one generic nsIAccessible interface.

1. attributes in .idl. I cannot for the life of me remember why, but Eric
Vaughan had a very good reason for not using attributes. They were messing up
something, such as the ability for us to support these interfaces in XBL, or
something. Sorry I don't remember why.

2. I believe the presShell checks are necessary because a presshell can go away
while accessibility is active. For example, a 3rd party utility may be
attempting to retrieve information about a node or shell that is disappearing as
they ask.

3. First of all, our stuff is only used when 3rd party access software is used.
   When that happens, nsWindow.cpp gets a WM_GETOBJECT message that it generates
   an owner nsRootAccessible for the cross-platform netscape side, and a 
   plain RootAccessible, the owner for the Windows side.
   As long as the window exists, it will hold onto the nsRootAccessible.
   After the window is destroyed, there are no longer any references to that.
   As far as the windows side objects that are create for passing across
   process, it is the responsibility of the calling app to free those objects,
   in accordance to COM rules. We have done leak testing, as you can see
   from the #ifdef DEBUG_LEAKS in various places.

4. We haven't done any performance impact tests. Like I said, this stuff is ony
loaded when a 3rd party accessibility aid is loaded by a user with disabilities,
an extremely rare circumstance.  Barring that, accessibility.dll is not even loaded.

5. As far is NS_IF_ADDREF(*aAcc = acc), I have been trained to never use complex
   expressions inside a C-style Macro.

6. nsDocAccessible -> nsDocAccessibleMixin name changed
   NS_DECL_ISUPPORTS_INHERITED now used.   
   I don't understand your comment about rederiving from nsIAccessibleDocument*,
   because we're not - nsDocAccessible doesn't support those interfaces.  

7. We have not compiled this newer version of the branch on all the strange
compilers, but the older version has been. We'll check some of that out.

8. NULL -> nsnull fixed

9. Why have nsHTMLImageAccssible::CreateAreaAccessible return value as out
parameter when it's only a helper function?

10-16. I'll let John Gaunt comment.

Comment 11

[John Gaunt (redfive)]

Attachment: changes made based on waterson's remarks

Comment 12

[John Gaunt (redfive)]

10. For right now I think it makes sense to leave it where it is. It is
currently only used in SelectAccessibles ( as a parent class ), but it is
generic enough to be used in other accessibles ( possibly in XUL accessibles
when they get here ), so I don't think it should be renamed either.

11. Added checks for the new creations

12,13,15 -- Frame walking issues -- I totally agree. Can you point me to the
Frame-type atoms so I can work up a solution.?

14. Aaron removed the DEBUG, got a couple more in nsMenuListenerAccessible

16. Yes, changed to NS_DECL_NSISUPPORTS_INHERITED and corresponding changes in
the .cpp -- note, i took out the NS_INIT_REFCOUNT from the constructor of
nsHTMLSelectAccessible since we directly call nsAccessible's constructor. There
isn't a need to do this(init the refcount) again is there?

I'd like to ask what the problem with the return in an else statement is? Is it
a clarity issue, performance? I've seen it commented on in a couple of reviews
and never seen the reasoning behind why it is bad.
if ( conditional ) {
do stuff
return;
}
else {
do stuff or not
return;
}

Comment 13

[Chris Waterson]

> I'd like to ask what the problem with the return in an else statement is?
> Is it a clarity issue, performance? I've seen it commented on in a couple
> of reviews and never seen the reasoning behind why it is bad.

It's a non-sequitur. It doesn't bother me that much, but it drives brendan
crazy. Okay, it _does_ bother me a bit when I see this...

  if (/* some condition */) {
    /* insert a ton of code here */
    return;
  }
  else
    /* do something else */;

  /* more code here */

You can see how this is a bone-headed non-sequitur that should be written as:

  if (/* some condition */) {
    /* insert a ton of code here */
    return;
  }

  /* do something else */
  /* more code here */

We'll _always_ ``do something else'' and run ``more code here'' if ``some
condition'' failed. Why separate them?

Comment 14

[Chris Waterson]

1. You leak |path| here. Use nsXPIDLCString.

+NS_IMETHODIMP nsDocAccessible::GetURL(PRUnichar **aURL)
+{  
+  nsCOMPtr<nsIURI> pURI(mDocument->GetDocumentURL());
+  char *path;
+  pURI->GetSpec(&path);
+  *aURL = ToNewUnicode(nsDependentCString(path));
+  return NS_OK;


2. This is silly, you just tested mDocument!

+NS_IMETHODIMP nsDocAccessible::GetDocument(nsIDocument **doc)
+{
+  *doc = mDocument;
+  if (mDocument) {
+    NS_IF_ADDREF(*doc);

3. jst should sr= changes to dom/src. (I personally don't like the commented out
stuff in nsDOMClassInfo.cpp; just remove it if you are going to remove it.)

4. hyatt, blizzard, and saari should _all_ review changes to focus in PresShell.
This is an extremely fragile are that's been broken more often than it's worked.

5. I'm a little nervous about the changes to the combo box frame, mostly because
I don't know the code and believe that it's fairly fragile. Test it extensively.

6. Why are you checking this in commented out? Remove it if you don't need it.
(widget/src/windows/nsAccessible.cpp)

+
+//#undef _WINDOWS_
+//#include "afxcoll.h"
+//#define _WINDOWS_

7. Could you add a comment as to why your chscking in an swath of #if 0'd code
in that file, too?

8. Use nsXPIDLString here. At best, you're using |delete| on something that
probably wants to be |PR_Free|'d, or maybe |delete[]|-ed.

+  if (accDoc) {
+    PRUnichar *pszURL;
+    if (NS_SUCCEEDED(accDoc->GetURL(&pszURL))) {
+      *aURL= ::SysAllocString(pszURL);
+      delete pszURL;

There are a half-dozen functions in DocAccessible that need cleanup.

9. Any one-off non-XPCOM class you've created (like SimpleDOMNode) should
include MOZ_COUNT_[CTOR|DTOR] declarations. Put MOZ_COUNT_CTOR(SimpleDOMNode) in
each ctor, and MOZ_COUNT_DTOR(SimpleDOMNode) in its dtor. This will allow us to
catch leaks on tinderbox.

10. You leak pszAttributeValue here:

+  for (index = 0; index < numAttribs; index++) {
+    aNameSpaceIDs[index] = 0; aAttribValues[index] = aAttribNames[index] = NULL;
+    nsAutoString attributeValue;
+    PRUnichar *pszAttributeValue;
+    const PRUnichar *pszAttributeName; 
+
+    if (NS_SUCCEEDED(content->GetAttributeNameAt(index, nameSpaceID,
*getter_AddRefs(nameAtom), *getter_AddRefs(prefixAtom)))) {
+      aNameSpaceIDs[index] = NS_STATIC_CAST(short, nameSpaceID);
+      nameAtom->GetUnicode(&pszAttributeName);
+      aAttribNames[index] = ::SysAllocString(pszAttributeName);
+      if (NS_SUCCEEDED(content->GetAttribute(nameSpaceID, nameAtom,
attributeValue))) {
+        pszAttributeValue = attributeValue.ToNewUnicode();
+        aAttribValues[index] = ::SysAllocString(pszAttributeValue);
+      }
+    }
+  }

I'd recommend you run your code through Purify, because I'll wager there are
several leaks I've missed.

11. You leak a PresShell here (SimpleDOMNode::GetComputedStyleDeclaration)

+  nsCOMPtr<nsIPresShell> shell;
+  if (doc) 
+    shell = doc->GetShellAt(0);
+

12. Mismatched alloc/free. Use nsString::Recycle() or something. |delete| is
definitely wrong.

+    PRUnichar *pszValue = value.ToNewUnicode();
+    aStyleValues[index] = ::SysAllocString(pszValue);
+    delete pszValue;

13. You leak another pres shell in SimpleDOMNode::MakeSimpleDOMNode.

14. Get kmcclusk to sign-off on your changes to nsWindow.cpp

15. Get ben to sr= your changes to xpfe/browser/resources/content/navigator.xul
and the bindings files.

Meta-comment: I don't have a warm fuzzy feeling about your leak testing, having
found a few leaks just by visual inspection of the code. How much testing has
this gone through, again?

I'd like to see all the above issues addressed, and a new (complete) patch
posted to the bug.

Thanks!

Comment 15

[Chris Waterson]

people just cc'd on this bug, please see where I've asked for your help, above.

Comment 16

[Brendan Eich [:brendan]]

<delurk>
jgaunt: else after return is bad style.  But (not to pick on it) even worse is

  if (/* not some condition */) {
    /* do something else */;
  }
  else {
    /* insert a ton of code here */;
    return;
  }

  /* more code here */;

Such code cries out for if-condition inversion, to return early and cast out the
uncommon case (and just to be less obtuse!).

Non-sequiturs, arbitrary control flow, and redundant tests compound badly over
time as many hands hack on the code piecemeal.  It's happened too much already
in Mozilla.  Readability (ease of mental simulation of code execution based on
rapid reading) declines, which reduces the effectiveness of code review and
strong code ownership.  Motherhood, apple pie, and puppies and kittens suffer
too.

/be
</delurk>

Comment 17

[Johnny Stenback (:jst)]

The changes to nsDOMClassInfo.cpp needs to be modified before this lands, I want
the removal of the CLASSINFO_INTERFACES_ONLY flag to be isolated to only the
classes where it matters for accessibility, and AFAIK it only matters for
element classes. There's already a #define ELEMENT_SCRIPTABLE_FLAGS in
nsDOMClassInfo.cpp so the change should be made there, i.e. something like this:

#define ELEMENT_SCRIPTABLE_FLAGS                                               \
  (NODE_SCRIPTABLE_FLAGS |                                                     \
   nsIXPCScriptable::WANT_POSTCREATE) &                                        \
   ~nsIXPCScriptable::CLASSINFO_INTERFACES_ONLY

Comment 18

[Johnny Stenback (:jst)]

Make that:

#define ELEMENT_SCRIPTABLE_FLAGS                                               \
  ((NODE_SCRIPTABLE_FLAGS | nsIXPCScriptable::WANT_POSTCREATE)                 \
   ~nsIXPCScriptable::CLASSINFO_INTERFACES_ONLY)

As Brendan pointed out in private email.

Also add parens around everything in the definition of NODE_SCRIPTABLE_FLAGS
(w/o screwing up the indentation ;-)

Comment 19

[Johnny Stenback (:jst)]

Eh, hmm, one more try, the above suggestion is missing an '&':

#define ELEMENT_SCRIPTABLE_FLAGS                                               \
  ((NODE_SCRIPTABLE_FLAGS | nsIXPCScriptable::WANT_POSTCREATE) &               \
   ~nsIXPCScriptable::CLASSINFO_INTERFACES_ONLY)

Comment 20

[Aaron Leventhal]

Items 1 & 2, 8-13 were in my code and taken care of.
Will look into running Purify.

Comment 21

[Kevin McCluskey (gone)]

r=[e-mail] for nsWindow.cpp changes

Comment 22

[John Gaunt (redfive)]

Attachment: revised branch to base diff

Comment 23

[Chris Waterson]

I think maybe you guys should merge with the trunk first. There have been some
changes (e.g., static build, nsIDocument whackage) that may impact this patch...

Comment 24

[Aaron Leventhal]

Attachment: Whoops last diff forgot new files. This also optimizes our string usage.

Comment 25

[Aaron Leventhal]

Chris, this last diff includes the changes mentioned here, as well as the
wstring to DOMString IDL optimization you and I talked about. Half as many heap
allocations, because of all the ToNewUnicode() calls removed.

Comment 26

[Aaron Leventhal]

Chris, 

Does your last comment indicate sr= for the trunk?

Aaron

Comment 27

[Chris Waterson]

No, my last comment indicates that I think you should merge your changes with
the trunk and re-diff.

Comment 28

[John Gaunt (redfive)]

Attachment: Diff of Accessible_052901_Branch4 merged with trunk - minus mac specific changes

Comment 29

[John Gaunt (redfive)]

waterson: here is an example of the weird wr stuff: ( from 
nsAccessibilityService.cpp )

 nsCOMPtr<nsIPresShell> tempShell;
 d->GetShellAt(0, getter_AddRefs(tempShell));
 nsCOMPtr<nsIWeakReference> wr(getter_AddRefs(NS_GetWeakReference(tempShell)));

Comment 30

[Chris Waterson]

Okay, round deux. Looks pretty good. Still a few minor comments.

1. Don't mangle spaces in accessible/build/Makefile.in

 SHARED_LIBRARY_LIBS = \
+    $(DIST)/lib/xpcom.lib \
+    $(DIST)/lib/timer_s.lib \
		$(DIST)/lib/libaccessibility_s.$(LIB_SUFFIX) \
		$(DIST)/lib/libchrome_s.$(LIB_SUFFIX) \
+    $(DIST)/lib/contentshared_s.$(LIB_SUFFIX) \
	$(NULL)

2. For the static build to work right, you need to separate
   SUB_LIBRARIES and LLIBS. Specifically, SUB_LIBRARIES should be any
   libraries that your component is ``made up from'', and LLIBS should
   be any libraries that your component depends on at runtime.

   The situation is similar on Unix. Use SHARED_LIBRARY_LIBS for the
   libraries that comprise your component, and EXTRA_DSO_LDOPTS to
   include dependencies that you need at runtime. (cc'ing cls to be
   sure.)

3. I think that the preferred form for this:

+  nsCOMPtr<nsIWeakReference> wr (getter_AddRefs(NS_GetWeakReference(tempShell)));

   is:

   nsCOMPtr<nsIWeakReference> wr = do_GetWeakReference(tempShell);

   scc, dbaron, is that right?

4. nsHTMLCheckboxAccessible::AccDoAction() should comment on magic
   value zero for index.

5. This makes me pine for CopyUTF8toUCS2(), because you'll potentially
   do incorrect conversion on an I18n site. Put an XXX comment here,
   and bug scc!

+NS_IMETHODIMP nsDocAccessibleMixin::GetURL(nsAWritableString& aURL)
+{ 
+  nsCOMPtr<nsIURI> pURI;
+  mDocument->GetDocumentURL(getter_AddRefs(pURI));
+  nsXPIDLCString path;
+  pURI->GetSpec(getter_Copies(path));
+  CopyASCIItoUCS2(nsDependentCString(path), aURL);

6. jst, can you look at the stuff in dom/src again and make sure
   you're happy with what's been done?

7. Get blizzard or hyatt to sr= focus changes in PresShell.

8. How many of these are new files that are going in with the checkin?

? mozilla/widget/src/windows/ISimpleDOMNode.h
? mozilla/widget/src/windows/ISimpleDOMNode_iid.h
? mozilla/widget/src/windows/ISimpleDOMDocument.h
? mozilla/widget/src/windows/ISimpleDOMDocument_iid.h
? mozilla/widget/src/windows/expose/ISimpleDOMDocument/ISimpleDOMDocument_p.c
? mozilla/widget/src/windows/expose/ISimpleDOMDocument/dlldata.c
? mozilla/widget/src/windows/expose/ISimpleDOMDocument/ISimpleDOMDocument_i.c
? mozilla/widget/src/windows/expose/ISimpleDOMDocument/ISimpleDOMDocument.h
? mozilla/widget/src/windows/expose/ISimpleDOMDocument/dlldata.pdb
? mozilla/widget/src/windows/expose/ISimpleDOMDocument/isimpledomdocument_p.pdb
? mozilla/widget/src/windows/expose/ISimpleDOMDocument/isimpledomdocument_i.pdb
? mozilla/widget/src/windows/expose/ISimpleDOMNode/ISimpleDOMNode_p.c
? mozilla/widget/src/windows/expose/ISimpleDOMNode/dlldata.c
? mozilla/widget/src/windows/expose/ISimpleDOMNode/ISimpleDOMNode_i.c
? mozilla/widget/src/windows/expose/ISimpleDOMNode/ISimpleDOMNode.h
? mozilla/widget/src/windows/expose/ISimpleDOMNode/dlldata.pdb
? mozilla/widget/src/windows/expose/ISimpleDOMNode/isimpledomnode_p.pdb
? mozilla/widget/src/windows/expose/ISimpleDOMNode/isimpledomnode_i.pdb

   If none, then be sure to add .cvsignore entries for the files that
   are built on the fly.

9. Use nsnull, not NULL, e.g. here (SimpleDOMNode.cpp):

+  for (index = 0; index < numAttribs; index++) {
+    aNameSpaceIDs[index] = 0; aAttribValues[index] = aAttribNames[index] = NULL;

10. No need to test |doc| here, testing for |shell| is sufficient,
    |doc| == nsnull necessarily implies |shell| == nsnull. This is in
    SimpleDOMNode::GetComputedStyleDeclaration():

+  if (!shell || !doc)
+    return NS_ERROR_FAILURE;   
+

11. Still need ben to sr= changes to navigator.xul and the XBL files.

12. jag said he'd spotted some leaks in the code earlier. jag, can you
    take a look at make sure they've been addressed?

So, conditional sr=waterson based on 

- fixing any problems noted above
- cls signing off on Makefile.in changes
- jst signing off on dom/src changes
- hyatt and/or blizzard sr='ing PresShell focus changes
- ben sr='ing XBL and XUL changes
- jag verifying the leaks he was concerned about

Comment 31

[Johnny Stenback (:jst)]

In nsGlobalWindow.cpp:

+    nsCOMPtr<nsIFocusController>focusController;

add a space between the type and the variable name.

+    GetRootFocusController(getter_AddRefs(focusController));
+    if ( focusController )

loose the spaces inside the parens, no other code in this file uses spaces
inside parens like this.

I'd like to have saari look at the focus related changes to nsGlobalWindow.cpp
before this is checked in, saari?

The changes to nsDOMClassInfo.cpp are ok, modulo some whitespace issues I'll
clean up once this has landed.

Comment 32

[John Gaunt (redfive)]

The XBL and XUL changes have been sr'd by jag in the bug for those ( see bug 
83588 )

The nsPresShell.cpp changes ( which were not in the last diff incidentally ) 
were already checked into the trunk by Hyatt under different bugs ( bug 83220 
and bug 83707 )

made jst's recommendations

fixed all issues listed by waterson

waiting to hear from jag on the leaks and cls on the makefiles. -- I will post a 
revised diff of just our makefile changes, since they have changed a bit, took a 
bunch of stuff out that wasn't needed ( cruft from before ).

Comment 33

[Chris Waterson]

> The XBL and XUL changes have been sr'd by jag in the bug for those ( see bug 
> 83588 )

I'm glad that jag has reviewed the code, but he is not a super-reviewer. 
Please have ben, alecf, or hyatt sr= those changes. (See 
http://www.mozilla.org/hacking/reviewers.html.)

Comment 34

[John Gaunt (redfive)]

whoops mistyped, i meant r=, i'll get ahold of one of the above to take a look.

Comment 35

[David Hyatt]

sr=hyatt on xul stuff, but I don't like the additional methods/properties in 
button.xml.  We shouldn't be adding these methods/properties until we've taken 
care of some of the overhead issues with XBL (mscott's brutal sharing bug) and 
added a way to add conditional or additive XBL so that we don't incur overhead 
when accessibility APIs aren't used.

I'll give an sr on the XML stuff assuming that the accessibility functions are 
removed from button.xml.

Comment 36

[cls]

First, what waterson said is correct: only use SHARED_LIBRARY_LIBS for libraries
that composite your library and EXTRA_DSO_LDOPTS for runtime dependencies.  

Second, did someone build this on unix?  I'm guessing not since there are win32
library names being referenced in accessible/build/Makefile.in .

Third, why are we linking directly against libconshared_s again?  Shouldn't
those functions be referenced via the content component?  Or it it time to break
that out into a separate lib?

Comment 37

[Aaron Leventhal]

I have followed hyatt's instructions.
Now cls's comments appear to be the last bit.

Comment 38

[John Gaunt (redfive)]

Attachment: merged versions of the makefile.win and Makefile.in

Comment 39

[John Gaunt (redfive)]

ignore those makefiles. I just failed to build on win and I see problems in the 
files already. I'll post good ones when I verify they are at least building.

the contentshared_s.lib is in there for the nsIAtom/nsLayoutAtom stuff. It 
wasn't compiling without it, but that was on the branch. I haven't tried the 
trunk w/out that lib.

Comment 40

[jag (Peter Annema)]

in nsAccessibilityService.cpp:

+  nsCOMPtr<nsIPresShell> tempShell;
+  document->GetShellAt(0, getter_AddRefs(tempShell));
+  *aShell = NS_GetWeakReference(tempShell);
+  NS_IF_ADDREF(*aShell);

This is a leak. I believe you fixed this per discussion with dbaron, just
pointing it out in here so it doesn't slip through the cracks.


Try to write explicit comparisons against nsnull like

+                if ( frameAcc != nsnull ) {

as implicit comparisons, i.e.:

+                if (frameAcc) {



After talking to evaughan, I believe in the idl you can now change the explicit
attribute getters and setters to straight attributes.

If possible I'd like to change the variable and function names changed from
|accFoo| to |accessibleFoo|. Yes, it's a little more typing, but it's also more
expressive.


+      nsAutoString text;
+      textContent->CopyText(text);
+      if (text.Length()>0)
+        aFlatString->Append(text);

Instead of |if (str.Length() > 0)| try to use |if (!str.IsEmpty())| which is
potentially cheaper.

Btw, in this specific case, I wonder if doing the test is worth. I'm sure Append
does a similar test in order to see whether it needs to do anything at all, and
even if not, I'd personally simplify these lines and just remove the if.

If you have a couple of Appends in a row you can actually do this:

+      aFlatString->Append(NS_LITERAL_STRING(" ") +
+                          textEquivalent +
+                          NS_LITERAL_STRING(" "));

This will call Append only once and for longer strings will mean it will only
need to reallocate once.


+  if (numChildren == 0) {
+    nsAutoString contentText;
+    AppendFlatStringFromContentNode(aContent, aFlatString);
+    return NS_OK;
+    }

That looks a little strange. That nsAutoString contentText doesn't seem to be
used, and the indentation on the closing bracket is off.


+        mLinkContent->HandleDOMEvent(presContext, &linkClickEvent, 
+          nsnull, NS_EVENT_FLAG_INIT, &eventStatus);

Again indentation, though I believe all that was pointed out before...

Let me go over the rest when I come back.

Comment 41

[John Gaunt (redfive)]

Attachment: LayoutAtom Changed needed for Frame Checking

Comment 42

[John Gaunt (redfive)]

Attinasi: Need review for the last diff posted, changes to nsLayoutAtomList.h 
and nsGfxButtonControlFrame.cpp so I can get the frame type as I walk the frame 
tree in nsHTMLSelectAccessible::nsHTMLSelectButtonAccessible()

This particular frame, and its parent classes, did not implement GetFrameType(), 
so the call was going to nsFrame which returns null and does me no good.

Comment 43

[Marc Attinasi]

You need to include a prototype for the method GetFrameType in
nsGfxButtonControlFrame.h, but other than that it looks good. sr=attinasi

Comment 44

[John Gaunt (redfive)]

whoops, actually the declaration _was_ there, I just missed the file on the 
diff. my bad.

building on Linux with a merged makefile, when it gets past the accessible dir I 
will post it here for review from cls.

Comment 45

[John Gaunt (redfive)]

Ok, the build has completed the accesible directory.

I'll post the bodies of the makefiles as an attachment.

cls, pls review. thanks.

Comment 46

[John Gaunt (redfive)]

Attachment: makefile bodies

Comment 47

[Chris Waterson]

Changes to nsGfxButtonFrame.cpp look fine. In makefile.win...

LLIBS=\
  $(DIST)\lib\xpcom.lib                     \
  $(DIST)\lib\accessibility_s.lib           \
  $(DIST)\lib\timer_s.lib                   \
  $(DIST)\lib\gkgfx.lib                     \
  $(DIST)\lib\contentshared_s.lib           \
  $(LIBNSPR)                                \
  $(NULL)

You don't need accessibility_s.lib here, because you put it into SUB_LIBRARIES.
Also, is contentshared_s.lib still needed? (I didn't check, but did you do the
layout atom hackery we talked about?) Either way, it's not a big deal...

Comment 48

[John Gaunt (redfive)]

right, made that makefile.win change.

It looks like we are waiting for cls for makefiles ( or are you satisfied with 
the latest post ? ) and any leak spottings from jag.

Comment 49

[Aaron Leventhal]

Choffman has given verbal approval for trunk checkin when remaining issues are
reviewed and superreviewed.

Remaining issues:
Removing our tab to URL bar & select fix - 83588 (Blake's fix is better)
--disable-bidi issue - 88509 (it's old, needs to be removed, and conflicts with us) 

Comment 50

[lchiang]

aaron - how should QA verify this fix?  Pls give some suggestions so we can help
you check this out on the trunk prior to any branch checkins.  Thank you.

Comment 51

[Aaron Leventhal]

Oops, now we have 2 conflicting requests.
Choffman has asked us to check this into the branch tonight.
We have tested it as much as possible internally.
Dharma Sarnipalli is the QA for MSAA API stuff.
We need to get this in.

Comment 52

[Peter Van der Beken [:peterv]]

This change:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/build/mac/build_scripts&command=DIFF_FRAMESET&file=MozillaBuildList.pm&rev2=1.99&rev1=1.98
breaks restarting a stopped build (eg for a build error) on Macintosh. You're
not allowed to nest Start/EndBuildModule, your change causes the following
sequence: StartBuildModule("content"), StartBuildModule("accessiblity"),
EndBuildModule("accessiblity"), EndBuildModule("content"). Please explain why
you made this change, so we can try to fix your error.

Comment 53

[saari (gone)]

Oops, I keep doing that... I did that for expediency, getting the build running
and tested on mac ASAP. The reason is in the comment, the accessibility bits
land in the middle of the "layout" dependencies.

Probably best to not have accessibility be a "module" and just drop its contents
in the layout module.

It is a shame we can't nest...

Comment 54

[Simon Fraser [no longer active]]

What exactly are the dependencies? There are, of course, no interdependencies 
between component DLLs, so that makes the build order usually not matter much.

Comment 55

[saari (gone)]

If I remember correctly, accessible depends on gfx, and layout depends on
accessible. jgaunt or aaronl could speak as to why.

Comment 56

[Aaron Leventhal]

John Gaunt is gone until next week, but my understanding is that we need layout
for the layout atoms, so we can test layout type.

Apparantly John was told to use that rather than QueryInterface'ing the frames,
because it was more efficient.

One thing is for certain, having that dependency has been a pain thus far.

Comment 57

[Aaron Leventhal]

Checked into 092 branch

Comment 58

[Simon Fraser [no longer active]]

What was checked into the 0.9.2 branch?

Comment 59

[Simon Fraser [no longer active]]

I fixed bug 89473 on the mac build script bustage.