Hello! This is a proposal for a consolidated log message object. Currently the HUDService creates the message nodes on behalf of the HeadsUpDisplay object. It also does the filtering, it also handles the hiding of message nodes, with the ConsoleUtils.hideLogMessage() method, and some more. The main HUDService uses the logNetActivity() and logConsoleActivity() methods to create the message object - these are invoked by the logActivity() method depending on the origin of the activity. Both, logNetActivity() and logConsoleActivity(), invoke the messageFactory() method that in turn creates the messageNode, using a new instance of the LogMessage() object. All/some of this is, at the end, sent to the HUDService.logMessage() method, which acts as a proxy for the HUDService.logHUDMessage() method. This last method filters the content of the log message based on the current value of the filter box, and stores the message object in the ConsoleStorage object instance (HUDService.storage). The messageNode is appended to the outputNode of the HeadsUpDisplay object instance. My proposal is to simplify the code: - Defined a new HUDmessage() object, that is meant to be stored in the ConsoleStorage instance, which does not hold any DOM node, and which always has an id property, a timestamp (integer, not a hard-coded string), an origin, and a hudId. For global messages that do not come from a HUD ... or cannot be associated to a HUD, I suggest we use a known hudId, like -1. This is the raw message object that should be used everywhere. - Removed the following objects/methods, as they are no longer needed: ConsoleUtils.hideLogMessage() HUDService.filterLogMessage() HUDService.getFilterStringByHUDId() HUDService.getFilterTextBox() HUDService.logActivity() HUDService.logConsoleMessage() - it was not unused. Removed it so it doesn't cause any confusion. HUDService.logHUDMessage() HUDService.messageFactory() LogMessage() object constructor - Moved the code of the HUDService.logHUDMessage() method into the HUDService.logMessage() method, while simplifying it. - The HUDService.logNetActivity() method now holds only the code that finds the hudId for the activity being logged. At the end it only creates the message object with the HUDMessage() constructor. This is sent to the HUDService.logMessage() method. Ideally, there should be less code, and it shouldn't involve DOM nodes, but I don't know if it's possible - I haven't checked yet. Anyhow, this is beyond the purpose of this proposal. - For the HUDService.logConsoleActivity() method the approach is almost identical. There's only code for finding the hudId (based on the URI.spec), and for constructing the HUDMessage instance, which is sent to the HUDService.logMessage() method. - The filtering of messages in the logNetActivity() and logConsoleActivity() methods is only performed based on states, using the HUDService.getFilterState() method. - The HUDConsole.sendToHUDService() method constructed its own message object and messageNode, without using the HUDService.messageFactory() method. Now the code creates a new HUDMessage object which is sent to the HUDService.logMessage() method, just like in the logNetActivity() and logConsoleActivity() methods. - The HUDService.logMessage() method takes only one argument: the HUDMessage object instance. What it does is really simple: it stores the HUDMessage object into the HUDService.storage (with the recordEntry() method), and it finds the HeadsUpDisplay object instance, to give it the HUDMessage object. This is practically what the old HUDService.logHUDMessage() method did. That method stored the message object and directly appended the messageNode to the outputNode. It also did text filtering. Now this happens in the new HeadsUpDisplay.logMessage() method. - Added three methods to the HeadsUpDisplay objects: logMessage(), logNetMessage() and logConsoleMessage(). - The HeadsUpDisplay.logMessage() method takes a raw HUDMessage object, and it creates a message node which is then appended to the outputNode of the HUD. The content of the message node depends on the HUDMessage type which is determined based on the origin. The content is given by the two logNetMessage() and logConsoleMessage() methods - more can be added easily. Each "know" how to display their type of message. The messageNode content is filtered, based on the value of the filterBox input. That is pretty much the overview of the changes. Here are some additional changes included in the patch: - There was some inconsistency in the property names used for messageObjects. For example, activityObj versus activity versus activityObject, level versus logLevel. The code now consistently uses message.activity and level. - The HUDService.logNetActivity() method had most of its code in a big try. Changed the code to no longer need a try statement. - Added error handling in the HUDService.getLoadContext() method, such that we can have less try statements - in logNetActivity() and in other methods. - Added error handling in the HUDService.getOutputNodeFromRequest() method, such that we can have less try statements - in logNetActivity() and in other methods. - Updated the HUDService.reportConsoleServiceMessage() and reportConsoleServiceContentScriptError() methods to invoke the HUDService.logConsoleActivity() method. - Also updated the HUDService.startHTTPObservation() method to invoke logNetActivity(), instead of logActivity(). - Fixed a bug in the HUDConsole.sendToHUDService() method. The message object that is constructed did not reference the correct message property. Instead of this.hud.message, it should point to this.message. - Did a minor change in the ConsoleStorage.recordEntry() and recordEntries() methods: they no longer take a HUD ID argument. The hudId property of the message object is now used. - The ConsoleEntry() object has a minor update, to sync it with the consistent naming of the messageObject.level property. - Did minor updates for the test code, such that recorded entries have hudIds and level properties, instead of logLevel. Ideally, the test code should use the HUDMessage constructor for creating message objects, instead of having its own hard-coded message objects. I restrained myself from making too much changes in the test code. Please let me know if you believe I should make such changes. That is all. I believe the changes are not too big, and the patch actually removes more code than it adds. I also believe the patch simplifies the overall logging mechanism, making it clearer. Obviously, I'd love feedback from everyone and I'd really appreciate improvements, so this can be integrated some day. Please note that this patch depends on bug #568657 - consolidated timestamp code. Thanks!

This is the proposed implementation. All tests pass and I haven't found any regressions. The patch applies cleanly on top of the default branch. However, the patch of bug 568657 is needed.

Does this patch apply cleanly on mozilla-central? This patch is WAY too large and touches a lot of methods that you may or may not realize are needed. For instance, getting network activity via loadGroup probably only works 25% of the time. You should figure out how to break this into 3 to 5 patches, each logically separated. When you do too much in one patch things can get a bit confusing. This is good work, and does indeed simplify things, but you are doing too much in one patch.

Yes, this does apply on m-c as of yesterday (at least). All of the UI work I'm doing is dependent on this patch, so for all intents and purposes this is blocking bug 559481.

(In reply to comment #3) > Yes, this does apply on m-c as of yesterday (at least). The attachment 457295 [details] [diff] [review] doesn't apply cleanly on trunk. I sent a version of that patch to Patrick yesterday, that I made apply cleanly. Will upload this modified patch(es) in one sec...

This is mostly former attachment 457295 [details] [diff] [review] that applies cleanly on m-c.

This are some additions I've done to part1: - removes the not called function HS_initializeJSTerm - route the output of JST_execute via the HUDService.logMessage(..args) method as well - other small changes

(In reply to comment #4) > (In reply to comment #3) > > Yes, this does apply on m-c as of yesterday (at least). > > The attachment 457295 [details] [diff] [review] doesn't apply cleanly on trunk. I sent a version of that > patch to Patrick yesterday, that I made apply cleanly. Will upload this > modified patch(es) in one sec... Oh, sorry. I had assumed that the one you had sent me was the same one that was posted here.

What follows is a few patches that have been broken from the main patch, as requested by David. These smaller patches take a step-by-step approach. Each patch needs to be applied in the order I am attaching them - each depending on the previous one. This patch applies cleanly on mozilla-central. Patch description: it removes HUDService.logActivity(). Now only logNetActivity and logConsoleActivity() are used. The logActivity() method did not do anything with the arguments, it only acted as a proxy for the other two methods.

This patch merges the HUDService.logHUDMessage() method into the HUDService.logMessage() method. The latter was only a proxy method. All code tests pass, no known regressions.

This patch switches the code to the new HUDMessage object. There are changes to the HUDService.logMessage() method to only invoke the storage mechanism, and passes the new HUDMessage object to the HeadsUpDisplay.logMessage() method (new code). The logConsoleActivity() and logNetActivity() methods have been simplified to no longer deal with the message node. They only create a HUDMessage object that is passed to the HUDService.logMessage() method. The HUDConsole sendToHUDService() function was also updated accordingly. The JSTerm.execute() method was also updated to use the new HUDMessage object. Thanks to Julian! ;) ConsoleStorage.recordEntry() and recordEntries() no longer need the hudId argument. The hudId comes from the message object. updated test code as well. ConsoleEntry objects now use the HUDMessage.level property, not the logLevel property. updated test code as well. Known regression: HUD objects are garbage collected after a period of time, thus HUDService.logMessage() fails to work after some time - when GC kicks-in. All code tests pass. (The patch is a bit big, but that's because the code has a lot of places where messageNodes are created manually. If this is still to big, please let me know.)

The HUD objects are not always accessible from the HUDService.logMessage() method. They are garbage collected. This patch fixes the issue. Thanks to Julian! (see bug 580618) The HeadsUpDisplay constructor no longer takes the config.consoleOnly option. When the HUD already exists, the HUD object is reused, without creating a new instance. All code tests pass, no known regressions.

Code cleanup. Removed code that became obsolete with the previous patches: HUDService.getFilterStringByHUDId() HUDService.filterLogMessage() HUDService.getFilterTextBox() HUDService.logConsoleMessage() - was never used HUDService.messageFactory() HUDService.initializeJSTerm() - was never used JSTerm.writeOutput() JSTerm.clearOutput() - was never used ConsoleUtils.hideLogMessage() LogMessage() object constructor All code tests pass. No known regressions. This is the final patch.

Comment on attachment 459436 [details] [diff] [review] split 1: HUDService.logActivity() removal >- * Parse log messages for origin or listener type >- * Get the correct outputNode if it exists >- * Finally, call logMessage to write this message to >- * storage and optionally, a DOM output node >- * >- * @param string aType >- * @param nsIURI aURI >- * @param object (or nsIScriptError) aActivityObj >- * @returns void >- */ >- logActivity: function HS_logActivity(aType, aURI, aActivityObject) >- { >- var displayNode, outputNode, hudId; >- >- if (aType == "network") { >- var result = this.logNetActivity(aType, aURI, aActivityObject); >- } >- else if (aType == "console-listener") { >- this.logConsoleActivity(aURI, aActivityObject); >- } >- }, >- >- /** If you remove logActivity here, you should probably add logConsoleActivity and logNetActivity to this patch - unless there is something preventing you. I have not looked at subsequent patches yet.

Comment on attachment 459439 [details] [diff] [review] split 2: merge HUDService.logHUDMessage() into logMessage() nice work. much easier to review like this.

(In reply to comment #13) > Comment on attachment 459436 [details] [diff] [review] > split 1: HUDService.logActivity() removal > > If you remove logActivity here, you should probably add logConsoleActivity and > logNetActivity to this patch - unless there is something preventing you. I have > not looked at subsequent patches yet. The logNetActivity() and logConsoleActivity() method changes cannot come into this patch, because then it would be in a broken state - the whole Web Console would break. The changes in those methods require code from split 3 - the HUDMessage object. The approach for these patches is to have small chunks of working code, with no regressions. (thanks for feedback+ for split 2!)

Comment on attachment 459446 [details] [diff] [review] split 3: switch to the new HUDMessage object > const ERRORS = { LOG_MESSAGE_MISSING_ARGS: > "Missing arguments: aMessage, aConsoleNode and aMessageNode are required.", >- CANNOT_GET_HUD: "Cannot getHeads Up Display with provided ID", >+ CANNOT_GET_HUD: "Cannot get HeadsUpDisplay with provided ID", >+ HUD_WEAK_REFERENCE_FAIL: "Cannot get HeadsUpDisplay from weak reference", please reference "Web Console" in these error messages instead of "Heads Up Display" >+ throw new Error(ERRORS.HUD_WEAK_REFERENCE_FAIL + ": " + aMessage.hudId); I'm not sure we should throw here as we potentially would be throwing a lot. You might want to use Cu.reportError > } > } > else { >- // log everything >- aConsoleNode.appendChild(aMessageNode); >- aMessageNode.scrollIntoView(false); >+ throw new Error(ERRORS.CANNOT_GET_HUD + ": " + aMessage.hudId); here as well - maybe just report the error? >+ var messageObject = new HUDMessage(hudId, aType, "network", >+ aActivityObject); A new constructor like HUDMessage should be called 'WebConsoleMessage' >+ /** >+ * Display a log message object in this HUD. >+ * >+ * @param HUDMessage aMessage >+ * @returns void >+ */ >+ logMessage: function HUD_logMessage(aMessage) >+ { >+ var klass = "hud-msg-node hud-" + aMessage.level; >+ >+ var textMessage = aMessage.message; >+ var timestamp = ConsoleUtils.timestampString(aMessage.timestamp); >+ >+ var messageNode = this.makeHTMLNode("div"); >+ messageNode.setAttribute("class", klass); >+ >+ if (!textMessage) { >+ switch (aMessage.origin) { >+ case "console-listener": >+ textMessage = this.logConsoleMessage(aMessage); >+ break; >+ >+ case "network": >+ textMessage = this.logNetMessage(aMessage, messageNode); >+ break; >+ >+ case "HUDConsole": >+ textMessage = this.logHUDConsoleMessage(aMessage); >+ break; >+ >+ case "jsterm-execute": >+ textMessage = "> " + aMessage.message; >+ break; I see. very nice. >+ >+ case "jsterm-result": >+ textMessage = "< " + aMessage.message; >+ break;} >+ } cool. >+ >+ /** >+ * Display a message originating from a console-listener. >+ * >+ * @private >+ * @param HUDMessage aMessage >+ * @returns string The textual message that needs to be displayed in the >+ * console. >+ */ >+ logConsoleMessage: function HUD_logConsoleMessage(aMessage) >+ { >+ if (!aMessage.activity) { >+ return; >+ } when will aMessage not have an activity property? >+ /** >+ * Display messages originating from the window.console API. >+ * >+ * @private >+ * @param HUDMessage aMessage The message object that describes the log event. >+ * @returns string The textual message that needs to be displayed in the >+ * console. >+ */ >+ logHUDConsoleMessage: function HUD_logHUDConsoleMessage(aMessage) >+ { >+ // TODO: I expect this will grow to include expanded views for objects, not >+ // just each activity.toString(). please file a bug and add the bug number after the TODO: >-function JSTerm(aContext, aParentNode, aMixin) >+function JSTerm(aContext, aParentNode, aMixin, aHudId) since you are adding a new argument, you might want to change it to 'aWebConsoleId' >- this.writeOutput(str); >+ var messageObject = new HUDMessage(this.hudId, "log", "jsterm-execute", >+ null, str); reminder: s/WebConsoleMessage/HUDMessage/ we will do the rename patch before the next beta or 2 (I hope) > for (var prop in aConfig) { > if (!(typeof aConfig[prop] == "function")){ is this a possibility? will a function end up in there? > this[prop] = aConfig[prop]; > } > } > >- if (aConfig.logLevel == "network") { >+ if (aConfig.level == "network") { > this.transactions = { }; > if (aConfig.activity) { > this.transactions[aConfig.activity.stage] = aConfig.activity; > } > } > I like these changes. Will get r+ with changes and answers

Comment on attachment 459449 [details] [diff] [review] split 4: fix the HUD weak refs This includes Julian's patch right? what is that bug number?

Comment on attachment 459452 [details] [diff] [review] split 5: code cleanup We will have to be very careful applying these patches - do they apply in the order from 1 -5? > * HUDMessage represents a single message logged in the HUD console. > * > * @param string aHudId the ID of the HUD this message belongs to > * @param string aLevel message level: info, notice, warning, error, etc. > * @param string aOrigin message origin: network, console-listener, etc. > * @param string [aActivity] the activity object associated to this message. > * @param string [aMessage] the message this object holds.

All in all looking really good. SO MUCH EASIER to review like this. Thanks for taking the time.

(In reply to comment #16) > please reference "Web Console" in these error messages instead of "Heads Up > Display" Ah, yes. Will do. > >+ throw new Error(ERRORS.HUD_WEAK_REFERENCE_FAIL + ": " + aMessage.hudId); > I'm not sure we should throw here as we potentially would be throwing a lot. > You might want to use Cu.reportError Agreed. I believe we won't throw a lot, but your idea is good. Will switch to Cu.reportError(). (I haven't seen any throws yet!) > > } > > } > > else { > >- // log everything > >- aConsoleNode.appendChild(aMessageNode); > >- aMessageNode.scrollIntoView(false); > >+ throw new Error(ERRORS.CANNOT_GET_HUD + ": " + aMessage.hudId); > here as well - maybe just report the error? Yes. > >+ var messageObject = new HUDMessage(hudId, aType, "network", > >+ aActivityObject); > A new constructor like HUDMessage should be called 'WebConsoleMessage' Yes. > >+ /** > >+ * Display a message originating from a console-listener. > >+ * > >+ * @private > >+ * @param HUDMessage aMessage > >+ * @returns string The textual message that needs to be displayed in the > >+ * console. > >+ */ > >+ logConsoleMessage: function HUD_logConsoleMessage(aMessage) > >+ { > >+ if (!aMessage.activity) { > >+ return; > >+ } > when will aMessage not have an activity property? This is a sanity check only. I don't expect it will *not* have an activity, but it's better to return rather than throw. (the code takes the existence of .activity for granted after the check). Should I remove the check? > >+ /** > >+ * Display messages originating from the window.console API. > >+ * > >+ * @private > >+ * @param HUDMessage aMessage The message object that describes the log event. > >+ * @returns string The textual message that needs to be displayed in the > >+ * console. > >+ */ > >+ logHUDConsoleMessage: function HUD_logHUDConsoleMessage(aMessage) > >+ { > >+ // TODO: I expect this will grow to include expanded views for objects, not > >+ // just each activity.toString(). > please file a bug and add the bug number after the TODO: Ah, yes. Will do. > >-function JSTerm(aContext, aParentNode, aMixin) > >+function JSTerm(aContext, aParentNode, aMixin, aHudId) > > since you are adding a new argument, you might want to change it to > 'aWebConsoleId' Agreed. > >- this.writeOutput(str); > >+ var messageObject = new HUDMessage(this.hudId, "log", "jsterm-execute", > >+ null, str); > reminder: s/WebConsoleMessage/HUDMessage/ > we will do the rename patch before the next beta or 2 (I hope) Cool. > > for (var prop in aConfig) { > > if (!(typeof aConfig[prop] == "function")){ > is this a possibility? will a function end up in there? This is not from my patch. It's code in ConsoleEntry, in mozilla-central. On a related note, ConsoleEntry should just be removed. When time allows, please see my two proposals in bug 552143. The grand idea is to get, some day, the second proposal for ConsoleStorage reviewed and integrated. Raw WebConsoleMessage objects would be stored directly - no need for ConsoleEntry wrapping. (these patches are made in this idea.) > I like these changes. Will get r+ with changes and answers Great! Thank you very much for your review. Will post updated patch ASAP.

(In reply to comment #17) > Comment on attachment 459449 [details] [diff] [review] > split 4: fix the HUD weak refs > > This includes Julian's patch right? what is that bug number? Yes, but it's not a direct inclusion. His patch removed HeadsUpDisplay.reattachConsole() and I think it introduced regressions. I tried to follow the entire logic of the code and I reached the conclusion it needs a bit of fine-tuning. My patch includes updates for the contentWindow of the HUD object, and for the .context and .sandbox properties of hud.jsterm. Nonetheless, I give credit to Julian for the great idea - his patch fixes the issues I had with my initial patch - HUD weak refs were garbage collected. Julian reported bug 580618 and provided his patch proposal there.

(In reply to comment #18) > Comment on attachment 459452 [details] [diff] [review] > split 5: code cleanup > > We will have to be very careful applying these patches - do they apply in the > order from 1 -5? Yes, correct. They apply in order, cleanly on mozilla-central. Agreed: special care should be taken - regression testing and the likes.

Updated patch, according to feedback. Please let me know if further changes are needed. Patch split 4 still applies cleanly on top of this one - no need for updates. Patch split 5 needs an update. Will follow.

Updated patch. Rebased the patch for split 3b. Also, now the JSTerm.clearOutput() method is no longer removed - it's used by test code.

Comment on attachment 459568 [details] [diff] [review] split 3b: switch to the new WebConsoleMessage object >+ case "jsterm-result": >+ textMessage = "< " + aMessage.message; I think you should provide a default here, and add a "break" under "jsterm-result" >+ } >+ } >+ >+ var textNode = this.chromeDocument.createTextNode(timestamp + ": " + r+ with changes

Updated the patch as requested. Again, all tests pass and the rest of the patches do not need rebasing. Thanks for your feedback!

Comment on attachment 459570 [details] [diff] [review] split 5b: code cleanup Gavin: please apply each patch in the other of the split number. This means that split 1 is to be applied on a clean default branch from mozilla-central. Split 2 should then be applied, and so on. Each patch should work fine, and it should apply cleanly. Thank you!

Attention drivers: this bug IMO should land as soon as b3 is tagged. A lot of further patches depend on this. I had it marked lower priority on accident.

Given the current timing, the size of this patch and the fact that we're no longer dependent on this in order to make console filtering fast, I'm pulling this bug from our current scheduled queue. If something else comes up that raises the priority of the bug, let me know.

Apologies, this patch has also bitrotted since attaching it. Please refresh to current trunk and reattach? Also, I'm not a toolkit peer, so this should be reviewed by someone else to make it fully official. :)

Comment on attachment 459849 [details] [diff] [review] split 3c: switch to the new WebConsoleMessage object >+ var result = msgLevel + " " + aMessage.activity.errorMessage + " " + >+ errFile + " " + lineCol + " " + msgCategory + " " + >+ aMessage.activity.category; Small suggestion: for the various functions that do this, you might consider: var result = [foo, bar, baz].join(" ");

Dolske: no worries. Thanks! Will update the patches and ask someone else to review them. :)

Rebased patch. Asking a toolkit reviewer for review, as Dolske suggested.

Rebased patch.

Rebased patch. It includes changes suggested by Dolske and output improvements, as in the main mozilla-central branch.

Rebased patch, with some more fixes.

Rebased patch.

Comment on attachment 466275 [details] [diff] [review] split 1b: HUDService.logActivity() removal r=sdwilsh

Comment on attachment 466276 [details] [diff] [review] split 2b: merge HUDService.logHUDMessage() into logMessage() >+ // we have successfully filtered a message, we need to log it nit: if you are going to write so much, please use sentences that are correct grammatically please? This is a run-on. >+ // we need to ignore this message by changing its css class - we are >+ // still logging this, it is just hidden ditto >+ // store this message in the storage module: >+ this.storage.recordEntry(aMessage.hudId, aMessage); nit: colon also seems weird here r=sdwilsh

Comment on attachment 466277 [details] [diff] [review] split 3d: switch to the new WebConsoleMessage object global-nit: your bracing style of if statements is inconsistent. Pick one (whatever is file-dominant) and stick with it please. >+ logMessage: function HS_logMessage(aMessage) > { > if (!aMessage) { > throw new Error(ERRORS.MISSING_ARGS); Unrelated to this patch, but you folks might want to consider using Components.Exception (https://developer.mozilla.org/en/Components.Exception) instead of new Error and pass in Components.stack.caller as the stack so the call site reports the exception instead of your code. We use this in NetUtil.jsm like so: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#83 >+ /** >+ * Display a message originating from a console-listener. >+ * >+ * @private >+ * @param WebConsoleMessage aMessage >+ * @param object aOutput The log message output object given by the >+ * this.logMessage method. This object is used to control the output of the >+ * message we are logging. >+ * @returns string The textual message that needs to be displayed in the >+ * console. >+ */ [global] This doesn't follow the formatting of other comments in this patch (namely, there should be a newline after the variable name and the description. In a few of these, you don't give a description of aMessage either. >+ logConsoleMessage: function HUD_logConsoleMessage(aMessage, aOutput) >+ { >+ if (!aMessage.activity) { >+ return; >+ } In what situations would this not be a bug? Applies to at least one other instance of this too. Holding off on review until at least the last question is answered.

(In reply to comment #41) > Comment on attachment 466277 [details] [diff] [review] > split 3d: switch to the new WebConsoleMessage object > > global-nit: your bracing style of if statements is inconsistent. Pick one > (whatever is file-dominant) and stick with it please. > > >+ logMessage: function HS_logMessage(aMessage) > > { > > if (!aMessage) { > > throw new Error(ERRORS.MISSING_ARGS); > Unrelated to this patch, but you folks might want to consider using > Components.Exception (https://developer.mozilla.org/en/Components.Exception) > instead of new Error and pass in Components.stack.caller as the stack so the > call site reports the exception instead of your code. We use this in > NetUtil.jsm like so: > http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#83 > > >+ /** > >+ * Display a message originating from a console-listener. > >+ * > >+ * @private > >+ * @param WebConsoleMessage aMessage > >+ * @param object aOutput The log message output object given by the > >+ * this.logMessage method. This object is used to control the output of the > >+ * message we are logging. > >+ * @returns string The textual message that needs to be displayed in the > >+ * console. > >+ */ > [global] This doesn't follow the formatting of other comments in this patch > (namely, there should be a newline after the variable name and the description. > In a few of these, you don't give a description of aMessage either. David told me you're still waiting my reply. Sorry for the delay. I did not reply for a simple reason: I got caught-up in higher-priority changes for the Inspector (the new Style panel), and I couldn't apply the changes you requested, and this patch does not include new strings - string freeze comes soon! You have good points. I'll apply the changes when we get back to this patch. > >+ logConsoleMessage: function HUD_logConsoleMessage(aMessage, aOutput) > >+ { > >+ if (!aMessage.activity) { > >+ return; > >+ } > In what situations would this not be a bug? Applies to at least one other > instance of this too. Hm, correct. Will need to remove this checks actually. If aMessage comes to logConsoleMessage without the .activity object, then that's a bug that needs to be fixed - not expected behavior. AFAIK, that won't happen. Will update the patch when the time comes for this bug again. Thank you very much for your review!

Canceling requests until I get the updates promised in comment 42

Patrick: is this bug covered completely by the work in bug 605621? If so, we should probably just close this one since this is just a code cleanup bug as it is.

(In reply to comment #44) > Patrick: is this bug covered completely by the work in bug 605621? If so, we > should probably just close this one since this is just a code cleanup bug as it > is. Yes, it is. The patches here are also very out of date.

OK, then we'll say goodnight to this bug and I'll mark it a dupe so that people know where the actual work ended up.