Web Inspector: Uncaught Exception page should have better styles and handle more error cases

• Component: Web Inspector
• Status: RESOLVED FIXED

[BJ Burg]

WIP patch matches Safari error page font/colors better.

[Radar WebKit Bug Importer]

<rdar://problem/23776690>

[BJ Burg]

Created attachment 266735 [details]
[Screenshot] different styling

[BJ Burg]

Created attachment 266736 [details]
Proposed Fix

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

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.

[BJ Burg]

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!)

[BJ Burg]

Committed r193646: <http://trac.webkit.org/changeset/193646>