103443 - return rv?rv:NS_ERROR_FAILURE; looks ugly

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Bernd]

at 
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableRowGroupFrame.cpp#1734
the following code snippet can be found:

if(NS_FAILED(rv) || !tempParentFrame)
 return rv?rv:NS_ERROR_FAILURE;

this should look like:

if (NS_FAILED(rv))
  return rv;
if (!tempParentFrame)
  return NS_ERROR_FAILURE;

a search at lxr for rv?rv reveals more than 20 hits. I will attach a patch to
bring this search down to zero.

Comment 1

[Bernd]

sorry Marc, this should be my work

Comment 2

[Bernd]

Attachment: the layout part of the work

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 52380 [details] [diff] [review]
the layout part of the work

r=dbaron, although I would prefer if you put the return statements on a
separate line, e.g.
if (NS_FAILED(rv))
  return rv;
rather than
if (NS_FAILED(rv)) return rv;
mainly since that makes it easier to work with in a debugger.

Comment 4

[Bernd]

Attachment: patch with Davids comments

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 52948 [details] [diff] [review]
patch with Davids comments

Except for the missing ')' on the line below, r=dbaron.
>Index: html/base/src/nsTextFrame.cpp
>+    if (NS_FAILED(rv)

Comment 6

[Marc Attinasi]

Comment on attachment 52948 [details] [diff] [review]
patch with Davids comments

Make it compile, and sr=attinasi 

+    if (NS_FAILED(rv)

Comment 7

[Bernd]

Attachment: the editor part

Comment 8

[kinmoz]

Comment on attachment 53605 [details] [diff] [review]
the editor part

There are a couple of places where you made this change:


-  if (NS_FAILED(rv) || !aContent)
-    return rv?rv:NS_ERROR_FAILURE;
-  else
+  if (NS_FAILED(rv))
+    return rv;
+  if (!aContent)
+    return NS_ERROR_FAILURE;
+  
     return NS_OK;


Make sure you outdent the "return NS_OK;" that used to be under the "else".

I probably would've made it look more like ...


-  if (NS_FAILED(rv) || !aContent)
-    return rv?rv:NS_ERROR_FAILURE;
-  else
-    return NS_OK;
+  if (!aContent)
+    return NS_ERROR_FAILURE;
+  
+  return rv;



... though that's not a requirement for checkin.


Thanks for removing this pattern. It's been the source of several bugs in the past!

sr=[e-mail]

Comment 9

[Bernd]

The NS_OK is correctly indented in my source, I posted a diff -uw which may be
confusing.

Comment 10

[Charles Manske]

Comment on attachment 53605 [details] [diff] [review]
the editor part

Note that editor/base is supposed to be removed, so those changes don't need to
be checked in.
All editor files are in the libeditor and composer directories.

Comment 11

[Charles Manske]

r=cmanske

Comment 12

[Bernd]

Attachment: get rid of the remaining occurences

Comment 13

[Marc Attinasi]

Comment on attachment 54677 [details] [diff] [review]
get rid of the remaining occurences

sr=attinasi

Comment 14

[kinmoz]

Comment on attachment 54677 [details] [diff] [review]
get rid of the remaining occurences

r=[e-mail]

Comment 15

[kinmoz]

Comment on attachment 54677 [details] [diff] [review]
get rid of the remaining occurences

r=[e-mail]

Comment 16

[Bernd]

fix checked in