[webkit-reviews] review granted: [Bug 215555] Web Inspector: Audit: should be able to create/edit imported audits : [Attachment 407278] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 215555: Web Inspector: Audit: should be able to create/edit imported audits
https://bugs.webkit.org/show_bug.cgi?id=215555

Attachment 407278: Patch

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




--- Comment #14 from Brian Burg <bburg at apple.com> ---
Comment on attachment 407278
  --> https://bugs.webkit.org/attachment.cgi?id=407278
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