333295 - [@ nsDocAccessible::GetBoundsRect] if !vm

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[timeless]

found by coverity

Comment 1

[Mike Gao]

Attachment: The fix of this bug.

I take this bug per Aaron's suggestion. And I got a patch ready for review. But I could not assign the bug to myself;)

Comment 2

[Mike Gao]

Comment on attachment 222042 [details] [diff] [review]
The fix of this bug.

Index: mozilla/accessible/src/base/nsDocAccessible.cpp
===================================================================
RCS file: /cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v
retrieving revision 1.91
diff -p -u -5 -r1.91 nsDocAccessible.cpp
--- mozilla/accessible/src/base/nsDocAccessible.cpp     24 Apr 2006 20:19:44 -0000      1.91
+++ mozilla/accessible/src/base/nsDocAccessible.cpp     15 May 2006 16:30:53 -0000
@@ -505,12 +505,17 @@ void nsDocAccessible::GetBoundsRect(nsRe
       return;
     }
     nsIViewManager* vm = presShell->GetViewManager();

     nsIScrollableView* scrollableView = nsnull;
-    if (vm)
+
+    if (!vm) {
+      return;
+    }
+    else {
       vm->GetRootScrollableView(&scrollableView);
+    }

     nsRect viewBounds(0, 0, 0, 0);
     if (scrollableView) {
       viewBounds = scrollableView->View()->GetBounds();
     }

Comment 3

[Mike Gao]

Attachment: The fix of this bug.

Should be this one..

Comment 4

[Aaron Leventhal]

Comment on attachment 222049 [details] [diff] [review]
The fix of this bug.

r+, but remove the else, because it is not necessary. As a general rule Mozilla code avoids else after return.

Comment 5

[[e-mail]]

Comment on attachment 222049 [details] [diff] [review]
The fix of this bug.

>     nsIViewManager* vm = presShell->GetViewManager();
> 
>     nsIScrollableView* scrollableView = nsnull;
>-    if (vm)
>+
>+    if (!vm) {
>+      return;
>+    }
>+    else {
>       vm->GetRootScrollableView(&scrollableView);
>+    }
As well as what Aaron said, the if (!vm) { return; } belongs just after the construction of vm, rather than just before its use, i.e.

nsIViewManager* vm = presShell->GetViewManager();
if (!vm) {
  return;
}

nsIScrollableView* scrollableView = nsnull;
vm->GetRootScrollableView(&scrollableView);

Comment 6

[Mike Gao]

Attachment: Update according to the comments.

I updated the patch according to your comments. But the previous patch got (super)review+ already. I am not sure if this is necessary.

Comment 7

[Aaron Leventhal]

Comment on attachment 223286 [details] [diff] [review]
Update according to the comments.

r+ as long as you tested it.

Comment 8

[[e-mail]]

Comment on attachment 223286 [details] [diff] [review]
Update according to the comments.

>     nsIViewManager* vm = presShell->GetViewManager();
> 
>+    if (!vm) {
>+      return;
>+    }
Nit: that blank line isn't needed here.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

mozilla/accessible/src/base/nsDocAccessible.cpp 	1.73.2.11