506121 - consolidate the 30 logging functions found in js code

Status: NEW

(Firefox :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

or ifdef DEBUG them out. or something. every chunk of js that's not needed is drag, causes more ms spent in js_Execute on platforms like arm and wince.

Comment 1

[Nochum Sossonko [:Natch]]

The url misses some logging functions defined as |log: function| or |var log = function| etc.

I have a few ideas for this:

1) Provide two helper functions in /browser and in /toolkit (I haven't decided which file yet, as I'm not sure which would be the best, but globalOverlay seems like a good candidate for /browser) that log based on a hidden pref.

2) Provide a js module for pref-based logging, one hidden pref turns on logging for all files that use this module.

3) #if defined(WINCE) && !defined(DEBUG)

Let me know which you would prefer.

Comment 2

[Nochum Sossonko [:Natch]]

Another idea, we already have a module (debug.js), I'll place it in a memoized getter and add a "log" function to it. The getter won't import unless a local variable is true (similar to how the video controls' logging works).

I'm not sure that this is better though, again, thoughts?

Comment 3

[Nochum Sossonko [:Natch]]

Ok, there's tons of wasted code for logging/asserting in js. Personally I can't beleive NS_ASSERT is defined and used in release builds, even C++ assertions, which are definitely worse than a simple JS problem, are udefed in release builds.

Problem is, the amount of code it takes to write up a LOG function is tiny, and even if there's a centralized place for it, the code in the centralized place would be js_Execute'd every time it's encountered. The only reasonable thing to do is to #ifdef DEBUG them out in each individual file, you can't do that in a centralized file because then you have to define an empty function for logging, or the various LOG calls will fail. 

Furthermore, for each js component there will have to be individual logging functions (or imported functions ifdef DEBUG), which makes for a lot of duplicated statements, such as:

#ifdef DEBUG
__defineGetter__("LOG_JS", function() {
...
import("resource://gre/modules/debug.js");
return LOG_JS;
});
#else
function LOG_JS(txt) {}
#endif

Unless every call will be ifdef DEBUG, which makes for even messier functionality, but that's at least agreeable. Note, globalOverlay.js imports debug.js (that's time in compiling and executing) via a memoized getter, and FeedConverter.js #includes it...

Since I already started working on this, I'll attach a patch soon of some stuff I was working on.

Comment 4

[Dietrich Ayala (:dietrich)]

(In reply to comment #2)
> Another idea, we already have a module (debug.js), I'll place it in a memoized
> getter and add a "log" function to it. The getter won't import unless a local
> variable is true (similar to how the video controls' logging works).
> 
> I'm not sure that this is better though, again, thoughts?

i like this. could even ifdef DEBUG inside that file, making LOG a no-op in release builds.

a followup bug could be to pull in Lab's logging module into debug.js: https://wiki.mozilla.org/Labs/JS_Modules#Logging.

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> i like this. could even ifdef DEBUG inside that file, making LOG a no-op in
> release builds.
>

heh, i need coffee. ignore me :)

Comment 6

[Nochum Sossonko [:Natch]]

Attachment: wip

This is still missing a bunch of log functions, just throwing it up to see if this would be acceptable.

I've added a convenience getter for a logging function that can be used anywhere globalOverlay.js is in scope.

All logging is controlled via a build-time configure option, --enable-debug-logging. Logging, IMHO, doesn't belong in release builds, even if it doesn't take up too much time. Besides, a lot of these files need to be modified to enabled logging, so why not just rebuild with --enable-debug-logging. I'm still not sure if I did the configure stuff right at all, it was basically c&p.

Thoughts?

Comment 7

[Nochum Sossonko [:Natch]]

->default owner, I don't know of a good way to really do this aside from just ifdef'ing the world, either way I'm not going to be working on this for a while.

Comment 8

[Nochum Sossonko [:Natch]]

Comment on attachment 392650 [details] [diff] [review]
wip

This doesn't work for the ifdef'ing and has other issues with it as well.