Web Inspector: Audit: should be able to create/edit imported audits

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

It's annoying that the only way to create/edit audits is to write it using a separate program and then import it into Web Inspector.  Considering the fact that there is a (somewhat) specific format, it would be nicer to have a dedicated UI for this.

[Devin Rousso]

Created attachment 406693 [details]
Patch

putting this up for review so that people can look while I write tests :)

[Devin Rousso]

Created attachment 406694 [details]
[Image] After Patch is applied (test case)

[Devin Rousso]

Created attachment 406695 [details]
[Image] After Patch is applied (group)

[Radar WebKit Bug Importer]

<rdar://problem/67255483>

[Devin Rousso]

Created attachment 406769 [details]
Patch

[BJ Burg]

Comment on attachment 406769 [details]
Patch

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

Overall this looks real nice. Holding off on r+ until we can iterate on the UI at least once. My main complaint is that the alignment is off and it looks very busy overall due to lack of whitespace.

In light of recent localization bugs, I'm being more stringent about labelling UIStrings with appropriate descriptions and/or keys.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:111
> +localizedStrings["Add Test Case"] = "Add Test Case";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:357
> +localizedStrings["Create"] = "Create";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:401
> +localizedStrings["Delete Group"] = "Delete Group";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:573
> +localizedStrings["Fail"] = "Fail";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:874
> +localizedStrings["Not yet run"] = "Not yet run";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:921
> +localizedStrings["Pass"] = "Pass";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1142
> +localizedStrings["Setup: "] = "Setup: ";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1247
> +localizedStrings["Supports: "] = "Supports: ";

Please add a better key / description that mentions Audits tab.

Nit: It seems weird to include the whitespace after the colon.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1259
> +localizedStrings["Test Case"] = "Test Case";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1373
> +localizedStrings["Unsupported"] = "Unsupported";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1410
> +localizedStrings["Warn"] = "Warn";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1531
> +localizedStrings["too new to run in this Web Inspector"] = "too new to run in this Web Inspector";

Please add a better key / description that mentions Audits tab.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:144
> +        this.dispatchEventToListeners(WI.AuditManager.Event.RunningStateChanged);

This should go inside a setter for this.runningState so we don't miss any cases, and don't fire events when the state has not actually changed. It's hard to know right now whether a noop would fire the event.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:175
> +        this.dispatchEventToListeners(WI.AuditManager.Event.RunningStateChanged);

Ditto (line 144)

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:197
> +        this.dispatchEventToListeners(WI.AuditManager.Event.RunningStateChanged);

Ditto (line 144)

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:-286
> -        WI.objectStores.audits.deleteObject(test);

I don't understand why this was removed.

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

Thanks.

> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:46
> +        this.checkSupports({warn: true});

Nit: this name is awkward. Maybe this.validateSupportedVersion ?

> Source/WebInspectorUI/UserInterface/Models/AuditTestGroup.js:211
> +    checkSupports(options = {})

Ditto the above complaint about this function name.

[Devin Rousso]

Comment on attachment 406769 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:111
>> +localizedStrings["Add Test Case"] = "Add Test Case";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:357
>> +localizedStrings["Create"] = "Create";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:401
>> +localizedStrings["Delete Group"] = "Delete Group";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:573
>> +localizedStrings["Fail"] = "Fail";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:874
>> +localizedStrings["Not yet run"] = "Not yet run";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:921
>> +localizedStrings["Pass"] = "Pass";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1142
>> +localizedStrings["Setup: "] = "Setup: ";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1247
>> +localizedStrings["Supports: "] = "Supports: ";
> 
> Please add a better key / description that mentions Audits tab.
> 
> Nit: It seems weird to include the whitespace after the colon.

OK

NIT: we normally do that if the text is used for a label right before a form control element so that things look nice without needing any CSS `margin` or anything (e.g. `<label>TEXT: <input></label>`)

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1259
>> +localizedStrings["Test Case"] = "Test Case";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1373
>> +localizedStrings["Unsupported"] = "Unsupported";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1531
>> +localizedStrings["too new to run in this Web Inspector"] = "too new to run in this Web Inspector";
> 
> Please add a better key / description that mentions Audits tab.

OK

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:144
>> +        this.dispatchEventToListeners(WI.AuditManager.Event.RunningStateChanged);
> 
> This should go inside a setter for this.runningState so we don't miss any cases, and don't fire events when the state has not actually changed. It's hard to know right now whether a noop would fire the event.

