Bug 151923 - Web Inspector: Uncaught Exception page should have better styles and handle more error cases
Summary: Web Inspector: Uncaught Exception page should have better styles and handle m...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks: InspectorDebug
  Show dependency treegraph
 
Reported: 2015-12-06 10:34 PST by Brian Burg
Modified: 2015-12-07 12:57 PST (History)
8 users (show)

See Also:


Attachments
[Screenshot] different styling (308.60 KB, image/png)
2015-12-06 13:52 PST, Brian Burg
no flags Details
Proposed Fix (27.40 KB, patch)
2015-12-06 14:05 PST, Brian Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2015-12-06 10:34:55 PST
WIP patch matches Safari error page font/colors better.
Comment 1 Radar WebKit Bug Importer 2015-12-06 10:35:07 PST
<rdar://problem/23776690>
Comment 2 Brian Burg 2015-12-06 13:52:10 PST
Created attachment 266735 [details]
[Screenshot] different styling
Comment 3 Brian Burg 2015-12-06 14:05:31 PST
Created attachment 266736 [details]
Proposed Fix
Comment 4 WebKit Commit Bot 2015-12-06 14:06:42 PST
Attachment 266736 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.js:145:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.js:157:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.js:158:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Joseph Pecoraro 2015-12-07 11:06:02 PST
Comment on attachment 266736 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=266736&action=review

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:374
> -        m_frontendPage->mainFrame().script().executeScript("InspectorFrontendAPI.dispatch(" + expression + ")");
> +        m_frontendPage->mainFrame().script().executeScript("if (InspectorFrontendAPI) InspectorFrontendAPI.dispatch(" + expression + ")");

This kinda bugs me. It shouldn't be possible.

> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.css:26
> +div.sheet-container {

I think all the "div." prefix on all of these just adds clutter. The class name alone is good enough. Are they there for any particular reason?

> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.css:50
> +    font-size: 97%;

This is a rather weird value.

> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.js:26
> +// Hide these functions from global scope.

Comment not necessary.

> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.js:147
> +    let sheetElement = window.__sheetElement = document.createElement("div");

An uncaught exception doesn't always make the inspector totally unusable. Multiple times last week I hit new uncaught exceptions unrelated to my changes and it made working on the inspector more difficult since I had to reload / workaround the bugs.

I'm glad to see this doesn't overwrite the entire document like it did previously. Maybe we could allow closing this overlay to continue using the inspector despite the exception?

Also, can we select text in the sheet?

> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.js:167
> +    <img src="Images/Console.svg" />

No need for "/>" on self-closing tags.

> Source/WebInspectorUI/UserInterface/Protocol/MessageDispatcher.js:72
> +    // If something has gone wrong and the uncaught exception sheet is showing,
> +    // then don't try to dispatch more messages. It will probably just cause more
> +    // spurious uncaught exceptions.
> +    if (this.__uncaughtExceptions)
> +        return;

If we did allow closing the sheet, then this would need to be removed. Again, one uncaught exception doesn't always break the entire inspector.
Comment 6 Brian Burg 2015-12-07 11:16:17 PST
Comment on attachment 266736 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=266736&action=review

>> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:374
>> +        m_frontendPage->mainFrame().script().executeScript("if (InspectorFrontendAPI) InspectorFrontendAPI.dispatch(" + expression + ")");
> 
> This kinda bugs me. It shouldn't be possible.

It happens sometimes, especially when reloading the frontend. I think we have seen tests mysteriously hit this as well. Might be worth understanding deeper.

>> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.css:26
>> +div.sheet-container {
> 
> I think all the "div." prefix on all of these just adds clutter. The class name alone is good enough. Are they there for any particular reason?

no

>> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.css:50
>> +    font-size: 97%;
> 
> This is a rather weird value.

it makes the underline somewhat less attention-seeking by keeping it at about the same em size.

>> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.js:26
>> +// Hide these functions from global scope.
> 
> Comment not necessary.

Oops.

> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.js:94
> +    unblockEventHandlers();

This is the code that closes the overlay.

>> Source/WebInspectorUI/UserInterface/Debug/UncaughtExceptionReporter.js:147
>> +    let sheetElement = window.__sheetElement = document.createElement("div");
> 
> An uncaught exception doesn't always make the inspector totally unusable. Multiple times last week I hit new uncaught exceptions unrelated to my changes and it made working on the inspector more difficult since I had to reload / workaround the bugs.
> 
> I'm glad to see this doesn't overwrite the entire document like it did previously. Maybe we could allow closing this overlay to continue using the inspector despite the exception?
> 
> Also, can we select text in the sheet?

The dismissOptionHTML allows clicking to hide the overlay and clear the saved exceptions. Do you want a more permanent way to disable it?

Text selection still doesn't work for unknown reasons. I was going to tackle that separately.

>> Source/WebInspectorUI/UserInterface/Protocol/MessageDispatcher.js:72
>> +        return;
> 
> If we did allow closing the sheet, then this would need to be removed. Again, one uncaught exception doesn't always break the entire inspector.

We do, and closing the sheet will clear window.__uncaughtExceptions (oops, typo!)
Comment 7 Brian Burg 2015-12-07 12:57:53 PST
Committed r193646: <http://trac.webkit.org/changeset/193646>