117440 - remove usages of DEBUG_PRINTF in favor of NS_ASSERTION/NS_WARNING

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Morten Nilsen]

Timeless asked me to do this...
not yet tested fully, but it doesn't break my optimized tree fwiw.

will do a debug build as soon as I have freed up enough space.

patch coming up in a minute...

Comment 1

[Morten Nilsen]

Attachment: first cut

Comment 2

[Morten Nilsen]

Attachment: new, improved patch

With space freed, I was able to build debug build, and make the patch work.

Comment 3

[Morten Nilsen]

Attachment: new patch, use nsPrintfCString more places

Comment 4

[Morten Nilsen]

Attachment: missed one

Comment 5

[Morten Nilsen]

Attachment: dropped a couple of values :(

Comment 6

[Brendan Eich [:brendan]]

NS_WARNING does not work like PR_LOG -- it does not take a parenthesized actual
parameter list.  It takes a single string argument.  So changes such as this:

+          NS_WARNING(("\n\ncharset = %s", atomToName(charset)));

do not have the effect you want -- they'll pass the final operand in a comma
expression as the single string argument.

/be

Comment 7

[Morten Nilsen]

Attachment: Better patch, hopefully final?

Comment 8

[Brendan Eich [:brendan]]

I'd like jag to sr= the last patch.  Do we really need to wrap nsPrintfCString
in an nsCAutoString?  Maybe the kLocalBufferSize in nsPrintfCString.h is too
small; if it were bigger, we could just declare a local nsPrintfCString and pass
it to NS_WARNING, and avoid another (bigger) stack-allocated string, and the
copy into that auto-string's buffer.

/be

Comment 9

[Morten Nilsen]

I wrapped nsPrintfCString in nsCAutoString because otherwise it wouldn't compile...
I got that piece of code from jag.
IIRC, he said "use nsCAutoString(nsPrintfString(..)).get() for now"

so I suppose there might be a better solution.

Comment 10

[jag (Peter Annema)]

Attachment: Make nsPrintfCString inherit from nsAFlatCString [Checkin: Comment 42]

I advised wrapping the result of nsPrintfCString in a nsCAutoString so he could
use .get(). With this patch one can call .get() on a nsPrintfCString directly.

Comment 11

[jag (Peter Annema)]

Right, what Morten said (no conflicts when submitting comments from patch manager).

Comment 12

[Alec Flett]

Comment on attachment 64080 [details] [diff] [review]
Make nsPrintfCString inherit from nsAFlatCString
[Checkin: Comment 42]

sr=alecf

Comment 13

[Morten Nilsen]

Attachment: remove nsCAutoString wrapping [Checkin: See comment 21=44]

Comment 14

[Darin Fisher]

Comment on attachment 64080 [details] [diff] [review]
Make nsPrintfCString inherit from nsAFlatCString
[Checkin: Comment 42]

r/sr=darin

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 64085 [details] [diff] [review]
remove nsCAutoString wrapping
[Checkin: See comment 21=44]

>@@ -136,7 +134,7 @@
>                            const char* registryLocation,              \
>                            const nsModuleComponentInfo *info)         \
> {                                                                     \
>-  UCONV_DEBUG_PRINTF("UnRegSelf " _From " to " _To "converter not implement\n"); \
>+  NS_WARNING("UnRegSelf " _From " to " _To "converter not implement\n"); \
>   return NS_OK;                                                       \
> }

"not implemented\n" for proper English -- and why isn't this returning
NS_ERROR_NOT_IMPLEMENTED rather than NS_OK?

Fix those two things and sr=[e-mail].  I recorded timeless's r=,
which carries over.

/be

Comment 16

[Morten Nilsen]

Attachment: new patch to nsICharsetConverterManager.h

this patch replaces part of attachment 64058 [details] [diff] [review], but not all of it.

this patch also removes unneeded \'s from most of the lines...

Comment 17

[Morten Nilsen]

bah... seems atleast some \'s where actually needed... investigating them

Comment 18

[Morten Nilsen]

Attachment: *sigh* I need a good slapping... new patch not removing \'s

Comment 19

[Morten Nilsen]

Attachment: final patch to nsICharsetConverterManager.h [Checkin: Comment 21=43]

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 64097 [details] [diff] [review]
final patch to nsICharsetConverterManager.h
[Checkin: Comment 21=43]

sr=[e-mail]

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachments 64085/64097 checked in.

Comment 22

[Morten Nilsen]

allrighty then

Comment 23

[kill this account]

These used to be off by default and could be turned on if needed.

