Bug 193476 – Web Inspector: improve invalid Audit/Recording JSON error messages

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

We should log more explicit error messages to the console when importing/processing Audit/Recording JSON payloads.

[Radar WebKit Bug Importer]

<rdar://problem/47303659>

[Devin Rousso]

Created attachment 359962 [details]
Patch

[Devin Rousso]

Created attachment 359963 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

Comment on attachment 359962 [details]
Patch

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

r=me

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1056
> +localizedStrings["\u0022%s\u0022 expects a newer backend"] = "\u0022%s\u0022 expects a newer backend";
> +localizedStrings["\u0022%s\u0022 expects a newer frontend"] = "\u0022%s\u0022 expects a newer frontend";

I don't think developers will know what the frontend and backend are here. Some suggestions:

    The inspected page is not new enough to run <test>.
    This Web Inspector is not new enough to run <test>.

    <test> is too new to run on this inspected page. Please update your browser.
    <test> is too new to run in this Web Inspector. Please update your browser.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:47
> +        message = WI.UIString("Audit warning: %s").format(message);

I'd vote for "Audit Warning: " with capital, but maybe there is precedent across the system for non-capitals.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:-176
> -            if (!object) {
> -                WI.AuditManager.synthesizeError(WI.UIString("invalid JSON."));

Is there some output when someone tries to import invalid JSON somewhere else?

> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:32
> -        console.assert(supports === undefined || typeof supports === "number");
> +        console.assert(supports === undefined || (typeof supports === "number" && !isNaN(supports)));

Do we need to worry about NaN all over the place? NaN is not JSON serializeable? You're already checking if something is a number.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:53
> +            WI.AuditManager.synthesizeError(WI.UIString("\u0022%s\u0022 has a non-string \u0022%s\u0022 value").format(payload.name, WI.unlocalizedString("test")));

At optimized build time do we strip out the WI.unlocalizedStrings(...) calls? Maybe we should, heh.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:70
> +        if (typeof payload.disabled === "boolean")
> +            options.disabled = payload.disabled;

Hmm, can someone include a `disabled: true` in their JSON themselves to start some tests off as disabled by default? If so maybe this also could use a warning.

[Devin Rousso]

Comment on attachment 359962 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:32
>> +        console.assert(supports === undefined || (typeof supports === "number" && !isNaN(supports)));
> 
> Do we need to worry about NaN all over the place? NaN is not JSON serializeable? You're already checking if something is a number.

Good point!  Every path to this (other than the default audits) goes through a `JSON.parse`.  I'll remove them.

>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:53
>> +            WI.AuditManager.synthesizeError(WI.UIString("\u0022%s\u0022 has a non-string \u0022%s\u0022 value").format(payload.name, WI.unlocalizedString("test")));
> 
> At optimized build time do we strip out the WI.unlocalizedStrings(...) calls? Maybe we should, heh.

lol

>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:70
>> +            options.disabled = payload.disabled;
> 
> Hmm, can someone include a `disabled: true` in their JSON themselves to start some tests off as disabled by default? If so maybe this also could use a warning.

Yeah, they could do that.  One issue with this, however, is that restoring from the `WI.ObjectStore` also goes through this path, so we'd show warnings when opening WebInspector.  I think it's fine if they want to add a `disabled` key.  I think it's a fair to say that it's pretty self explanatory :|

[Devin Rousso]

Created attachment 360086 [details]
Patch

[Devin Rousso]

Created attachment 360087 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 360087 [details]
Patch

Clearing flags on attachment: 360087

Committed r240471: <https://trac.webkit.org/changeset/240471>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.