78665 - [console] gtk widgets must not print to console in opt builds

Status: RESOLVED FIXED

(Core :: XUL, 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 patch

Comment 2

[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 3

[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 4

[Jon Granrose]

76720 is targetted for 0.9.1, can we move this up?

Comment 5

[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 6

[Stuart Parmenter]

r=pavlov.  gagan, give this to one of our lucky interns.  a free trip for one 
to printf removal camp for a week!

Comment 7

[Gagan]

lets try jtaylor

Comment 8

[John Taylor]

Attachment: New patch. Waiting for review

Comment 9

[Gagan]

john: few comments here--

1. You have moved some of the #if 0 code to #ifdef DEBUG... I don't think that 
would be very desirable-- #if 0 is completely commented out code. In some cases 
switching it on may even directly hurt. So my suggestion would be to leave the 
#if 0 code there. 

2. Try and maintain the indentation for rest of the code as is. There are some 
cases where you have removed some spaces too. 

3. Make sure you do try and build this on at least one other platform. 

Comment 10

[John Taylor]

Attachment: Newer patch

Comment 11

[Gagan]

In file nsFilePicker.cpp, for this patch--
@@ -88,7 +92,9 @@
 {
   //  nsFilePicker *f = (nsFilePicker*)data;
   gchar *foo = (gchar*)gtk_object_get_data(GTK_OBJECT(w), "filters");
+#ifdef DEBUG
   g_print("filter_item_activated(): %s\n", foo);
+#endif
 }

if foo is not being used for anything else (and I suspect it is the case) then 
wrap that under the DEBUG check as well. Same thing with filters (a few lines 
below)

just fyi doing something like this will result in a leak!
  x = something.ToNewCString()
  printf(x)

So make sure a corresponding nsCRT::free(x) is there. I couldn't tell from the 
patches if it was there or not.

Comment 12

[John Taylor]

Attachment: GTK patch

Comment 13

[cls]

Wrt attach 37987:

The cases that would wind up as | else {} | in an opt build look slightly
questionable but assuming it compiles, r=cls .

Comment 14

[John Taylor]

Attachment: Patch fix for gtk

Comment 15

[Scott Collins]

It's really hard to see what you did in this last patch because it looks as
though the entire file was replaced.  Did you replace spaces with tabs or
something?  Maybe you need to attach a -w patch for review, ensure that what you
intend to check in uses spaces, not tabs.

Comment 16

[John Taylor]

Attachment: gtk fix without cvs madness

Comment 17

[Christopher Blizzard (:blizzard)]

+#ifdef DEBUG
   //  nsFilePicker *f = (nsFilePicker*)data;
   gchar *foo = (gchar*)gtk_object_get_data(GTK_OBJECT(w), "filters");
   g_print("filter_item_activated(): %s\n", foo);
+  nsCRT::free(foo);
+#endif

Don't free that.  It's a reference, not a copy.

Fix that and you have an sr=blizzard.

Comment 18

[John Taylor]

Attachment: Final gtk patch

Comment 19

[Gagan]

r=gagan now get an approval and land it in! :)

Comment 20

[John Taylor]

I got SR, submitted for approval earlier today to [e-mail].

Comment 21

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.2

Comment 22

[John Taylor]

Fix in.

Comment 23

[John Taylor]

Fix checked in, resolving.