There isn't a `set runningState`.  All of the functions inside `WI.AuditManager` have checks to make sure the `_runningState` is in the expected state in order to change.

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:-286
>> -        WI.objectStores.audits.deleteObject(test);
> 
> I don't understand why this was removed.

oh oops yes i'll un-remove this

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

(☞ﾟヮﾟ)☞

>> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:46
>> +        this.checkSupports({warn: true});
> 
> Nit: this name is awkward. Maybe this.validateSupportedVersion ?

Yeah I agree.  `checkSupports` is basically supposed to "reset" the `_supported` value based on comparing `_supports` against `WI.AuditTestBase.Version` and `InspectorBackend.getVersion("Audit")`.  In a sense, `checkSupports` is literally what the function is doing (which is why I chose the name originally), but it does read a bit awkwardly.

Maybe `determineSupported`?

[Devin Rousso]

Created attachment 407168 [details]
Patch

[Devin Rousso]

Created attachment 407169 [details]
[Image] after Patch is applied (test case)

[Devin Rousso]

Created attachment 407170 [details]
[Image] after Patch is applied (group)

[Devin Rousso]

Created attachment 407171 [details]
Patch

forgot new files :(

[Devin Rousso]

Created attachment 407174 [details]
Patch

prevent Enter from inserting newlines into `contenteditable`

[Devin Rousso]

Created attachment 407278 [details]
Patch

[BJ Burg]

Comment on attachment 407278 [details]
Patch

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

r=me with a few small comments. Great work!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:569
> +localizedStrings["Export Result (%s)"] = "Export Result (%s)";

Please include more context for this string. The others without descriptions are OK because they explicitly say 'Audit".

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:703
> +localizedStrings["Import audit or result"] = "Import audit or result";

Please choose one or the other. We don't need two versions of this string.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:116
> +                if (test[WI.AuditTestBase.DefaultAuditSymbol])

I would prefer to wrap the symbol usage in a getter. This is an ugly way to expose a flag to clients.

Suggestions: .editable? .isDefaultAudit?

Can it be not editable and be a non-default audit?

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:284
> +        console.assert(this._tests.includes(test) || test[WI.AuditTestBase.DefaultAuditSymbol], test);

Ditto earlier comment about symbols.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:286
> +        if (test[WI.AuditTestBase.DefaultAuditSymbol]) {

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:1068
> +            test[WI.AuditTestBase.DefaultAuditSymbol] = true;

Alternatives:

test.editable = false?
test.markAsDefaultTest()?

> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:75
> +

NIt: extra newlines

> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:145
> +    get editable()

oh HAHA it's already here. Please convert to use this instead of the symbol when outside the class.

> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:259
> +        return this.constructor.fromPayload(this.toJSON());

LOL

> Source/WebInspectorUI/UserInterface/Views/AuditNavigationSidebarPanel.js:37
> +        return WI.UIString("Create", "Create @ Audit Tab Navigation Sidebar", "Title of button that creates a new audit.");

Beautiful.

> Source/WebInspectorUI/UserInterface/Views/AuditTabContentView.js:32
> +            disableBackForward: true,

Cool.

> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:77
> +            this._resultImageElement.title = WI.UIString("Editing audit");

Please add context that this is a tooltip.

> Source/WebInspectorUI/UserInterface/Views/AuditTestGroupContentView.js:66
> +

Nit: extra newlines

> Source/WebInspectorUI/UserInterface/Views/AuditTestGroupContentView.js:225
> +                this.representedObject.removeEventListener(WI.AuditTestGroup.Event.TestRemoved, this._handleTestGroupTestRemoved, this);

Will these listeners leak if a test group becomes non-editable after this content view is created? I have the inverse question about a test group that is not editable and later becomes editable, won't it be missing listeners?

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:224
> +        this._recordingAutoCaptureFrameCountInputElement.autosize();

Ohh, that's neat. I didn't realize this is already in the codebase.

> Source/WebInspectorUI/UserInterface/Views/CreateAuditPopover.js:95
> +        const testFunctionString = WI.AuditTestCase.stringifyFunction(testFunction, 8);

Nit: placeholderTestFunctionString ?

> Source/WebInspectorUI/UserInterface/Views/Main.css:230
> +.navigation-item-help:not(:first-child) {

This seems extremely obscure. Can this go in a more specific file / selector?

> LayoutTests/inspector/model/auditTestGroup-expected.txt:215
> +PASS: Test 0 should not be supported.

Nit: Test0

[Devin Rousso]

Comment on attachment 407278 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:569
>> +localizedStrings["Export Result (%s)"] = "Export Result (%s)";
> 
> Please include more context for this string. The others without descriptions are OK because they explicitly say 'Audit".

good idea

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:703
>> +localizedStrings["Import audit or result"] = "Import audit or result";
> 
> Please choose one or the other. We don't need two versions of this string.

The uppercase version is text shown in the UI, while the lowercase version is a tooltip.  I believe this is in accordance with HIG.

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:116
>> +                if (test[WI.AuditTestBase.DefaultAuditSymbol])
> 
> I would prefer to wrap the symbol usage in a getter. This is an ugly way to expose a flag to clients.
> 
> Suggestions: .editable? .isDefaultAudit?
> 
> Can it be not editable and be a non-default audit?

`WI.AuditTestBase.prototype.get editable` actually does check for `this[WI.AuditTestBase.DefaultAuditSymbol]`.  Currently, `editable` and `default` are mutually exclusive, but that isn't to say that in the future we might want to have other audits that are not `editable`.  In this case, I want to explicitly check for `default` because we're dealing with code related to the default audits, not anything about editability.

I'll add a getter for it :)

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:284
>> +        console.assert(this._tests.includes(test) || test[WI.AuditTestBase.DefaultAuditSymbol], test);
> 
> Ditto earlier comment about symbols.

see above

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:286
>> +        if (test[WI.AuditTestBase.DefaultAuditSymbol]) {
> 
> Ditto.

see above

>> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:1068
>> +            test[WI.AuditTestBase.DefaultAuditSymbol] = true;
> 
> Alternatives:
> 
> test.editable = false?
> test.markAsDefaultTest()?

I like `markAsDefault` :)

>> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:145
>> +    get editable()
> 
> oh HAHA it's already here. Please convert to use this instead of the symbol when outside the class.

see above

>> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:259
>> +        return this.constructor.fromPayload(this.toJSON());
> 
> LOL

ikr

>> Source/WebInspectorUI/UserInterface/Views/AuditNavigationSidebarPanel.js:37
>> +        return WI.UIString("Create", "Create @ Audit Tab Navigation Sidebar", "Title of button that creates a new audit.");
> 
> Beautiful.

(☞ﾟヮﾟ)☞

>> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:77
>> +            this._resultImageElement.title = WI.UIString("Editing audit");
> 
> Please add context that this is a tooltip.

good idea

>> Source/WebInspectorUI/UserInterface/Views/AuditTestGroupContentView.js:225
>> +                this.representedObject.removeEventListener(WI.AuditTestGroup.Event.TestRemoved, this._handleTestGroupTestRemoved, this);
> 
> Will these listeners leak if a test group becomes non-editable after this content view is created? I have the inverse question about a test group that is not editable and later becomes editable, won't it be missing listeners?

`editable` should never change

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:224
>> +        this._recordingAutoCaptureFrameCountInputElement.autosize();
> 
> Ohh, that's neat. I didn't realize this is already in the codebase.

(☞ﾟヮﾟ)☞

>> Source/WebInspectorUI/UserInterface/Views/CreateAuditPopover.js:95
>> +        const testFunctionString = WI.AuditTestCase.stringifyFunction(testFunction, 8);
> 
> Nit: placeholderTestFunctionString ?

Sure.  It's not exactly a placeholder as it is used if not changed, but the intention is that it will change, so yeah.

>> Source/WebInspectorUI/UserInterface/Views/Main.css:230
>> +.navigation-item-help:not(:first-child) {
> 
> This seems extremely obscure. Can this go in a more specific file / selector?

This really should only be about when multiple `.navigation-item-help` are inside a `.message-text-view`.  I'll make it more specific.

>> LayoutTests/inspector/model/auditTestGroup-expected.txt:215
>> +PASS: Test 0 should not be supported.
> 
> Nit: Test0

nice catch!

[Devin Rousso]

Created attachment 407514 [details]
Patch

[EWS]

Committed r266317: <https://trac.webkit.org/changeset/266317>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407514 [details].

[Devin Rousso]

Comment on attachment 407514 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:7
> +        Reviewed by Devin Rousso.

oh oops I put "Devin Rousso" instead of "Brian Burg" 😅