Closed Bug 588730 Opened 14 years ago Closed 14 years ago

Adding a textNode to an existing xul:label causes an error in the WebConsole

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 final+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: julian.viereck, Assigned: pcwalton)

References

Details

(Whiteboard: [kd4b5] [patchclean:0826])

Attachments

(2 files, 2 obsolete files)

In bug 568634 I add a xul:textNodes to an existing xul:label in the WebConsole. This new insert node makes the

  this.outputNode.addEventListener("DOMNodeInserted", function(ev) {

throw an error (taken from an unit test):

TEST-INFO | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Console message: [JavaScript Error: "ev.target.classList is undefined" {file: "resource://gre/modules/HUDService.jsm" line: 2265}]

TextNodes doesn't have a classList object (== undefined) and therefore the check

line 2265:  if (ev.target.classList.contains("hud-msg-node")) {

failes.
Whiteboard: [kd4b5]
Attached patch Proposed patch. (obsolete) — Splinter Review
Patch attached.
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
Attachment #467949 - Flags: feedback?
Patrick, I haven't tried the patch but it looks good from just reading the code. This is not part of this bug, but if a new textNode is insert, the parent node of the textNode should get checked in case it matches the filtering rules or not.

Imagine a label has the text "foo" and filtering is set to "bar". The "foo"-label is not visible. Now the string "baring" is added to the "foo"-label. This now matches the search string "bar" and the "foo"-label should show up. Do you want me to fill a separate bug on that?
(In reply to comment #1)
> Created attachment 467949 [details] [diff] [review]
> Proposed patch.
> 
> Patch attached.

Patrick, I included your patch into my repo without the fix so that only the unit tests run. They passed although I could see some 

  Console message: [JavaScript Error: "ev.target.classList is undefined" {file: "resource://gre/modules/HUDService.jsm" line: 3005}]

which means that the unit test doesn't work. I tried to figure out why the unit test is not working (adding the textNode after a timeout of 1 sec and see if an error occurred) but I couldn't. Let's discuss this later.
This patch is based on Patrick's submission with some modifications that should make the unit test fail - but they don't.

Patrick, can you reproduce that this isn't failing?
Whiteboard: [kd4b5] → [kd4b6]
Whiteboard: [kd4b6] → [kd4b6] [patchbitrot]
Attached patch Proposed patch, version 2. (obsolete) — Splinter Review
New patch attached. Unit tests seem to pass on my machine; try is borked due to bug 591074 so I can't be 100% confident at this point.
Attachment #467949 - Attachment is obsolete: true
Attachment #469643 - Flags: feedback?
Attachment #467949 - Flags: feedback?
Whiteboard: [kd4b6] [patchbitrot] → [kd4b6] [patchclean:0826]
(In reply to comment #5)
> Created attachment 469643 [details] [diff] [review]
> Proposed patch, version 2.
> 
> New patch attached. Unit tests seem to pass on my machine; try is borked due to
> bug 591074 so I can't be 100% confident at this point.

Do you have a windows build and linux build to try it out on? I think we should all have them at this point:(
Whiteboard: [kd4b6] [patchclean:0826] → [kd4b6] [patchbitrot]
Patch rebased to trunk.
Attachment #469643 - Attachment is obsolete: true
Attachment #469696 - Flags: feedback?
Attachment #469643 - Flags: feedback?
Whiteboard: [kd4b6] [patchbitrot] → [kd4b6] [patchclean:0826]
Blocks: 568634
blocking2.0: --- → ?
This bug is blocking blockers (some even designated as beta5+)
Whiteboard: [kd4b6] [patchclean:0826] → [kd4b5] [patchclean:0826]
Attachment #469696 - Flags: review?(dietrich)
Attachment #469696 - Flags: feedback?(mihai.sucan)
Attachment #469696 - Flags: feedback?
Comment on attachment 469696 [details] [diff] [review]
Proposed patch, version 2.1.

This patch looks fine to me.

As far as I know the listener in the test code does not need to be an object with the observe method. I think you can directly pass the function. (this works with the observer service - might work with the console service listeners as well)

Also, you manually get the nsIConsoleService component. You should get it much quicker with Services.console from Services.jsm.
Attachment #469696 - Flags: feedback?(mihai.sucan) → feedback+
Attachment #469696 - Flags: review?(dietrich) → review+
Keywords: checkin-needed
blocking2.0: ? → final+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: