Closed Bug 601177 Opened 11 years ago Closed 11 years ago

Errors/Warnings checkboxes are confusing

Categories

(DevTools :: General, defect)

x86
All
defect
Not set
normal

Tracking

(blocking2.0 betaN+)

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

People

(Reporter: bzbarsky, Assigned: msucan)

References

Details

(Whiteboard: [patchclean:1119])

Attachments

(1 file, 5 obsolete files)

STEPS TO REPRODUCE:

1)  Open web console.
2)  Load attached testcase.
3)  Uncheck the "Errors" checkbox.

ACTUAL RESULTS: Nothing

EXPECTED RESULTS: Hide the errors, while still showing the warnings

Looking at the text before the checkboxes more carefully, it's _possible_ that these checkboxes only apply to console.error/console.warning or something and not to _actual_ errors or warnings.  In which case we need to make the clearer in the UI (e.g. not putting them right after the "JS" checkbox) and we need some way to separate out actual JS warnings and errors.
blocking2.0: --- → ?
Blocks: devtools4b8
The "Errors" checkbox actually comes after the word "Console:" not after the "JS" checkbox. I will grant that the whole feature is called "Web Console", so it might not be completely clear that the items after "Console:" specifically relate to the console.* functions. But, I can't think of a clearer word there at the moment.

It is possible to just view JavaScript errors/warnings by unchecking the others, but it is not currently possible to specify that you just want to view JS errors (and not warnings). It seems like that adding that level of granularity would make that UI cumbersome.
> But, I can't think of a clearer word there at the moment.

I think that fundamentally mixing the webpage-triggered "console.whatever" stuff with the browser-logged messages is pretty unclear...  Perhaps the "Console" label should be "Webpage-triggered console output" or something?  Or just "window.console output"?

> It seems like that adding that level of granularity would make that UI
> cumbersome.

The thing is.... debug builds turn on JS strict warnings by default.  So for your typical crappy web site finding the errors is impossible due to the slew of warnings.  So for this console to be usable in a debug build it really needs that level of control somehow.
(In reply to comment #2)
> > It seems like that adding that level of granularity would make that UI
> > cumbersome.
> 
> The thing is.... debug builds turn on JS strict warnings by default.  So for
> your typical crappy web site finding the errors is impossible due to the slew
> of warnings.  So for this console to be usable in a debug build it really needs
> that level of control somehow.

OK, I didn't realize that debug builds did that. I can see where that would be a problem.
Interesting... I just discovered that the "Warnings" checkbox does affect CSS warnings, apparently just not JavaScript ones. Perhaps that is the easiest solution: just make turning off "Warnings" turn off *all* warnings, regardless of source.
I just spoke with ddahl. Going back to the original problem: it does indeed sound like script errors are possibly coming through the console at "warning" level or something along those lines, which is why filtering them out by unchecking "errors" didn't work. We should be able to get it filtering exactly as you were expecting initially...

sorry for the back and forth here. I just needed to get some additional clarification on what was going on.
It looks like these checkboxes are going to be going through a visual change (bug 599498). However, that won't fix the levels at which things are logged.

The fix to ensure that the levels are as expected:

1. network requests with an HTTP status of 4xx or 5xx should be logged at the error level
2. all other network requests are at the info level
3. CSS messages that contain "error" in them are logged at the error level. (from what ddahl was saying, these messages are not differentiated in any other way when they come in to the service)
4. all other CSS messages are at the warning level
5. JS messages should be logged at the error or warning level depending on the object passed in
Assignee: nobody → mihai.sucan
> 3. CSS messages that contain "error" in them are logged at the error level.
> (from what ddahl was saying, these messages are not differentiated in any other
> way when they come in to the service)
> 4. all other CSS messages are at the warning level

I'm not sure why you're making that distinction, exactly... All CSS messages are parse errors, no?
(In reply to comment #7)

> I'm not sure why you're making that distinction, exactly... All CSS messages
> are parse errors, no?

Sounds like it. Maybe we are reading into the messages too much e.g.:

Unknown property 'zoom'.  Declaration dropped.

vs:

Error in parsing value for 'background-image'.  Declaration dropped.

The results are the same: Declaration Dropped

An unknown property seemed like a warning to me, but the second message starts with "Error"
Well, so... the former is "unknown property" the latter is "unknown value", basically.  Both lead to the same result: declaration dropped.
Yeah, you're right, Boris. All the same level... fresh summary of the behavior we need to ensure:

1. network requests with an HTTP status of 4xx or 5xx should be logged at the
error level
2. all other network requests are at the info level
3. CSS messages are all at the error level
4. JS messages should be logged at the error or warning level depending on the
object passed in
maybe a bit of semantic quibblery, but I'd consider CSS messages to be warnings. Should we treat vendor-specific properties as errors?
I agree.  When looking for errors I never want CSS errors, and I don't think they should even be selected by default. If we have a 404 loading a CSS page, that's an error, but otherwise modern recommended authoring practices generate overwhelming noise here.
Currently all css parsing "errors" are treated as warnings. The existing error console also does this.
Yeah, I meant that if a user opens the error console, they should only get a subset of things.  Errors are probably the right set of things.  (I think for CSS warnings we should actually special-case -webkit prefix and some other such cases to suppress them, since they are virtually always noise.)

See also the useless "charCode from keydown" or whatever warning, which we construct and dispatch for every key you type on gmail-chat or twitter web interface...
See bug 601667 for where we're going with the toolbars.
Attached patch proposed patch (obsolete) — Splinter Review
Proposed patch. Notes:

1. network requests with an HTTP status of 4xx or 5xx are now logged at the
error level.

2. all other network requests are at the info level.

3. CSS messages are all at the warning level. (as it seems, this was the consensus in the comments above). Please note that David is correct saying that the css parser errors are already at warning level.

4. It seems that Gecko already filters-out CSS parser failures for all -vendor-prefixed CSS properties.

5. "JS messages should be logged at the error or warning level depending on the
object passed in." This is almost true. However, there's a bit of confusion in the code:

- we have nsIScriptError messages coming from the nsIConsoleService which have either error, warning or exception flag. We log them accordingly. JS exceptions are obviously logged at the exception level. The toolbar option for toggling JS messages only toggled JS exceptions, not errors or warnings.

The "Console:Errors" checkbox only toggles messages logged at error level.

- we have the window.onerror handler which catches more JS exceptions. It currently uses console.error() to log those messages. I change the code to use console.exception(). This allows one to toggle JS exceptions from both sources, as expected, with the "Page:JS" checkbox.

6. JS strict warnings were not caught due to an error in matching flags to localization strings. This bug is now fixed. These warnings are caught with the nsIConsoleService (nsIScriptErrors).

Please note that JS strict warnings coming from external scripts are not caught by the nsIConsoleService observer (they can't be matched to the current tab). The window.onerror handler does not catch them either.

We should file a bug for this, I presume?

7. We lack a checkbox for toggling Console:Exceptions. Currently, we cannot disable JS exceptions/console exceptions, unless one uses Page;JS (which is, btw, a misnomer).

To better fix the situation we would need to:
- categorize messages based on "source": network, css, js, console API.
- categorize messages into levels: log, info, warning, error, exception.

There's some confusion in the code with regards to JS errors and exceptions.

I'd like comments on the patch, and suggestions where to take things further. Thanks!
Attachment #483727 - Flags: feedback?(rcampbell)
Status: NEW → ASSIGNED
OS: Mac OS X → All
Whiteboard: [patchclean:1016]
in https://bugzilla.mozilla.org/attachment.cgi?id=483727&action=diff#a/toolkit/components/console/hudservice/HUDService.jsm_sec3

You're adding "warn" and "error" flags for strict and "typeError" and "typeStrict" for logLevel. This is somewhat confusing as you're reusing warn and error in the flags, typeError in level, though probably harder to fix without rewriting the code that makes use of this, as you say at the end of your comment.

> Please note that JS strict warnings coming from external scripts are not caught
> by the nsIConsoleService observer (they can't be matched to the current tab).
> The window.onerror handler does not catch them either.
>
> We should file a bug for this, I presume?

Yes.

I'm not sure we have the time to take this as far you'd like. Looks like what you have here should address most of it.
Comment on attachment 483727 [details] [diff] [review]
proposed patch

f+ with some reservations around the http status parsing (status code number is in a string that you're later comparing to numbers).

The aforementioned repetition of error, warn, and typeError.
Attachment #483727 - Flags: feedback?(rcampbell) → feedback+
Thanks Robert for the feedback+!

I agree with the reservations around the repetition of error/warn/typeError, etc... it's a work around making bigger changes. As explained in the comment, we could go for a far better cataloging of messages, but that would need quite some changes.

Will update the patch to include parseInt(status), to make the comparison safer.
Attached patch updated patch (obsolete) — Splinter Review
Updated patch. Added parseInt() for the status. Asking for review now.
Attachment #483727 - Attachment is obsolete: true
Attachment #484082 - Flags: review?(dietrich)
Whiteboard: [patchclean:1016] → [patchclean:1018]
> 4. It seems that Gecko already filters-out CSS parser failures for all
> -vendor-prefixed CSS properties.

Property names and @-rules.  And font descriptors.  Not anything else.

> The "Console:Errors" checkbox only toggles messages logged at error level.

It should also toggle exceptions.  The distinction between "error" and "exception" only matters if you're trying to catch them; if you're reporting them they're the same thing (in that you know by this point the exception is uncaught).

> - we have the window.onerror handler which catches more JS exceptions.

This should only see uncaught exceptions in the script; those should also show up in the console service.  Is that not the case?  If so, in what situation?

> We should file a bug for this, I presume?

For which?
(In reply to comment #21)
> > 4. It seems that Gecko already filters-out CSS parser failures for all
> > -vendor-prefixed CSS properties.
> 
> Property names and @-rules.  And font descriptors.  Not anything else.

that should be sufficient though, shouldn't it? Are we missing anything else that should be filtered?

> > The "Console:Errors" checkbox only toggles messages logged at error level.
> 
> It should also toggle exceptions.  The distinction between "error" and
> "exception" only matters if you're trying to catch them; if you're reporting
> them they're the same thing (in that you know by this point the exception is
> uncaught).

You're right. Maybe we don't need the exception category at all. Or should just combine the two levels.

...
> > We should file a bug for this, I presume?
> 
> For which?

"Please note that JS strict warnings coming from external scripts are not caught
by the nsIConsoleService observer (they can't be matched to the current tab).
The window.onerror handler does not catch them either."
(In reply to comment #21)
> > We should file a bug for this, I presume?
> 
> For which?

Please see bug 605241.
Blocking+, the changes here affect out-of-the-box usability of this feature significantly.
blocking2.0: ? → betaN+
Comment on attachment 484082 [details] [diff] [review]
updated patch


>                 data = [ httpActivity.url,
>-                         httpActivity.response.status ];
>+                  httpActivity.response.status ];

i prefer the current indentation, but will defer to whatever the current style approach of the console code is.

everything looks good otherwise, r=me.
Attachment #484082 - Flags: review?(dietrich) → review+
Attached patch rebased patch (obsolete) — Splinter Review
Thanks for the review+!

Rebased the patch and made the indentation change requested.
Attachment #484082 - Attachment is obsolete: true
Whiteboard: [patchclean:1018] → [patchclean:1115][checkin]
Whiteboard: [patchclean:1115][checkin] → [patchclean-enough:1118][checking-in]
Attached patch checkboxes (obsolete) — Splinter Review
patch for checkin
Attached file mochitest-browser-chrome log (obsolete) —
failures. See log contents for additional info.

TEST-INFO | already focused
TEST-INFO | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XrayWrapper [object Window]]) about:blank desired window: ([object ChromeWindow]) chrome://browser/content/browser.xul child window: ([object XrayWrapper [object Window]]) about:blank docshell visible: true

INFO TEST-START | Shutdown
Browser Chrome Test Summary
	Passed: 423
	Failed: 1
	Todo: 0

*** End BrowserChrome Test Results ***
INFO | automation.py | Application ran for: 0:00:47.393038
INFO | automation.py | Reading PID log: /var/folders/Bv/BvxnJzazGzeR6Yy4QRUEFU+++TI/-Tmp-/tmpFibz5qpidlog
WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected!

INFO | runtests.py | Running tests: end.
mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_601177_log_levels.js | Test timed out
Rebased patch. This fixes the error Rob saw.
Attachment #490652 - Attachment is obsolete: true
Attachment #491846 - Attachment is obsolete: true
Attachment #491847 - Attachment is obsolete: true
Depends on: 597460
Whiteboard: [patchclean-enough:1118][checking-in] → [patchclean:1119][checkin]
Blocks: 600183
Comment on attachment 491899 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/2b792c9d96b7
Attachment #491899 - Attachment description: rebased patch → [checked-in] rebased patch
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:1119][checkin] → [patchclean:1119]
Duplicate of this bug: 596908
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.