WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
193476
Web Inspector: improve invalid Audit/Recording JSON error messages
https://bugs.webkit.org/show_bug.cgi?id=193476
Summary
Web Inspector: improve invalid Audit/Recording JSON error messages
Devin Rousso
Reported
2019-01-15 17:43:14 PST
We should log more explicit error messages to the console when importing/processing Audit/Recording JSON payloads.
Attachments
Patch
(46.04 KB, patch)
2019-01-23 15:44 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
[Image] After Patch is applied
(773.35 KB, image/png)
2019-01-23 15:45 PST
,
Devin Rousso
no flags
Details
Patch
(50.82 KB, patch)
2019-01-24 23:40 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(52.07 KB, patch)
2019-01-24 23:51 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-01-15 17:44:25 PST
<
rdar://problem/47303659
>
Devin Rousso
Comment 2
2019-01-23 15:44:53 PST
Created
attachment 359962
[details]
Patch
Devin Rousso
Comment 3
2019-01-23 15:45:03 PST
Created
attachment 359963
[details]
[Image] After Patch is applied
Joseph Pecoraro
Comment 4
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.
Devin Rousso
Comment 5
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 :|
Devin Rousso
Comment 6
2019-01-24 23:40:18 PST
Created
attachment 360086
[details]
Patch
Devin Rousso
Comment 7
2019-01-24 23:51:33 PST
Created
attachment 360087
[details]
Patch
WebKit Commit Bot
Comment 8
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
>
WebKit Commit Bot
Comment 9
2019-01-25 01:27:57 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug