Bug 161358 – Web Inspector: Test.html should support globals reportInternalError, reportUnhandledRejection, reportUncaughtException

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

1. this function isn't defined in Test.js, just Main.js. So it doesn't work in tests.

[Error] TypeError: WebInspector.reportInternalError is not a function. (In 'WebInspector.reportInternalError(e, {"cause": `An uncaught exception was thrown while handling event: ${qualifiedName}`})', 'WebInspector.reportInternalError' is undefined)
	_dispatchEvent (TestCombined.js:5061)
	dispatch (TestCombined.js:4857)
	dispatchNextQueuedMessageFromBackend (TestCombined.js:5712)

We should just stub this out to call InspectorTest.reportUncaughtException, which notifies the test page and completes the test.

[Radar WebKit Bug Importer]

<rdar://problem/28066446>

[Blaze Burg]

Created attachment 297841 [details]
Proposed Fix

[Joseph Pecoraro]

Comment on attachment 297841 [details]
Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Test/FrontendTestHarness.js:194
> +        // Stop default handler so we can empty InspectorBackend's message queue.

Style: Newline before this.

> Source/WebInspectorUI/UserInterface/Test/FrontendTestHarness.js:201
> +        if (url === "undefined")

Wow, it is the string "undefined"? Thats crazy.

> Source/WebInspectorUI/UserInterface/Test/FrontendTestHarness.js:215
> +            result = `Uncaught exception in inspector page: ${message} [${sanitizedURL}:${lineNumber}:${columnNumber}]\n`;
> +        else
> +            result = `Uncaught exception in inspector page: ${message}\n`;

Nit: "inspector page" => "Inspector page"?

> Source/WebInspectorUI/UserInterface/Test/Test.js:120
> +window.reportUnhandledRejection = InspectorTest.reportUnhandledRejection.bind(InspectorTest);
> +window.onerror = InspectorTest.reportUncaughtExceptionFromEvent.bind(InspectorTest);

Maybe we should do something like this in TestStub.js for ProtocolTest. But I'm not too concerned about that.

> LayoutTests/http/tests/inspector/resources/inspector-test.js:93
> +            // Using this instead of window.onerror will preserve the stack trace.
> +            e.code = testFunction.toString();
> +            InspectorTest.reportUncaughtException(e);

Very interesting!

> LayoutTests/inspector/unit-tests/globals-uncaught-exception-from-timer-callback.html:10
> +    setTimeout(() => { throw new Error("This is an exception thrown in the inspector page."); }, 0);

This would read better to me on multiple lines.

Thoughts on just doing setTimeout without the ", 0" for cases like this? I've been doing that recently, I'm sure its leaked into tests.

    setTimeout(() => {
        throw new Error(...);
    });

> LayoutTests/inspector/unit-tests/globals-uncaught-exception-in-test-suite.html:12
> +    let suite = InspectorTest.createAsyncSuite("ReportUnhandledRejection");
> +    suite.addTestCase({
> +        name: "CatchRejectedPromise",

Suite name should be different from the one actually about promises. Something like ReportUncaughtExceptionInTestCase or TestSuite

> LayoutTests/inspector/unit-tests/globals-unhandled-rejection-in-timer-callback.html:10
> +    setTimeout(() => { reportUnhandledRejection(new Error("This is an exception thrown in the inspector page.")); }, 0);

Style: Same thing about multiline.

[Blaze Burg]

Created attachment 298116 [details]
For Landing

[Blaze Burg]

Committed r210367: <http://trac.webkit.org/changeset/210367>