78672 - [console] xpfe must not print to console in opt builds

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect, P2)

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: (Av1) <xpfe/*/*.cpp> "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

[sairuh (rarely reading bugmail)]

remember this, jag? ;)

Comment 4

[Jon Granrose]

76720 is targetted for 0.9.1, setting same.

Comment 5

[Paul Chen]

adding mcafee to cc list

Comment 6

[Viswanath Ramachandran]

reassign to chris for load balancing : chris can you fix this ? in any case, an 
rtm stopper but not neccessary for beta. 

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

[Chris McAfee]

I think I was supposed to get this

Comment 9

[Paul Chen]

nav triage team:

Why is there a new bug for 76720? Or is this some work that's getting done 
without all that noise? At any rate, marking nsbeta1- to get off the triage 
radar. That doesn't mean we will be unhappy if it just happens to work. (wink, 
wink, nudge, nudge)

Comment 10

[Viswanath Ramachandran]

nav triage: not critical for m0.9.2, moving out. 

Comment 11

[Chris McAfee]

over to samir

Comment 12

[Samir Gehani]

cls,
What sort of xul and js output are you referring to?  (The only output I know of
is dump() which turns to a 'noop' -- or at least doesn't write to the console --
in opt builds, right?)  Need to know what else I should scan for before aplpying
and checking in your patch.  Thanks.

Comment 13

[cls]

After apply these various patches to my tree, I was still getting some output to
the console.  A cursory check of a particular line of output showed it coming
from a dump() inside a .js file.  This was on an opt build.  So if you can
convince whomever to make dump() a noop on opt builds (or release builds) that
would speed up this cleanup quite nicely.

Comment 14

[Samir Gehani]

Ah, thanks for clearing that up!  I'll add DEBUG flags to the offending .js
files to allow the file-level control of the dump() statements.  

Alternateively, could we just make dump() a noop in opt builds if we are
building thr browser?  I can't imagine we want a console for mozilla or
commercial.  

Comment 15

[jag (Peter Annema)]

If you make dump a noop, please only do so for release builds. In nightlies, and 
personal opt. builds, dump is an often used way to get a quick idea of what's 
going on when narrowing down a bug.

I'd prefer the noop over per-site disabling through DEBUG flags though. Really 
there just shouldn't be any dumps (except for a few places like the "page loaded 
successfully" in the browser), so anything you run across should probably just 
be commented out or completely removed.

Comment 16

[Samir Gehani]

jag,
Actually, in my experience putting DEBUG flags into js has turned out useful
because one won't get the dump()s until the DEBUG flags are explcitly switched
to true since I'll default them to false (like sidebar search for example). 
This satisfies your point about not needing the dump()s at all.  

About turning dump() to a noop, instead of a noop maybe we could do something
similar to the way NSPR logging is handled: i.e., if an environment var is set
such as MOZ_ENABLE_DUMP then we make dump an op, else a noop.  So, what do you
think of that idea?  That'll eliminate the need for everyone to go clean up
their code and remove dump() statements and for those working on opt builds who
need to see the output we could just have them set this environment var I mentioned.

cls, others,
Soliciting input on the afore mentioned solution to make dump() controllable
through an environment var.

Comment 17

[Alec Flett]

there's another bug about making this a pref, I thought it was assigned to jband

Comment 18

[Blake Ross]

cls, is this bug still valid?

Comment 19

[Samir Gehani]

-> Future

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Av2-SM) <xpfe/*/*.cpp> (SeaMonkey part) [Checked in: Comment 35]

Av1, updated against current trunk,
and a little revised too.

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 149449 [details] [diff] [review]
(Av2-SM) <xpfe/*/*.cpp> (SeaMonkey part)
[Checked in: Comment 35]


I have no compiler: Could you compile/test/(super-)review/check in this patch ?
Thanks.

Comment 22

[Serge Gautherie (:sgautherie)]

'blocking1.7=?':
Based on bug 243870...

Comment 23

[Asa Dotzler [:asa]]

not going to block for this but we'd certainly consider a reviewed patch for
inclusion in 1.7. You might seek a different reviewer because Jag's pretty busy
these days.

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-FF) <nsNativeAppSupportWin.cpp> (FireFox part) [Checked in: Comment 45]

"Same" as Av2-SM, applied to FireFox.

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 149449 [details] [diff] [review]
(Av2-SM) <xpfe/*/*.cpp> (SeaMonkey part)
[Checked in: Comment 35]


I have no compiler: Could you compile/test/review this patch ? Thanks.

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 149453 [details] [diff] [review]
(Bv1-FF) <nsNativeAppSupportWin.cpp> (FireFox part)
[Checked in: Comment 45]


I have no compiler: Could you compile/test/(super-)review/check in this patch ?
Thanks.

Comment 27

[Alec Flett]

