Closed
Bug 604293
Opened 14 years ago
Closed 14 years ago
nsIAccessible::GetBounds() should flush layout
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
People
(Reporter: surkov, Assigned: surkov)
References
Details
(Keywords: access)
Attachments
(1 file)
3.09 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter 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?
Assignee | ||
Comment 1•14 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•14 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 3•14 years ago
|
||
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•14 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•14 years ago
|
||
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
Updated•14 years ago
|
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in
before you can comment on or make changes to this bug.
Description
•