Closed
Bug 589162
Opened 14 years ago
Closed 14 years ago
CSS filtering on the console does not work
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(blocking2.0 betaN+)
VERIFIED
FIXED
Firefox 4.0b7
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: dangoor, Assigned: msucan)
References
Details
(Whiteboard: [patchclean:1012] [Web-Console-Testday])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
the first thing we need to do is create a failing test - anytime this kind of regression happens.
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → pwalton
Reporter | ||
Comment 2•14 years ago
|
||
Requesting blocking status for this, since there's a UI element that is non-functional...
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → betaN+
Reporter | ||
Updated•14 years ago
|
Severity: normal → blocker
Reporter | ||
Updated•14 years ago
|
Assignee: pwalton → mihai.sucan
Reporter | ||
Comment 3•14 years ago
|
||
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Assignee | ||
Comment 4•14 years ago
|
||
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.
Attachment #472466 -
Flags: feedback?(ddahl)
Reporter | ||
Updated•14 years ago
|
Blocks: devtools4b7
Reporter | ||
Updated•14 years ago
|
Attachment #472466 -
Flags: review?(sdwilsh)
Comment 5•14 years ago
|
||
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.
Attachment #472466 -
Flags: review?(sdwilsh)
Attachment #472466 -
Flags: review+
Attachment #472466 -
Flags: feedback?(ddahl)
Assignee | ||
Comment 6•14 years ago
|
||
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.
Attachment #472466 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 7•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6
Reporter | ||
Comment 8•14 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Updated•14 years ago
|
Whiteboard: [kd4b6]
Assignee | ||
Comment 9•14 years ago
|
||
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?
Reporter | ||
Comment 10•14 years ago
|
||
Perhaps it got rolled back with unrelated changes?
Comment 11•14 years ago
|
||
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
Updated•14 years ago
|
Whiteboard: [check patch for bitrot before checkin]
Assignee | ||
Comment 12•14 years ago
|
||
This is the rebased patch.
Attachment #473111 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [check patch for bitrot before checkin] → [patchclean:1004]
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [patchclean:1004] → [patchclean:1004][can-land-post-b7]
Comment 13•14 years ago
|
||
(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.
Keywords: checkin-needed
Whiteboard: [patchclean:1004][can-land-post-b7] → [patchclean:1004]
Reporter | ||
Comment 14•14 years ago
|
||
This patch has already gone through review, etc. Marking as checkin-needed now that the tree is open again.
Keywords: checkin-needed
Assignee | ||
Comment 15•14 years ago
|
||
Rebased patch. Only test file changes.
Attachment #480616 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:1004] → [patchclean:1012]
Updated•14 years ago
|
Version: unspecified → Trunk
Comment 16•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•14 years ago
|
Status: RESOLVED → VERIFIED
Whiteboard: [patchclean:1012] → [patchclean:1012] [Web-Console-Testday]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•