Bug 193476

Summary: Web Inspector: improve invalid Audit/Recording JSON error messages
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 190754, 193686    
Bug Blocks: 173807    
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied
none
Patch
none
Patch none

Description Devin Rousso 2019-01-15 17:43:14 PST
We should log more explicit error messages to the console when importing/processing Audit/Recording JSON payloads.
Comment 1 Radar WebKit Bug Importer 2019-01-15 17:44:25 PST
<rdar://problem/47303659>
Comment 2 Devin Rousso 2019-01-23 15:44:53 PST
Created attachment 359962 [details]
Patch
Comment 3 Devin Rousso 2019-01-23 15:45:03 PST
Created attachment 359963 [details]
[Image] After Patch is applied
Comment 4 Joseph Pecoraro 2019-01-24 17:28:52 PST
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.
Comment 5 Devin Rousso 2019-01-24 23:38:51 PST
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 :|
Comment 6 Devin Rousso 2019-01-24 23:40:18 PST
Created attachment 360086 [details]
Patch
Comment 7 Devin Rousso 2019-01-24 23:51:33 PST
Created attachment 360087 [details]
Patch
Comment 8 WebKit Commit Bot 2019-01-25 01:27:55 PST
Comment on attachment 360087 [details]
Patch

Clearing flags on attachment: 360087

Committed r240471: <https://trac.webkit.org/changeset/240471>
Comment 9 WebKit Commit Bot 2019-01-25 01:27:57 PST
All reviewed patches have been landed.  Closing bug.