Comment on attachment 149449 [details] [diff] [review]
(Av2-SM) <xpfe/*/*.cpp> (SeaMonkey part)
[Checked in: Comment 35]

I can't believe I'm going to do this, but sr=alecf

This must be compiled before it gets checked in though.

(frankly, I wish we would get rid of nsUrlbarHistory::PrintHistory altogether,
but this is probably worth the hackyness for now)

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to comment #27)
> (From update of attachment 149449 [details] [diff] [review])
> (frankly, I wish we would get rid of nsUrlbarHistory::PrintHistory altogether,
> but this is probably worth the hackyness for now)

Well, (1, 2, 3) PrintHistory seem to be unused:

<http://lxr.mozilla.org/mozilla/search?string=PrintHistory>
{{
/content/xml/tests/docbook.css, line 342 -- PrintHistory {

/xpfe/components/shistory/src/nsSHistory.cpp, line 281 -- nsSHistory::PrintHistory()
/xpfe/components/shistory/src/nsSHistory.h, line 85 -- nsresult PrintHistory();

/xpfe/components/urlbarhistory/public/nsIUrlbarHistory.idl, line 65 -- void
printHistory();
/xpfe/components/urlbarhistory/src/nsUrlbarHistory.cpp, line 220 --
nsUrlbarHistory::PrintHistory()
}}

Should we get rid of some/all of them ?

Comment 29

[Alec Flett]

lets let someone with a compiler do it.

Comment 30

[Serge Gautherie (:sgautherie)]

(In reply to comment #29)
> lets let someone with a compiler do it.

I take this as a "yes, try..." !!?

Comment 31

[[e-mail]]

Comment on attachment 149449 [details] [diff] [review]
(Av2-SM) <xpfe/*/*.cpp> (SeaMonkey part)
[Checked in: Comment 35]

>RCS file: /cvsroot/mozilla/xpfe/components/urlbarhistory/src/nsUrlbarHistory.cpp,v
Actually I got rid of this file altogether :-)

Comment 32

[[e-mail]]

Just want to get the all-clear for the platform-specific parts of this patch.

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #31)
> (From update of attachment 149449 [details] [diff] [review])
> >RCS file:
/cvsroot/mozilla/xpfe/components/urlbarhistory/src/nsUrlbarHistory.cpp,v
> Actually I got rid of this file altogether :-)

Noted: I'll update this patch after r+, for check-in.

***

Now that I have started to use the '-console' (Windows) parameter,

I wonder if the <nsNativeAppSupportWin.cpp> (for both M & FF) part should better
be kept active even for optimized build !?

Comment 34

[[e-mail]]

Comment on attachment 149449 [details] [diff] [review]
(Av2-SM) <xpfe/*/*.cpp> (SeaMonkey part)
[Checked in: Comment 35]

I got the go-ahead for the photon (qnx) change, but not the solaris change,
because it's ideally a fatal condition...

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 149449 [details] [diff] [review]
(Av2-SM) <xpfe/*/*.cpp> (SeaMonkey part)
[Checked in: Comment 35]


Check in: { 2004-06-14 14:50	neil%parkwaycc.co.uk }

Comment 36

[Serge Gautherie (:sgautherie)]

(In reply to comment #35)
> (From update of attachment 149449 [details] [diff] [review])
> 
> Check in: { 2004-06-14 14:50	neil%parkwaycc.co.uk }

Photon and Windows, yes;
Solaris and UrlBar, no.
(see previous comments)

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 149449 [details] [diff] [review]
(Av2-SM) <xpfe/*/*.cpp> (SeaMonkey part)
[Checked in: Comment 35]

(In reply to comment #23)
> we'd certainly consider a reviewed patch for
> inclusion in 1.7.

Well, here we are. (if not too late)

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 149453 [details] [diff] [review]
(Bv1-FF) <nsNativeAppSupportWin.cpp> (FireFox part)
[Checked in: Comment 45]

No (super-)review from <[e-mail]> since "2004-05-27" :-(
Neil pointed me to you...
I'm looking for both r and sr.
(Is there anything else to do to get it into aviary1.0 branch too ??)

Note: This was already checked in into Mozilla Trunk.

Comment 39

[Mike Connor [:mconnor]]

Comment on attachment 149453 [details] [diff] [review]
(Bv1-FF) <nsNativeAppSupportWin.cpp> (FireFox part)
[Checked in: Comment 45]

r=me, no SR needed, please get this in on aviary branch and trunk

Comment 40

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> (In reply to comment #27)
> 
> Well, (1, 2, 3) PrintHistory seem to be unused:
> 
> <http://lxr.mozilla.org/mozilla/search?string=PrintHistory>
> {{
> /content/xml/tests/docbook.css, line 342 -- PrintHistory {
> 
> /xpfe/components/shistory/src/nsSHistory.cpp, line 281 --
nsSHistory::PrintHistory()
> /xpfe/components/shistory/src/nsSHistory.h, line 85 -- nsresult PrintHistory();
> }}
> 
> Should we get rid of some/all of them ?

I filed bug 248010 and bug 248011 about these...

Comment 41

[Serge Gautherie (:sgautherie)]

I'm going to R.Fixed this bug after the FireFox patch lands;

Unless someone wants me not to,
or can give other cases to fix.

Comment 42

[Serge Gautherie (:sgautherie)]

[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE)

(In reply to comment #39)
> (From update of attachment 149453 [details] [diff] [review] [edit])
> r=me, no SR needed, please get this in on aviary branch and trunk

Brian, (or anyone)
Could you do this double check-in ? Thanks.

Comment 43

[Asa Dotzler [:asa]]

Comment on attachment 149453 [details] [diff] [review]
(Bv1-FF) <nsNativeAppSupportWin.cpp> (FireFox part)
[Checked in: Comment 45]

serge, please don't request approval for minor issues like this. Thanks.

Comment 44

[Serge Gautherie (:sgautherie)]

Comment on attachment 149453 [details] [diff] [review]
(Bv1-FF) <nsNativeAppSupportWin.cpp> (FireFox part)
[Checked in: Comment 45]

'approval1.8a2=?':
Trivial |#ifdef DEBUG|, no risk.

Comment 45

[Brian Ryner (not reading)]

Comment on attachment 149453 [details] [diff] [review]
(Bv1-FF) <nsNativeAppSupportWin.cpp> (FireFox part)
[Checked in: Comment 45]

checked in this patch