[webkit-reviews] review denied: [Bug 190754] Web Inspector: Audit: create Audit Tab : [Attachment 353117] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 190754: Web Inspector: Audit: create Audit Tab
https://bugs.webkit.org/show_bug.cgi?id=190754

Attachment 353117: Patch

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




--- Comment #17 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 353117
  --> https://bugs.webkit.org/attachment.cgi?id=353117
Patch

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

This looks great! I played with it a bit, and the UI (while I understand it is
preliminary) already looks/works very well. r- for now, since this patch will
need to iterate a bit. In addition to the code comments and style nits, I have
some general feedback/questions:

- I noticed the pass/failed status icons appear on tree elements for the
AuditTestCase that just ran, in addition to 'run' itself. Maybe it will be
obvious to people that the icons in the test case's tree elements are for the
run that just completed. I can't think of an alternative UI, except for
automatically opening the run once the test case stops executing. This might be
surprising though.
- When clicking around in the DOM tree for a result, I expected to be able to
select nodes and use the keyboard to expand/collapse and navigate around. Is
there a reason the experience isn't similar to the Elements tab DOM tree?
- The AuditTestCaseGroup header is taller than the header for individual
results. This causes the height to "jump" when stepping up/down through the
tree in the navigation sidebar.
- I noticed some progress spinner code, but never saw one. The tests probably
run too fast to see it; should the spinner be added on a delay?
- The content views for audit groups and results need to be updated to work in
Dark Mode.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCaseResult.js:28
> +    constructor(testCaseOrInfo, level, data)

Overloading this constructor doesn't provide a benefit that I can see, and
comes at the cost of clarity.

I suggest breaking out parameters `name` and `description`. AuditTestCaseResult
is only constructed in two places: in AuditTestCase, which can just pass
this._name and this._description, and in AuditTestCaseResult.fromPayload.

> Source/WebInspectorUI/UserInterface/Models/AuditTestGroup.js:27
> +{

Could you simplify your design by having AuditTestCase and AuditTestGroup
inherit from a common base class? They share much of the same API. If this
would just overcomplicate things, please disregard.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:266
> +    getPropertyDescriptors(callback, options = {})

Is this RemoteObject refactoring necessary?

> Source/WebInspectorUI/UserInterface/Views/AuditNavigationSidebarPanel.js:32
> +	   super(identifier, displayName);

I think this is overkill, but it's your call.

> Source/WebInspectorUI/UserInterface/Views/AuditNavigationSidebarPanel.js:72
> +	   const alternateToolTip = WI.UIString("Stop");

The default and alternate tooltips for toggle buttons are pretty self
explanatory; I'd remove the consts. The remaining constructor arguments are
just literals, so why not these two?

> Source/WebInspectorUI/UserInterface/Views/AuditNavigationSidebarPanel.js:129
> +

Style: drop blank line.

> Source/WebInspectorUI/UserInterface/Views/AuditNavigationSidebarPanel.js:147
> +	   this._addTest(test);

Why not just `this._addTest(event.data.test)`?

> Source/WebInspectorUI/UserInterface/Views/AuditTabContentView.js:31
> +	   const styleClassNames = ["audit"];

See similar comment above.

> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:227
> +    }

What about a single `_handleTestCaseChanged` event instead?

> Source/WebInspectorUI/UserInterface/Views/AuditTestContentView.js:195
> +

Style: drop blank line.

> Source/WebInspectorUI/UserInterface/Views/AuditTestGroupContentView.js:158
> +	   if (this._levelScopeBar) {

if (this._levelScopeBar && !levels) {...}

> Source/WebInspectorUI/UserInterface/Views/AuditTestGroupContentView.js:191
> +    get _subobjects()

Initially I liked using "private" properties like this, but now I think we
should avoid them since they're indistinguishable from private variables.

> Source/WebInspectorUI/UserInterface/Views/AuditTestGroupContentView.js:253
> +    }

All the events that just call this.needsLayout could be condensed into one
event.

> Source/WebInspectorUI/UserInterface/Views/AuditTreeElement.js:34
> +	   console.assert(isTestCase || isTestGroup || isTestCaseResult ||
isTestGroupResult);

IMO these would be nice as consts, but I know we don't use const very heavily.

> Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:497
> +	       this._placeholderScopeBarItem = new
WI.ScopeBarItem("canvas-recording-scope-bar-item-placeholder",
WI.UIString("Recordings"), {

This is already a HUGE patch. Could we remove the non-audit tab bits from this
patch? If it's because you're refactoring ScopeBarItem, I recommend doing the
refactor as a follow up.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:82
> +	       new WI.ScopeBarItem(WI.LogContentView.Scopes.Debugs,
WI.UIString("Debugs"), {className: "debugs", hidden: true}),

See above. Please break out any refactoring that has a ripple effect to other
files.