578658 - proposal for consolidating HUD log message objects

Status: RESOLVED DUPLICATE of bug 605621

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

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!

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed implementation

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.

Comment 2

[David Dahl :ddahl]

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.

Comment 3

[Patrick Walton (:pcwalton)]

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.

Comment 4

[Julian Viereck]

(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...

Comment 5

[Julian Viereck]

Attachment: Patch part1

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

Comment 6

[Julian Viereck]

Attachment: Patch part 2

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

Comment 7

[Patrick Walton (:pcwalton)]

(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.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: split 1: HUDService.logActivity() removal

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.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: split 2: merge HUDService.logHUDMessage() into logMessage()

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.

Comment 10

[Mihai Sucan [:msucan]]

Attachment: split 3: switch to the new HUDMessage object

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.)

Comment 11

[Mihai Sucan [:msucan]]

Attachment: split 4: fix the HUD weak refs

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.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: split 5: code cleanup

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 13

[David Dahl :ddahl]

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 14

[David Dahl :ddahl]

Comment on attachment 459439 [details] [diff] [review]
split 2: merge HUDService.logHUDMessage() into logMessage()

nice work. much easier to review like this.

Comment 15

[Mihai Sucan [:msucan]]

(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 16

[David Dahl :ddahl]

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 17

[David Dahl :ddahl]

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 18

[David Dahl :ddahl]

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.

Comment 19

[David Dahl :ddahl]

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

Comment 20

[Mihai Sucan [:msucan]]

(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.

Comment 21

[Mihai Sucan [:msucan]]

(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.

Comment 22

[Mihai Sucan [:msucan]]

(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.

Comment 23

[Mihai Sucan [:msucan]]

Attachment: split 3b: switch to the new WebConsoleMessage object

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.

Comment 24

[Mihai Sucan [:msucan]]

Attachment: split 5b: code cleanup

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 25

[David Dahl :ddahl]

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

Comment 26

[Mihai Sucan [:msucan]]

Attachment: split 3c: switch to the new WebConsoleMessage object

Updated the patch as requested. Again, all tests pass and the rest of the patches do not need rebasing.

Thanks for your feedback!

Comment 27

[Mihai Sucan [:msucan]]

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!

Comment 29

[David Dahl :ddahl]

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.

Comment 30

[Kevin Dangoor]

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.

Comment 31

[Justin Dolske [:Dolske]]

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 32

[Justin Dolske [:Dolske]]

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(" ");

Comment 33

[Mihai Sucan [:msucan]]

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

Comment 34

[Mihai Sucan [:msucan]]

Attachment: split 1b: HUDService.logActivity() removal

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

Comment 35

[Mihai Sucan [:msucan]]

Attachment: split 2b: merge HUDService.logHUDMessage() into logMessage()

Rebased patch.

Comment 36

[Mihai Sucan [:msucan]]

Attachment: split 3d: switch to the new WebConsoleMessage object

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

Comment 37

[Mihai Sucan [:msucan]]

Attachment: split 4b: fix the HUD weak refs

Rebased patch, with some more fixes.

Comment 38

[Mihai Sucan [:msucan]]

Attachment: split 5c: code cleanup

Rebased patch.

Comment 39

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

Comment 40

[Shawn Wilsher :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 41

[Shawn Wilsher :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.

Comment 42

[Mihai Sucan [:msucan]]

(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!

Comment 43

[Shawn Wilsher :sdwilsh]

Canceling requests until I get the updates promised in comment 42

Comment 44

[Kevin Dangoor]

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.

Comment 45

[Patrick Walton (:pcwalton)]

(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.

Comment 46

[Kevin Dangoor]

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.