589162 - CSS filtering on the console does not work

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Kevin Dangoor]

STR:

1. open console
2. go to www.yahoo.com
3. see that there are some CSS errors
4. click the "CSS" checkbox
5. see that the CSS errors are still visible

Comment 1

[David Dahl :ddahl]

the first thing we need to do is create a failing test - anytime this kind of regression happens.

Comment 2

[Kevin Dangoor]

Requesting blocking status for this, since there's a UI element that is non-functional...

Comment 3

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: proposed fix + test code

This is the proposed fix, with automated test included.

Please let me know if further improvements are needed. Thanks!

I should note that I also fixed a bug in testLogEntry(): the aOutputNode.querySelector() that was added some time ago only returned the first group - it should find all of the messages in the outputNode, not just the first group.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 472466 [details] [diff] [review]
proposed fix + test code

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>@@ -4394,16 +4394,21 @@ LogMessage.prototype = {
>     var ts = ConsoleUtils.timestamp();
>     this.timestampedMessage = ConsoleUtils.timestampString(ts) + ": " +
>       this.message.message;
>     var messageTxtNode = this.textFactory(this.timestampedMessage);
> 
>     this.messageNode.appendChild(messageTxtNode);
> 
>     var klass = "hud-msg-node hud-" + this.level;
>+
>+    if (this.activityObject.category == "CSS Parser") {
>+      klass += " hud-cssparser";
>+    }
Hrm, klass is a poor name here.  Please pick a better one (classNames or something?).  Might even consider making it an array of classes to apply, then just do a .join(" ") on it.  Note that "class" itself isn't even a reserved word in JS: https://developer.mozilla.org/en/JavaScript/Reference/Reserved_Words

>+function testLogEntry(aOutputNode, aMatchString, aSuccessErrObj, onlyVisible, failIfFound)
Please add a javadoc style header for this please explaining the parameters.  Also, your new arguments don't follow the prefix standard this method had before.

r=sdwilsh with these fixed.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch with the changes requested in the review. Thanks Shawn!

I also moved the test out of the main HUDService test file, such that this patch doesn't collide with the work being done in bug 581069.

Comment 7

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/bccbfe0956bd

Comment 8

[Kevin Dangoor]

This doesn't appear to be fixed...

STR:

1. open the web console
2. go to http://cnn.com or any site with lots of CSS errors
3. uncheck the CSS button
4. CSS errors still appear in output

expected:
no CSS errors displayed in output

Comment 9

[Mihai Sucan [:msucan]]

Interesting. The patch for this bug is not in mozilla-central. I can't find the fix in the HUDService.jsm file, nor can I find the mochitest file itself. I tested the STR, and yes, I can confirm the failure.

The patch got lost somehow?

Comment 10

[Kevin Dangoor]

Perhaps it got rolled back with unrelated changes?

Comment 11

[Rob Campbell [:rc] (:robcee)]

this was apparently backed it immediately afterwards and the bug status was not changed. Need to recheck the other bugs mentioned in this backout:

http://hg.mozilla.org/mozilla-central/rev/99b5dc548631

Comment 12

[Mihai Sucan [:msucan]]

Attachment: rebased patch

This is the rebased patch.

Comment 13

[Patrick Walton (:pcwalton)]

(In reply to comment #11)
> this was apparently backed it immediately afterwards and the bug status was not
> changed. Need to recheck the other bugs mentioned in this backout:
> 
> http://hg.mozilla.org/mozilla-central/rev/99b5dc548631

The reason is that these patches exposed the leaks in the Web Console. All of those patches should now be safe to land.

Comment 14

[Kevin Dangoor]

This patch has already gone through review, etc. Marking as checkin-needed now that the tree is open again.

Comment 15

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch. Only test file changes.

Comment 16

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/0bef2c993bb5