GNOME Bugzilla – Bug 675104
lookingGlass: Remove the Errors tab
Last modified: 2012-05-02 18:00:55 UTC
We already have one too many logging systems, and we should reduce the amount of random places to check in case something blows up.
Created attachment 213063 [details] [review] environment: Allow window.log and friends to take multiple arguments
Created attachment 213064 [details] [review] lookingGlass: Remove the "Errors" tab We already have one too many logging systems. Remove the errors tab and make global.log/global.logError point to window.log/window.logError instead.
Created attachment 213065 [details] [review] lookingGlass: Remove the "Errors" tab We already have one too many logging systems. Remove the errors tab and make global.log/global.logError point to window.log/window.logError instead.
Review of attachment 213063 [details] [review]: ::: js/ui/environment.js @@ +44,3 @@ +function _makeLoggingFunc(func) { + return function() { + return func([].slice.apply(arguments).join(', ')); 1) You don't need [].slice.apply, use Array.prototype.join.call. (Ok, it's difficult to tell which one is more readable, but at least it skips allocating the Array) 2) This is will regress logError, as that takes an exception as first argument, so it must not be coerced into a string. @@ +56,3 @@ + window.logError = _makeLoggingFunc(window.logError); + window.print = _makeLoggingFunc(window.print); + window.printerr = _makeLoggingFunc(window.printerr); Why print/printerr? They're not part of the legacy logging system.
Review of attachment 213065 [details] [review]: You can remove _errorLogStack declaration too. ::: js/ui/main.js @@ +155,3 @@ function start() { + // These are here so we don't break compatibility. + global.logError = window.log; Is it ok to have logError behave identically as log? (I don't think anyone uses global.logError, btw)
(In reply to comment #5) > Is it ok to have logError behave identically as log? Oops. > (I don't think anyone uses global.logError, btw) The extension system does, but we could change that. I'm mostly just worried about extension compatibility.
(In reply to comment #4) > Review of attachment 213063 [details] [review]: > > ::: js/ui/environment.js > @@ +44,3 @@ > +function _makeLoggingFunc(func) { > + return function() { > + return func([].slice.apply(arguments).join(', ')); > > 1) You don't need [].slice.apply, use Array.prototype.join.call. > > (Ok, it's difficult to tell which one is more readable, but at least it skips > allocating the Array) IIRC SpiderMonkey will compile this down into Array.prototype.slice.call anyway. I can just do the join directly, without the slice. > 2) This is will regress logError, as that takes an exception as first argument, > so it must not be coerced into a string. Ah, whoops. > @@ +56,3 @@ > + window.logError = _makeLoggingFunc(window.logError); > + window.print = _makeLoggingFunc(window.print); > + window.printerr = _makeLoggingFunc(window.printerr); > > Why print/printerr? > They're not part of the legacy logging system. Because they were there in context.c. I've seen some extensions using print, but I guess they won't need multiple arguments, and we should dissuade people from using them. I guess all we'll need to patch is window.log, then.
Created attachment 213066 [details] [review] environment: Allow window.log and friends to take multiple arguments
Created attachment 213067 [details] [review] lookingGlass: Remove the "Errors" tab We already have one too many logging systems. Remove the errors tab and make global.log/global.logError point to window.log/window.logError instead.
Review of attachment 213066 [details] [review]: Good to commit, but remove "and friends" from the message.
Review of attachment 213067 [details] [review]: Good to go!
Comment on attachment 213067 [details] [review] lookingGlass: Remove the "Errors" tab Attachment 213067 [details] pushed as 78e894c - lookingGlass: Remove the "Errors" tab