80505 - Move query interfaces to GetAccessible on nsIFrame

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Eric Vaughan]

nsIFrame should have a method called GetAccessible instead of query interfacing 
to a nsIAccessible.

Comment 1

[Eric Vaughan]

Enclosed is a patch that does 2 things:

1) Added GetAccessible to nsIFrame and add AccGetDOMNode to nsIAccessible
   so we don't have to do wierd query interfaces.

2) Fixed a major bug in accessibilies bounds code

Comment 2

[Eric Vaughan]

Attachment: Patch

Comment 3

[Aaron Leventhal]

In nsFrameTreeWalker, make sure to use
nsCOMPtr<nsIWeakReference> wr(NS_GetWeakReference .....
instead of using wr = NS_GetWeakReference ....
In fact, I see a bunch of other similar places, where you initialize an nsCOMPtr 
with = . You can just search the dif for " = do_Query" to find most of them.

Other than that, r=aaronl

Comment 4

[Eric Vaughan]

Brendan can I get a SR?

Comment 5

[Peter Trudelle]

retargetting to 0.9.2 because 0.9.1 is now restricted to beta-stoppers. If you
get SR, you can still checked this in without any special approval through Friday.  

Comment 6

[Brendan Eich [:brendan]]

nsWeakPtr is short for nsCOMPtr<nsIWeakReference>, FYI.

+   * Called to retrieve this frames accessible.

Apostrophe for possessive "frame's" please.

Why use a reference to an nsIFrame pointer for the result param, when methods
such as nsIFrame::GetParent use a proper XPCOM nsIFrame** out param pointer? 
But wait!  Why not use the return value instead of wasting it on void:

+void nsFrameTreeWalker::GetCommonParent(nsIFrame* aStartParent, nsIFrame*
aChild, nsIFrame*& aCommonParent)
+{
+  // go up our parent chain until we hit the common parent
+  nsIFrame* parent = nsnull;
+  aChild->GetParent(&parent);

Brace style varies from next-line-after-controlling-keyword to same-line-as...:

+  while(parent)
+  {
+    // if we find one return.
+    if (parent == aStartParent) {
+      aCommonParent = aStartParent;
+      return;
+    }

Nit, worth straightening out for the next hacker who comes along?  Anyway, use
the return value, it's faster and clearer than a void method returning a pointer
via a reference.  Same for GetBounds, which can simply propagate the r.v.

Move nsCOMPtr<nsIAccessible> accessible; down into the if (frame) then-block:


+        nsCOMPtr<nsIAccessible> accessible;
+        if (frame) {
+          frame->GetAccessible(getter_AddRefs(accessible));
+
+          if (!accessible)
+            accessible = do_QueryInterface(content);

So do content nodes also break the rules of COM by returning an nsIAccessible
which cannot be QI'd to the same nsISupports as the content node?  I don't see
any content QI impls returning nsIAccessible yet -- future-proofing?  Shouldn't
content have a GetAccessible _a la_ frame?

Why does "Acc" occur variously at the front and in the second (first noun) word
position in nsIAccessible method identifiers?

 nsresult nsAccessible::GetAccParent(nsIAccessible **  aAccParent)
+NS_IMETHODIMP nsAccessible::AccGetDOMNode(nsIDOMNode **_retval)

The naming convention rules are too subtle for me.  Why do you need Acc at all,
if nsIAccessible is not implemented by nsIFrame implementors (there should be no
GetParent collision, e.g.).

Indentation for the first level jumped to four spaces here, then returned to two
for higher levels:

+NS_IMETHODIMP nsImageFrame::GetAccessible(nsIAccessible** aAccessible)
+{
     nsresult rv;
     NS_WITH_SERVICE(nsIAccessibilityService, accService,
"@mozilla.org/accessibilityService;1", &rv);
     if (accService) {
       nsIAccessible* acc = nsnull;
-      accService->CreateHTMLImageAccessible(NS_STATIC_CAST(nsIFrame*,
this),&acc);
-      *aInstancePtr = acc;
-      return NS_OK;
+      return accService->CreateHTMLImageAccessible(NS_STATIC_CAST(nsIFrame*,
this), aAccessible);
     }
-    return NS_ERROR_FAILURE;
-  } 
 
-  return NS_NOINTERFACE;
+    return NS_ERROR_FAILURE;
 }

Further on, first level is 2 but second jumps to 4 before returning to 2:

+NS_IMETHODIMP nsTextFrame::GetAccessible(nsIAccessible** aAccessible)
+{
+  if (mRect.width > 0 || mRect.height > 0) {
+      nsresult rv;
+      NS_WITH_SERVICE(nsIAccessibilityService, accService,
"@mozilla.org/accessibilityService;1", &rv);
+      if (accService) {
+        nsIAccessible* acc = nsnull;
+        return accService->CreateHTMLTextAccessible(NS_STATIC_CAST(nsIFrame*,
this), aAccessible);
+      }
+  }
+  return NS_ERROR_FAILURE;
+}
+

Yoiks!  Even later, nsHTMLFrameOuterFrame::GetAccessible,
nsComboboxControlFrame::GetAccessible, nsGfxButtonControlFrame::GetAccessible,
nsGfxCheckboxControlFrame::GetAccessible, nsGfxRadioControlFrame::GetAccessible,
nsGfxTextControlFrame::GetAccessible, nsGfxTextControlFrame2::GetAccessible,
nsHTMLButtonControlFrame::GetAccessible, nsImageControlFrame::GetAccessible,
nsTableCellFrame::GetAccessible, nsTableOuterFrame::GetAccessible,  suffer from
the 4-2-... schizophrenia.

BTW, else after return here makes no sense:

+++ nsTableCellFrame.cpp        2001/05/12 21:26:59
@@ -1174,19 +1174,21 @@
   if (aIID.Equals(NS_GET_IID(nsITableCellLayout))) {
     *aInstancePtr = (void*) (nsITableCellLayout *)this;
     return NS_OK;
-  } else if (aIID.Equals(NS_GET_IID(nsIAccessible))) {
-    nsresult rv = NS_OK;
+  } else {
+    return nsHTMLContainerFrame::QueryInterface(aIID, aInstancePtr);
+  }
+}

Get rid of that last else and the braces and overindentation of the final
return.  Same for nsTableOuterFrame::QI.

Nice IBMBIDI #ifdef cleanup in nsTextFrame::QI, though -- who reviewed that?!

How about a new patch with these cleanups?

/be

Comment 7

[Eric Vaughan]

nsWeakPtr is short for nsCOMPtr<nsIWeakReference>, FYI.

YEP I KNOW BUT I USE "nsCOMPtr<nsIWeakReference>" AS WELL AS "nsIWeakReference*" 
 I REALLY DIDN'T WANT TO MIX "nsWeakPtr" IN AND MAKE IT REALLY CONFUSING.

> +   * Called to retrieve this frames accessible.
>
> Apostrophe for possessive "frame's" please.

FIXED

>Why use a reference to an nsIFrame pointer for the result param, when methods
>such as nsIFrame::GetParent use a proper XPCOM nsIFrame** out param pointer? 
>But wait!  Why not use the return value instead of wasting it on void:

FIXED TO USE **

I DIDN'T USE THE FRAME AS A RETURN VALUE IN CASE LATER WE WANT TO PROPAGATE
AN ERROR VIA nsresult. 

Move nsCOMPtr<nsIAccessible> accessible; down into the if (frame) then-block:


+        nsCOMPtr<nsIAccessible> accessible;
+        if (frame) {
+          frame->GetAccessible(getter_AddRefs(accessible));
+
+          if (!accessible)
+            accessible = do_QueryInterface(content);

FIXED

So do content nodes also break the rules of COM by returning an nsIAccessible
which cannot be QI'd to the same nsISupports as the content node?  I don't see
any content QI impls returning nsIAccessible yet -- future-proofing?  Shouldn't
content have a GetAccessible _a la_ frame?

NO THEY DON'T BREAK THE RULES. DOM NODES ARE REFCOUNTED AND CAN IMPLEMENT THE 
ACCESSIBLE INTERFACE VIA XBL. I'M ASSUMING THAT XBL DOES THE RIGHT THING 
UNDERNEATH AND FOLLOWS COM RULES.

Why does "Acc" occur variously at the front and in the second (first noun) word
position in nsIAccessible method identifiers?

ITS DIFFERENT BECAUSE THESE ARE GETTERS. GETTERS ALWAYS PUT THE GET IN FRONT. IF 
I MADE "AccParent" AN ATTRIBUTE THE "GET" WOULD BE ADDED IN FRONT I WANT TO 
ABILITY TO CHANGE THIS BACK TO ATTRIBUTES AT A LATER DATE

 nsresult nsAccessible::GetAccParent(nsIAccessible **  aAccParent)
+NS_IMETHODIMP nsAccessible::AccGetDOMNode(nsIDOMNode **_retval)

The naming convention rules are too subtle for me.  Why do you need Acc at all,
if nsIAccessible is not implemented by nsIFrame implementors (there should be no
GetParent collision, e.g.).

ACC IS USED TO AVOID METHOD CONFLICTS. ORIGINALLY FRAMES DID IMPLEMENT THIS 
INTERFACE AND A DOM NODE COULD IMPLEMENT THEM AS WELL CAUSING CONFLICTS. MSAA 
ALSO DOES THIS IN THEIR INTERFACE FOR THE SAME REASON.

Indentation for the first level jumped to four spaces here, then returned to two
for higher levels:

+NS_IMETHODIMP nsImageFrame::GetAccessible(nsIAccessible** aAccessible)
+{
     nsresult rv;
     NS_WITH_SERVICE(nsIAccessibilityService, accService,
"@mozilla.org/accessibilityService;1", &rv);
     if (accService) {
       nsIAccessible* acc = nsnull;
-      accService->CreateHTMLImageAccessible(NS_STATIC_CAST(nsIFrame*,
this),&acc);
-      *aInstancePtr = acc;
-      return NS_OK;
+      return accService->CreateHTMLImageAccessible(NS_STATIC_CAST(nsIFrame*,
this), aAccessible);
     }
-    return NS_ERROR_FAILURE;
-  } 
 
-  return NS_NOINTERFACE;
+    return NS_ERROR_FAILURE;
 }

Further on, first level is 2 but second jumps to 4 before returning to 2:

+NS_IMETHODIMP nsTextFrame::GetAccessible(nsIAccessible** aAccessible)
+{
+  if (mRect.width > 0 || mRect.height > 0) {
+      nsresult rv;
+      NS_WITH_SERVICE(nsIAccessibilityService, accService,
"@mozilla.org/accessibilityService;1", &rv);
+      if (accService) {
+        nsIAccessible* acc = nsnull;
+        return accService->CreateHTMLTextAccessible(NS_STATIC_CAST(nsIFrame*,
this), aAccessible);
+      }
+  }
+  return NS_ERROR_FAILURE;
+}
+

Yoiks!  Even later, nsHTMLFrameOuterFrame::GetAccessible,
nsComboboxControlFrame::GetAccessible, nsGfxButtonControlFrame::GetAccessible,
nsGfxCheckboxControlFrame::GetAccessible, nsGfxRadioControlFrame::GetAccessible,
nsGfxTextControlFrame::GetAccessible, nsGfxTextControlFrame2::GetAccessible,
nsHTMLButtonControlFrame::GetAccessible, nsImageControlFrame::GetAccessible,
nsTableCellFrame::GetAccessible, nsTableOuterFrame::GetAccessible,  suffer from
the 4-2-... schizophrenia.

ALL FIXED!

BTW, else after return here makes no sense:

YES THIS IS ALL OVER LAYOUT. MOST OF THEM WERE ALREADY THERE BEFORE I ADDED MY 
CODE IN. :-(

+++ nsTableCellFrame.cpp        2001/05/12 21:26:59
@@ -1174,19 +1174,21 @@
   if (aIID.Equals(NS_GET_IID(nsITableCellLayout))) {
     *aInstancePtr = (void*) (nsITableCellLayout *)this;
     return NS_OK;
-  } else if (aIID.Equals(NS_GET_IID(nsIAccessible))) {
-    nsresult rv = NS_OK;
+  } else {
+    return nsHTMLContainerFrame::QueryInterface(aIID, aInstancePtr);
+  }
+}

Get rid of that last else and the braces and overindentation of the final
return.  Same for nsTableOuterFrame::QI.

Nice IBMBIDI #ifdef cleanup in nsTextFrame::QI, though -- who reviewed that?!

THE IBMBIDI STUFF WAS JUST THERE. SO I JUST MADE SURE MY CODE DIDN'T CHANGE ITS 
LOGIC.

How about a new patch with these cleanups?

NEW DIFF ATTACHED!

Comment 8

[Eric Vaughan]

Attachment: Updated patch

Comment 9

[Brendan Eich [:brendan]]

OK! er, ok!  Thanks for fixing those nits.

sr=[e-mail]

/be

Comment 10

[Eric Vaughan]

fixed

Comment 11

[Eric Vaughan]

a

Comment 12

[Chris Petersen]

Marking verified per last comments