Now these warnings are firing all the time in debug builds and there is no way 
to enable them in a non-debug build.

Comment 24

[kill this account]

I'm curious why I was not included in the bug from the beginning.

Comment 25

[kill this account]

or Shanjian or Roland since this is the area we work in every day.

Comment 26

[kill this account]

One of the problems that the developers that work on font bugs face is that
the font setup varies wildly from system. The font code tries a variety of 
thing to get the best fonts so its behavior is fairly complex. To debug
problems a developer needs more info than just "it does not look good"

consider bug 128338: Incorrect fonts used for certain "lang=" attributes

> When setting NS_FONT_DEBUG=3 and loading the page in the URL 
> field I get the  output below. ...
> As far as I can tell from looking at the relevant standards 
> both "no", "no-bokmal" and "nb" should be correct identifiers 
> for the same language. 
> 
> 
> GetFontNames -adobe-courier-*-*-*-*-*-*-*-*-*-*-iso8859-1
> loaded -adobe-courier-medium-r-normal--12-120-75-75-m-70-iso8859-1
> FindFont(0061)[monospace, ]
> returns -adobe-courier-medium-r-normal--12-120-75-75-m-70-iso8859-1
> GetFontNames -*-*-*-*-*-*-*-*-*-*-*-*-cns11643.1986-2
> GetFontNames -*-*-*-*-*-*-*-*-*-*-*-*-cns11643.1992.2-0

Without information about what fonts are being considered how would a
developer tell what was happening? Withoug knowing the details of what is 
happening it is hard to fix bugs like this.

Is the plan that to fix font bugs the developer will always need to make
and post a debug build so the diagnostics will be available?

Won't requiring custom build make the task of debugging much more difficult 
and far slower as well as possibly introduce compilcations from new bugs?

Comment 27

[Mike Shaver (:shaver emeritus)]

Yeah, I'm with Brian: this is a job for PR_LOG, forced on in release builds (and
used sparingly in performance-critical paths!).

Comment 28

[kill this account]

Since this patch broke the existing working code (it now reports when it is not
wanted and cannot report when it is needed) can those who put this patch in
at least fix those parts and explaing how their fix will be used/controlled?

Comment 29

[kill this account]

I am distressed by the lack of comment or "commitment to make working code" by 
those who put in this patch. Nobody's mother works here. We all need to clean
up after ourselves.

I am also still waiting to hear comment on why the people that regularily work 
in this code were not contacted before this patch was put in.

Perhaps we should just back out this work.

Comment 30

[Brendan Eich [:brendan]]

bstell, sorry for not ensuring you got to r= -- this looked like pan-module
cleanup, and I blew it.  We've had problems with mailnews guys not being
apprised of pan-module cleanups touching their code, and not having a crack at
r= -- this has led to super-review angst, admonishment, and advice for a better
tomorrow -- tree-wide patches should be broken up by module, so that each module
owner or peer can r= using the patch manager.

Sorry again, do what you need to do to fix it right.  PR_LOG forced on sounds
good to me too.

/be

Comment 31

[kill this account]

Brendan: Thank you for responding. I am glad to see some discussion of the 
basic communication issue.

I do not feel any need to r= everything that gets done in this area. I am
in fact glad to see other working on trying to improve it. No one of us can
do it all.

My concern is that by failing to include those that work in an area it is all
too easy to misunderstand what the code is doing. Without knowing what the 
code is doing it is very hard not to break it even if unintentionally.

With a very little bit of input at an early stage this would probably be 
working code instead of broken code.

I am not especially concerned with the exact implementation of this code
but the feature it provides is there to help with maintenance and it weakens
the product to lose it. I am very open to an alternate implementations that 
provide similar maintenance utility.

I am, however, very sensitive to having clean up code broken by others as this 
was a major problem for the i18n department in the NS4x days. We worked very
hard to get out of this mode. Last time I left Netscape this was a major reason.

Comment 32

[kill this account]

At this point I would like to hear the people who made the change step 
forward and discuss how they will rectify this situation.

Comment 33

[Morten Nilsen]

I'm sorry if my patcch here stepped on anybody's toes, but I did it because
timeless wanted me to... I'm quite new to mozilla hacking, and timeless gave me
this little task as a nice easy thing to do...

If the consent is that this patch ruins the day, it's fine by me if it's backed
out, or at least two of the patches... the third one should stay in (attachment
64080 [details] [diff] [review])

Comment 34

[kill this account]

