Bug 196710

Summary: Web Inspector: Audit: there should be a default test for WebInspectorAudit.Resources functionality
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 190754    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Patch
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews114 for mac-highsierra
none
Archive of layout-test-results from ews211 for win-future
none
Patch
none
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Archive of layout-test-results from ews113 for mac-highsierra
none
Patch
none
Patch
none
Patch none

Description Devin Rousso 2019-04-08 14:46:58 PDT 
This will likely require that we expose another way for values to be returned, possibly even for arbitrary JSON objects (e.g. there's no non-error way of sending back a resource's contents).
Comment 1 Radar WebKit Bug Importer 2019-04-08 14:48:33 PDT 
<rdar://problem/49712348>
Comment 2 Devin Rousso 2019-05-26 22:57:12 PDT 
Created attachment 370671 [details]
Patch

My local build is currently not building (unrelated), so this is a test using the bots instead :|
Comment 3 EWS Watchlist 2019-05-26 22:59:33 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-05-26 23:46:05 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-05-26 23:46:07 PDT Comment hidden (obsolete)
Comment 6 Devin Rousso 2019-05-27 00:26:35 PDT 
Created attachment 370674 [details]
Patch

Fix typo :P
Comment 7 EWS Watchlist 2019-05-27 01:32:26 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-05-27 01:32:28 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-05-27 01:46:24 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-05-27 01:46:26 PDT Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-05-27 02:23:16 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-05-27 02:23:18 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2019-05-27 04:45:46 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2019-05-27 04:45:50 PDT Comment hidden (obsolete)
Comment 15 Devin Rousso 2019-05-27 11:39:19 PDT 
Created attachment 370699 [details]
Patch
Comment 16 EWS Watchlist 2019-05-27 12:45:06 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2019-05-27 12:45:08 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2019-05-27 12:58:16 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2019-05-27 12:58:18 PDT Comment hidden (obsolete)
Comment 20 EWS Watchlist 2019-05-27 13:27:02 PDT Comment hidden (obsolete)
Comment 21 EWS Watchlist 2019-05-27 13:27:04 PDT Comment hidden (obsolete)
Comment 22 Devin Rousso 2019-05-27 14:06:40 PDT 
Created attachment 370707 [details]
Patch
Comment 23 Devin Rousso 2019-05-29 23:20:38 PDT 
Comment on attachment 370707 [details]
Patch

There should be a provided `data-custom` default Audit that can act as a demo of both this special functionality and a good way to see the UI for custom return values.
Comment 24 Devin Rousso 2019-05-30 09:41:52 PDT 
Created attachment 370948 [details]
Patch
Comment 25 Joseph Pecoraro 2019-05-30 15:24:25 PDT 
Comment on attachment 370948 [details]
Patch

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

rs=me

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:285
> +                        } catch (e) {

Style: No need for the `(e)` if it is not used.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:287
> +                            if (key !== "__proto__")

Can we just skip __proto__ earlier, I don't think we'd want to set it.

> Source/WebInspectorUI/UserInterface/Models/AuditTestCaseResult.js:136
> +                if (key === "domNodes" || key === "domAttributes" || key === "errors") {

Might be useful to make a static function for special attributes?

    AuditTestCaseResult.isSpecialAttribute(key)

> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:36
> +        this._resultDataNonSpecialContainer = null;

Instead of "NonSpecial" maybe "General" or "CustomData" would be a better name.

> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:141
> +            let nonSpecialData = Object.filter(resultData, (key) => key !== "domNodes" && key !== "errors");

domAttributes?

> LayoutTests/inspector/unit-tests/object-utilities-expected.txt:26
> +PASS: filter should remove all entries where the key isn't in ["a","b","c"]

Nit: These normally end in a period.
Comment 26 Devin Rousso 2019-05-30 16:03:40 PDT 
Comment on attachment 370948 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/AuditTestCaseResult.js:136
>> +                if (key === "domNodes" || key === "domAttributes" || key === "errors") {
> 
> Might be useful to make a static function for special attributes?
> 
>     AuditTestCaseResult.isSpecialAttribute(key)

It's less "special", more that we require that the value is an array.

>> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:141
>> +            let nonSpecialData = Object.filter(resultData, (key) => key !== "domNodes" && key !== "errors");
> 
> domAttributes?

I'm fine showing `domAttributes` as part of `this._resultDataGeneralContainer`, as it otherwise would only be visible if `domNodes` has values in it.
Comment 27 Devin Rousso 2019-05-30 16:06:44 PDT 
Created attachment 370989 [details]
Patch
Comment 28 WebKit Commit Bot 2019-05-30 17:12:09 PDT 
Comment on attachment 370989 [details]
Patch

Clearing flags on attachment: 370989

Committed r245914: <https://trac.webkit.org/changeset/245914>
Comment 29 WebKit Commit Bot 2019-05-30 17:12:11 PDT 
All reviewed patches have been landed.  Closing bug.