Bug 190754 (WebInspectorAuditTab)

Summary: Web Inspector: Audit: create Audit Tab
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: aaron_chu, bburg, commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, mattbaker, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 190986, 190987, 190988, 190989    
Bug Blocks: 191045, 192102, 190853, 190857, 190858, 191044, 191566, 191659, 191727, 191758, 191843, 192105, 192106, 192107, 192171, 192209, 192210, 192211, 192317, 192346, 192764, 192769, 192868, 192870, 192918, 193149, 193158, 193225, 193226, 193227, 193228, 193262, 193476, 193594, 193675, 193686, 193743, 193861, 194454, 195265, 195266, 195292, 196710, 197470, 198270, 198719    
Attachments:
Description Flags
Patch
none
[JSON] Sample AuditTestGroup
none
[JSON] Sample AuditTestGroupResult
none
[Image] After Patch is applied
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Archive of layout-test-results from ews117 for mac-sierra
none
Patch
none
Patch
none
Patch
none
[Image] Audit tab - dark mode
none
Patch none

Description Devin Rousso 2018-10-19 09:29:15 PDT
.
Comment 1 Devin Rousso 2018-10-19 10:00:56 PDT
Created attachment 352793 [details]
Patch
Comment 2 Devin Rousso 2018-10-19 10:01:15 PDT
Comment on attachment 352793 [details]
Patch

Needs more tests and better icons.
Comment 3 Devin Rousso 2018-10-19 10:02:31 PDT
Created attachment 352794 [details]
[JSON] Sample AuditTestGroup
Comment 4 Devin Rousso 2018-10-19 10:04:04 PDT
Created attachment 352795 [details]
[JSON] Sample AuditTestGroupResult

Captured on <https://devinrousso.com/demo/WebKit/test.html>.
Comment 5 Devin Rousso 2018-10-19 10:06:11 PDT
Created attachment 352796 [details]
[Image] After Patch is applied
Comment 6 EWS Watchlist 2018-10-19 11:13:56 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-10-19 11:13:57 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-10-19 11:32:59 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-10-19 11:33:01 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-10-19 12:09:44 PDT Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-10-19 12:09:45 PDT Comment hidden (obsolete)
Comment 12 Devin Rousso 2018-10-19 15:34:37 PDT
Created attachment 352829 [details]
Patch

Fixed tests
Comment 13 Radar WebKit Bug Importer 2018-10-24 11:41:53 PDT
<rdar://problem/45527619>
Comment 14 Devin Rousso 2018-10-25 15:39:16 PDT
Created attachment 353117 [details]
Patch
Comment 15 Matt Baker 2018-10-26 12:56:48 PDT
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 ===
Comment 16 Devin Rousso 2018-10-26 13:42:29 PDT
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.
Comment 17 Matt Baker 2018-10-26 15:17:11 PDT
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.
Comment 18 Matt Baker 2018-10-26 15:17:57 PDT
(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.
Comment 19 Matt Baker 2018-10-26 16:15:04 PDT
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.
Comment 20 Devin Rousso 2018-10-26 16:42:44 PDT
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.
Comment 21 Devin Rousso 2018-10-30 13:24:02 PDT
Created attachment 353404 [details]
Patch
Comment 22 Matt Baker 2018-10-30 16:00:07 PDT
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.
Comment 23 Matt Baker 2018-10-30 16:03:08 PDT
Created attachment 353424 [details]
[Image] Audit tab - dark mode

Looking good!
Comment 24 Devin Rousso 2018-10-30 16:08:27 PDT
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.
Comment 25 BJ Burg 2018-10-30 16:11:20 PDT
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.
Comment 26 Matt Baker 2018-10-30 16:17:25 PDT
(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.
Comment 27 Devin Rousso 2018-10-30 17:41:51 PDT
Created attachment 353448 [details]
Patch
Comment 28 WebKit Commit Bot 2018-10-30 18:11:45 PDT
Comment on attachment 353448 [details]
Patch

Clearing flags on attachment: 353448

Committed r237613: <https://trac.webkit.org/changeset/237613>
Comment 29 WebKit Commit Bot 2018-10-30 18:11:47 PDT
All reviewed patches have been landed.  Closing bug.