We should log more explicit error messages to the console when importing/processing Audit/Recording JSON payloads.
<rdar://problem/47303659>
Created attachment 359962 [details] Patch
Created attachment 359963 [details] [Image] After Patch is applied
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 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 :|
Created attachment 360086 [details] Patch
Created attachment 360087 [details] Patch
Comment on attachment 360087 [details] Patch Clearing flags on attachment: 360087 Committed r240471: <https://trac.webkit.org/changeset/240471>
All reviewed patches have been landed. Closing bug.