78645 - [console] gfx/gfx2 must not print to console in opt builds

Status: VERIFIED FIXED

(Core :: Layout, defect, P4)

Description

[cls]

<formletter>It has been decreed (or requested at any rate) that our release
(read non-debug) builds must not print anything to the console when the app is
running.  See bug 76720 for details.  I have done a preliminary tree scouring
and created mini-patches for each module that has bare printfs.   These patches
are not all inclusive as I didn't even think about xul/js output until post
scour so module owners & peers will still need to scour their modules themselves
as well as make sure the preliminary patches do not break anything.</formletter>

Comment 1

[cls]

Attachment: prelim gfx patch

Comment 2

[cls]

Attachment: prelim gfx2 patch

Comment 3

[cls]

Sorry about the additional spammage but I should clear up a couple of things
before everyone starts replying.

1) I'm just the messenger.  Discussions outside of the specific module/patches
should be discussed in the parent bug ( bug 76720).
2) I have no intention of checking in the patches as is; that's why the bugs are
assigned to someone else ;).
3) The patches are the result of a far & wide-reaching grep across the entire
tree.  They may affect some cases that are not even used and they are far from
optimal.
4) Some platforms/ports will not need the printfs shutoff as they use other
mechanisms to stop the printfs.  That's fine.  Note it in the bug and close it
as invalid(?).  Depending upon the platform/port some people may still be
interested in removing the overhead from the printfs.

Comment 4

[Stuart Parmenter]

for the gfx patch, r=pavlov.

for the gfx2 patch, all of the places where it printfs should be assertions. 
I'll post a new patch for gfx2 shortly...

Comment 5

[Angela Butler]

Per PDT Triage effort: changing the targeted milestone to "mozilla0.9.2".
Please feel free to renominate/address bug if time is available AFTER all
currently targeted 0.9.1 bugs are resolved.
.Angela...

Comment 6

[Jon Granrose]

76720 is targetted for 0.9.1.  can this be moved up?

Comment 7

[chris hofmann]

lets keep dribbling these [console] bugs into the tree as quick as we
can, but they shouldn't hold up or block 0.9.1 so moving the target milestone
to 0.9.2.

Comment 8

[Stuart Parmenter]

in the gfx2 patch, the printfs should be changed to asserts.  intern want this?

r=pavlov

Comment 9

[Gagan]

->jtaylor

Comment 10

[John Taylor]

Attachment: New gfx patch

Comment 11

[John Taylor]

Attachment: New gfx2 patch

Comment 12

[Gagan]

John: The review comments from bug 78665 also apply here. Additionally I noticed 
that you have converted some code in gfxImageFrame.cpp to an assertion but have 
left out the return statement--

-  if (aWidth <= 0 || aHeight <= 0) {
-    printf("error - negative image size\n");
-    return NS_ERROR_FAILURE;
-  }
+  NS_ASSERTION(aWidth <= 0 || aHeight <= 0,"error - negative image size\n");

Assertion is good but you still need a return NS_ERROR_FAILURE. I'd probably 
consider writing this as--

   if (aWidth <= 0 || aHeight <= 0) {
       NS_ASSERTION(0, "error - negative image size\n");
       return NS_ERROR_FAILURE;
   }

Comment 13

[John Taylor]

Attachment: Newer patch for gfx

Comment 14

[John Taylor]

Attachment: Newer patch for gfx2

Comment 15

[Gagan]

What's wrong in this patch--

Index: xlibrgb/xlibrgb.c
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/xlibrgb/xlibrgb.c,v
retrieving revision 1.15
diff -u -r1.15 xlibrgb.c
--- xlibrgb.c   2001/01/14 00:58:06     1.15
+++ xlibrgb.c   2001/06/11 18:44:19
@@ -585,6 +585,7 @@
   pseudo = (visual->class == PseudoColor || visual->class == TrueColor);
 
   if (xlib_rgb_verbose)
+#ifdef DEBUG
     printf ("Visual 0x%x, type = %s, depth = %d, %ld:%ld:%ld%s; score=%x\n",
            (int)visual->visualid,
            visual_names[visual->class],
@@ -594,7 +595,7 @@
            visual->blue_mask,
            sys ? " (system)" : "",
            (quality << 12) | (speed << 8) | (sys << 4) | pseudo);
-  
+#endif
   return (quality << 12) | (speed << 8) | (sys << 4) | pseudo;
 }
 
Here is what's wrong-- the if (xlib_rgb_verbose) should be inside the DEBUG 
check as well. Since you have built this on DEBUG versions you wouldn't have 
caught the problem. Try building on release and you'll see the problem!

Fix that and also keep the indentations/spaces consistent. Not sure if that's 
you or cvs diff... but I'm hoping that we avoid changes in style. 

Comment 16

[John Taylor]

Attachment: GFX patch

Comment 17

[cls]

Wrt attach 37986:

When the else case only contains a printf, I think that you should wrap the
entire else clause in the ifdef DEBUG.  I'm not sure if all compilers support an
empty else clause.  gcc seems to handle the | else {} | case alright but I'm sur
e it and every other compiler will barf on the | else | case. (the xlib portion
of the patch).

Comment 18

[John Taylor]

Attachment: Fix for gfx

Comment 19

[cls]

A big chunk of that patch appears to be whitespace changes across whole files. 
It makes it hard to review the relevant parts.  Can you create the diff with -uw
?  Thanks.

Comment 20

[John Taylor]

Attachment: Fix for gfx with diff -uw

Comment 21

[cls]

There are conflicts in gfx/src/xlib/nsFontMetricsXlib.cpp.  Clean those up and r=cls

Comment 22

[John Taylor]

Attachment: New fix for gfx

Comment 23

[cls]

r=cls

Comment 24

[Christopher Blizzard (:blizzard)]

rs=blizzard

Comment 25

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.2

Comment 26

[John Taylor]

Fix in.

Comment 27

[John Taylor]

...checked in.

Comment 28

[Chris Petersen]

Marking verified per last comments.