Morten: I'm not especially concerned that you might have stepped on my
toes. Its no problem. In team activities everyone ends up stepping on others
toes every now and then. I certianly have stepped on others toes.

I am okay with someone going forward to make this work using PR_LOG.

I am okay with someone backing out the code (less the indicated bit) to make
it work like it used to.

How can we get to the point of working code (and "All's well that ends well")?

Comment 35

[kill this account]

Morten: are you going to fix this?

Comment 36

[Morten Nilsen]

Sorry if I didn't make clear on this, but I'm all out of time to do anything
mozilla-wise...

Somebody else would need to do this. Sorry.

Comment 37

[Roland Mainz]

bstell:
I can take this one... I only need a description what I should fix... :)

Comment 38

[kill this account]

Roland: 

Do to a misunderstanding of their purpose some of the NS_FONT_DEBUG 
statements were converted to assertions. This means that for debug
builds they report when they should not and for optimized builds
they cannot report when we want to find out what the font subsystem 
is doing on a user's system.

One possible solution would be to simply backout the changes.

Another possible solution would be to use PR_LOG. Of course we
would still want to have the messages not report during debug and
be able to be turned on in a user's sytem.

Comment 39

[Morten Nilsen]

reassigning bug to Roland since he has expressed a wish to take this.

Comment 40

[Morten Nilsen]

I'm looking over my old bugs (still not able to work on this myself)
should this bug be closed or reassigned to [e-mail]?

Comment 41

[Wayne Mery (:wsmwk)]

(In reply to comment #34)
> ...
> I am okay with someone going forward to make this work using PR_LOG.
> 
> I am okay with someone backing out the code (less the indicated bit) to make
> it work like it used to.

I don't see a comment, but it does appear this was backed out.  Please comment if I am incorrect.

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 64080 [details] [diff] [review]
Make nsPrintfCString inherit from nsAFlatCString
[Checkin: Comment 42]


Landed:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpcom/string/public/nsPrintfCString.h&rev=MOZILLA_1_8_BRANCH&mark=1.7

Then was superseded:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpcom/string/public/nsPrintfCString.h&rev=MOZILLA_1_8_BRANCH&mark=1.11

Comment 43

[Serge Gautherie (:sgautherie)]

Comment on attachment 64097 [details] [diff] [review]
final patch to nsICharsetConverterManager.h
[Checkin: Comment 21=43]


Landed:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/intl/uconv/public/nsICharsetConverterManager.h&mark=1.30

Then was superseded:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/intl/uconv/public/nsICharsetConverterManager.h&mark=1.31

Comment 44

[Serge Gautherie (:sgautherie)]

Comment on attachment 64085 [details] [diff] [review]
remove nsCAutoString wrapping
[Checkin: See comment 21=44]


Landed:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&sortby=Date&hours=2&date=explicit&mindate=2002-01-09+10%3A49&maxdate=2002-01-09+10%3A49
(without nsICharsetConverterManager.h part)

Comment 45

[Serge Gautherie (:sgautherie)]

(In reply to comment #41)
> I don't see a comment, but it does appear this was backed out.  Please comment
> if I am incorrect.

No backout. Yet the files affected by the 2 patches which "should have been" were all removed since.
Thus if any of this code was moved to some other/new files, then too bad.

Comment 46

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) <nsCompressedCharMap.cpp> remove commented out FONT_SCAN_PRINTF() [Checkin: Comment 47]

Comment 47

[Serge Gautherie (:sgautherie)]

Comment on attachment 374003 [details] [diff] [review]
(Dv1) <nsCompressedCharMap.cpp> remove commented out FONT_SCAN_PRINTF()
[Checkin: Comment 47]


http://hg.mozilla.org/mozilla-central/rev/e883a4efa220

Comment 48

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) <xpcwrappednativescope.cpp> remove unused |#ifdef + NS_WARNING()| case [Checkin: Comment 50]

jst, ftr, you checked this in in
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp&rev=1.10

Comment 49

[Serge Gautherie (:sgautherie)]

Comment on attachment 374212 [details] [diff] [review]
(Ev1) <xpcwrappednativescope.cpp> remove unused |#ifdef + NS_WARNING()| case
[Checkin: Comment 50]


Looking for any review(er)...

Comment 50

[Serge Gautherie (:sgautherie)]

Comment on attachment 374212 [details] [diff] [review]
(Ev1) <xpcwrappednativescope.cpp> remove unused |#ifdef + NS_WARNING()| case
[Checkin: Comment 50]


http://hg.mozilla.org/mozilla-central/rev/65f84c853781