Summary: | Web Inspector: improve invalid Audit/Recording JSON error messages | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, hi, 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
Devin Rousso
2019-01-15 17:43:14 PST
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. |