Closed Bug 604293 Opened 14 years ago Closed 14 years ago

nsIAccessible::GetBounds() should flush layout

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(1 file)

Attached patch patchSplinter Review
nsIAccessible::GetBounds() should flush layout to make sure we return correct accessible size. The code was moved out from the patch of bug 570275 since they are separate issues, even they are close.
Attachment #483085 - Flags: review?(bolterbugz)
Attachment #483085 - Flags: approval2.0?
(In reply to comment #0)

> The code was moved out from the patch of bug 570275 since they
> are separate issues, even they are close.

marking blocking bug 570275
Blocks: 570275
Comment on attachment 483085 [details] [diff] [review]
patch

cancelling approval per blocking status of bug 570275 which is blocking.
Attachment #483085 - Flags: approval2.0?
Comment on attachment 483085 [details] [diff] [review]
patch

r=me with optional nits:

>diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base/nsAccessible.cpp
>--- a/accessible/src/base/nsAccessible.cpp
>+++ b/accessible/src/base/nsAccessible.cpp
>@@ -980,49 +980,59 @@ void nsAccessible::GetBoundsRect(nsRect&
>     iterContent = nsnull;
>     if (depth == 0)
>       iterContent = iterFrame->GetContent();
>   }
> }
> 
> 
> /* void getBounds (out long x, out long y, out long width, out long height); */
>-NS_IMETHODIMP nsAccessible::GetBounds(PRInt32 *x, PRInt32 *y, PRInt32 *width, PRInt32 *height)
>+NS_IMETHODIMP
>+nsAccessible::GetBounds(PRInt32* aX, PRInt32* aY,
>+                        PRInt32* aWidth, PRInt32* aHeight)

Nit: I don't think the loss of blame history is worth the param rename; at least until we get a better code forensics interface.

> {
>+  NS_ENSURE_ARG_POINTER(aX);
>+  *aX = 0;
>+  NS_ENSURE_ARG_POINTER(aY);
>+  *aY = 0;
>+  NS_ENSURE_ARG_POINTER(aWidth);
>+  *aWidth = 0;
>+  NS_ENSURE_ARG_POINTER(aHeight);
>+  *aHeight = 0;

I realize why you did this order, but would it be nice as:
NS_ENSURE_ARG_POINTER(aX);
NS_ENSURE_ARG_POINTER(aY);
NS_ENSURE_ARG_POINTER(aWidth);
NS_ENSURE_ARG_POINTER(aHeight);
*aX = *aY = *aWidth = *aHeight = 0;

(Up to you)

>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;

Why not check this first?

>+
>+  // Flush layout so that all the frame construction, reflow, and styles are
>+  // up-to-date since we rely on frames, and styles when calculating state.
>+  // We don't flush the display because we don't care about painting.
>+  nsCOMPtr<nsIPresShell> presShell = GetPresShell();
>+  presShell->FlushPendingNotifications(Flush_Layout);
>+
>   // This routine will get the entire rectange for all the frames in this node

Might as well fix the spelling of rectangle here.
Attachment #483085 - Flags: review?(bolterbugz) → review+
(In reply to comment #3)

> Nit: I don't think the loss of blame history is worth the param rename; at
> least until we get a better code forensics interface.

Common, we do this. We look at code everyday but look at history when we don't understand the code, it's much rare case.

> I realize why you did this order, but would it be nice as:
> NS_ENSURE_ARG_POINTER(aX);
> NS_ENSURE_ARG_POINTER(aY);
> NS_ENSURE_ARG_POINTER(aWidth);
> NS_ENSURE_ARG_POINTER(aHeight);
> *aX = *aY = *aWidth = *aHeight = 0;
> 
> (Up to you)

yes, my approach is correct but your is nicer, let's follow correct way.

> >+
> >+  if (IsDefunct())
> >+    return NS_ERROR_FAILURE;
> 
> Why not check this first?

we need to check arguments before we do anything.

> >   // This routine will get the entire rectange for all the frames in this node
> 
> Might as well fix the spelling of rectangle here.

ok
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/8fab49d206d7
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.