Bug (WebInspectorAuditTab) – Web Inspector: Audit: create Audit Tab

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

.

[Devin Rousso]

Created attachment 352793 [details]
Patch

[Devin Rousso]

Comment on attachment 352793 [details]
Patch

Needs more tests and better icons.

[Devin Rousso]

Created attachment 352794 [details]
[JSON] Sample AuditTestGroup

[Devin Rousso]

Created attachment 352795 [details]
[JSON] Sample AuditTestGroupResult

Captured on <https://devinrousso.com/demo/WebKit/test.html>.

[Devin Rousso]

Created attachment 352796 [details]
[Image] After Patch is applied

[EWS Watchlist]

Comment on attachment 352793 [details]
Patch

Attachment 352793 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9667221

New failing tests:
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/debugger/tail-recursion.html
inspector/model/remote-object-fake-object.html
inspector/audit/audit-report.html
inspector/audit/audit-test-case.html
inspector/debugger/tail-deleted-frames.html
inspector/audit/audit-manager.html
inspector/audit/audit-test-suite.html
inspector/debugger/paused-scopes.html
inspector/debugger/breakpoint-scope.html
inspector/model/remote-object-get-properties.html

[EWS Watchlist]

Created attachment 352805 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352793 [details]
Patch

Attachment 352793 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9667362

New failing tests:
inspector/debugger/tail-deleted-frames.html
inspector/debugger/tail-recursion.html
inspector/model/remote-object-fake-object.html
inspector/audit/audit-report.html
inspector/audit/audit-test-case.html
http/tests/inspector/paymentrequest/payment-request-internal-properties.https.html
inspector/audit/audit-manager.html
inspector/audit/audit-test-suite.html
inspector/debugger/paused-scopes.html
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/debugger/breakpoint-scope.html
inspector/model/remote-object-get-properties.html

[EWS Watchlist]

Created attachment 352808 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352793 [details]
Patch

Attachment 352793 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9667600

New failing tests:
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/model/remote-object-fake-object.html
inspector/audit/audit-report.html
inspector/audit/audit-test-case.html
inspector/debugger/tail-deleted-frames.html
inspector/audit/audit-manager.html
inspector/audit/audit-test-suite.html
inspector/model/remote-object-get-properties.html

[EWS Watchlist]

Created attachment 352813 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 352829 [details]
Patch

Fixed tests

[Radar WebKit Bug Importer]

<rdar://problem/45527619>

[Devin Rousso]

Created attachment 353117 [details]
Patch

[Matt Baker]

Comment on attachment 353117 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:381
> +        Simple sublcass of `WI.View` that is used for showing text.

sublcass -> subclass

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:25
> +localizedStrings["%d Errored"] = "%d Errored";

"Errored" is either the past tense of "to error", or an adjective meaning "containing one or more errors"; we should try to rephrase this. How about "Errors"? I think that goes well with the others: Passed | Failed | Errors.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:43
> +        const level = WI.ConsoleMessage.MessageLevel.Error;

Since all three values include the name of the corresponding constructor argument, I would omit these consts and just pass the values. It's up to you though.

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:62
> +        if (!tests || !tests.length)

Unless there's a compelling reason, I think testing for the affirmative condition reads better:

if (tests && tests.length)
    tests = tests.filter((test) => test instanceof WI.AuditTestGroup || test instanceof WI.AuditTestCase);
else
    tests = this._tests;

> Source/WebInspectorUI/UserInterface/Controllers/AuditManager.js:116
> +            if (!object) {

I'd nest this under the `if` above, since this branch can only be taken if the one above it was taken.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:100
> +

Style: drop this empty line

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:130
> +                        setLevel(WI.AuditTestCaseResult.Level.Fail);

You could reduce the nesting by replacing this if...else with a ternary:

setLevel(remoteObject.value ? WI.AuditTestCaseResult.Level.Pass : WI.AuditTestCaseResult.Level.Fail);

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:186
> +                        setLevel(WI.AuditTestCaseResult.Level.Unsupported);

Should these all be `else if (...)` after the first `if ()`?

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:194
> +                        for (let i = 0; i < length; ++i) {

Since `i` isn't used in the body of the loop this should be a for...of.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:212
> +                        for (let i = 0; i < length; ++i) {

Since `i` isn't used in the body of the loop this should be a for...of.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:228
> +                        for (let i = 0; i < length; ++i) {

Since `i` isn't used in the body of the loop this should be a for...of.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:258
> +        if (this._runningState === WI.AuditManager.RunningState.Active) {

Do we need to assert that it is active, in the same way that we assert that we are inactive in `start()`?

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:290
> +    }

How about:

toJSON()
{
    let json = {
        type: WI.AuditTestCase.TypeIdentifier,
        name: this._name,
        test: this._test
    };

    if (this._description)
        json.description = this._description;
    return json;
}

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:299
> +    Stopping: "audit-test-case-stopping",

=== Left off here ===

[Devin Rousso]

Comment on attachment 353117 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:186
>> +                        setLevel(WI.AuditTestCaseResult.Level.Unsupported);
> 
> Should these all be `else if (...)` after the first `if ()`?

Yes, but in that case I should reverse the order.  It was done this way to ensure precedence (e.g. Fail overrules Pass if both are supplied).  I should also write a test for this 🤔

>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:258
>> +        if (this._runningState === WI.AuditManager.RunningState.Active) {
> 
> Do we need to assert that it is active, in the same way that we assert that we are inactive in `start()`?

Yes, but we should assert that we are either `Active` or `Stopping`.

>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:290
>> +    }
> 
> How about:
> 
> toJSON()
> {
>     let json = {
>         type: WI.AuditTestCase.TypeIdentifier,
>         name: this._name,
>         test: this._test
>     };
> 
>     if (this._description)
>         json.description = this._description;
>     return json;
> }

I did it this way so that the output JSON is nicer to read 😅  Doing it the way you describe would mean that the result JSON would have the description after the test JavaScript, which may be harder to read.

[Matt Baker]

Comment on attachment 353117 [details]
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.

[Matt Baker]

(In reply to Devin Rousso from comment #16)
> Comment on attachment 353117 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353117&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:186
> >> +                        setLevel(WI.AuditTestCaseResult.Level.Unsupported);
> > 
> > Should these all be `else if (...)` after the first `if ()`?
> 
> Yes, but in that case I should reverse the order.  It was done this way to
> ensure precedence (e.g. Fail overrules Pass if both are supplied).  I should
> also write a test for this 🤔

That makes sense. I didn't realize they could override each other.

[Matt Baker]

Comment on attachment 353117 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:28
> +    constructor(id, label, {className, exclusive, independent, hidden} = {})

To be honest I'm not a fan of this change. Is it just for aesthetic reasons, or is there a good reason to use the options bag style?

> Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:-116
> -        this.setSelected(!this.selected, event.metaKey && !event.ctrlKey && !event.altKey && !event.shiftKey);

Nice cleanup, but it should go in a separate patch with other ScopeBarItem refactoring stuff. Did we ever use these modifiers for anything?

> Source/WebInspectorUI/UserInterface/Views/TextView.js:26
> +WI.TextView = class TextView extends WI.View

This class seems unnecessary. It doesn't make use of any view features (like layout), and overriding standard View features to throw errors is odd. I'd drop this and use h1, p, and div elements directly instead.

[Devin Rousso]

Comment on attachment 353117 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:194
>> +                        for (let i = 0; i < length; ++i) {
> 
> Since `i` isn't used in the body of the loop this should be a for...of.

This is needed because `getPropertyDescriptorsAsObject` creates an Object from the `domNodes` array, meaning that it's no longer able to use an iterator.  It's the equivalent of going from ["a", "b"] to {"1": "a", "2": "b").  I need to save the length beforehand for the same reason.

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

Yes, because we don't want to `generatePreview`, which was previously always set to `true`.

>> Source/WebInspectorUI/UserInterface/Views/AuditNavigationSidebarPanel.js:147
>> +        this._addTest(test);
> 
> Why not just `this._addTest(event.data.test)`?

I find that this makes diffs cleaner in the future if we need to modify this, but I suppose that's really just worrying over something that may never happen.

>> 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.

The purpose of this refactor is to support a feature for Audits, but I can split it up for readability.

>> Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:28
>> +    constructor(id, label, {className, exclusive, independent, hidden} = {})
> 
> To be honest I'm not a fan of this change. Is it just for aesthetic reasons, or is there a good reason to use the options bag style?

This object bag style is something I personally believe makes the code MUCH more manageable, especially for cases like these where some of the options may be used but not others (a great example of this was the refactor for `WI.NetworkManager` <https://webkit.org/b/190318>).  Having to add extra parameters as "filler" of `null` or `false` when they aren't used just makes the code more unreadable.  Additionally, using an object allows us to define the options we're using at the callsite, making it MUCH clearer as to what is going on _without_ having to go look at what's being called to understand.

>> Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:-116
>> -        this.setSelected(!this.selected, event.metaKey && !event.ctrlKey && !event.altKey && !event.shiftKey);
> 
> Nice cleanup, but it should go in a separate patch with other ScopeBarItem refactoring stuff. Did we ever use these modifiers for anything?

The modifiers were just moved to line 87.  They allow you to select multiple `WI.ScopeBarItem` while holding ⌘.

>> Source/WebInspectorUI/UserInterface/Views/TextView.js:26
>> +WI.TextView = class TextView extends WI.View
> 
> This class seems unnecessary. It doesn't make use of any view features (like layout), and overriding standard View features to throw errors is odd. I'd drop this and use h1, p, and div elements directly instead.

I find it weird when mixing plain elements and `WI.View` objects.  `WI.AuditTestGroupContentView` uses this because it has a `WI.NavigationBar`, so that it's entire DOM uses the `WI.View` system.  I do agree that it wastes work, so I'll rework it.

[Devin Rousso]

Created attachment 353404 [details]
Patch

[Matt Baker]

Comment on attachment 353404 [details]
Patch

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

r=me. This looks great!

- Nice code cleanup/design
- Dark Mode looks great
- Love the new icons

> Source/WebInspectorUI/ChangeLog:70
> +        * UserInterface/Models/AuditTestBase.js: Added.

Nice design!

> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:89
> +            this.dispatchEventToListeners(WI.AuditTestBase.Event.ResultCleared);

I think an options object is unnecessary. Isn't `suppressResultClearedEvent` the only option? If so it should be an argument.

[Matt Baker]

Created attachment 353424 [details]
[Image] Audit tab - dark mode

Looking good!

[Devin Rousso]

Comment on attachment 353404 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:89
>> +            this.dispatchEventToListeners(WI.AuditTestBase.Event.ResultCleared);
> 
> I think an options object is unnecessary. Isn't `suppressResultClearedEvent` the only option? If so it should be an argument.

I prefer to make any options into an object-bag from the start, as it means that any changes in the future don't have to update every caller.

[Blaze Burg]

Comment on attachment 353404 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:89
>>> +            this.dispatchEventToListeners(WI.AuditTestBase.Event.ResultCleared);
>> 
>> I think an options object is unnecessary. Isn't `suppressResultClearedEvent` the only option? If so it should be an argument.
> 
> I prefer to make any options into an object-bag from the start, as it means that any changes in the future don't have to update every caller.

I'm with Devin here. Please no more optional boolean parameters.

[Matt Baker]

(In reply to Brian Burg from comment #25)
> Comment on attachment 353404 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353404&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Models/AuditTestBase.js:89
> >>> +            this.dispatchEventToListeners(WI.AuditTestBase.Event.ResultCleared);
> >> 
> >> I think an options object is unnecessary. Isn't `suppressResultClearedEvent` the only option? If so it should be an argument.
> > 
> > I prefer to make any options into an object-bag from the start, as it means that any changes in the future don't have to update every caller.

Generally I don't like doing something in case we *might* add more options in the future. But it seems like the group consensus in in favor, so I'll come around. :)

> I'm with Devin here. Please no more optional boolean parameters.

[Devin Rousso]

Created attachment 353448 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 353448 [details]
Patch

Clearing flags on attachment: 353448

Committed r237613: <https://trac.webkit.org/changeset/237613>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.