nsIAccessible::GetBounds() should flush layout

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Disability Access APIs
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access})

unspecified
mozilla2.0b7
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
Created attachment 483085 [details] [diff] [review]
patch

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?
(Assignee)

Comment 1

7 years ago
(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
(Assignee)

Comment 2

7 years ago
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+
(Assignee)

Comment 4

7 years ago
(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
(Assignee)

Comment 5

7 years ago
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/8fab49d206d7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.