Bug 190318

Summary: Web Inspector: refactor constructor of WI.Resource
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, inspector-bugzilla-changes, joepeck, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 189606    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews105 for mac-sierra-wk2
none
Archive of layout-test-results from ews100 for mac-sierra
none
Patch
none
Patch none

Devin Rousso
Reported 2018-10-05 11:24:16 PDT
It's a pretty huge mess right now, especially if we need to add more parameters.
Attachments
Patch (27.38 KB, patch)
2018-10-05 11:30 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews105 for mac-sierra-wk2 (3.07 MB, application/zip)
2018-10-05 12:22 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews100 for mac-sierra (2.45 MB, application/zip)
2018-10-05 12:38 PDT, EWS Watchlist 		no flags
Details
Patch (27.36 KB, patch)
2018-10-05 13:24 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (27.46 KB, patch)
2018-10-05 14:59 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2018-10-05 11:30:21 PDT
Created attachment 351685 [details] Patch
EWS Watchlist
Comment 2 2018-10-05 12:22:29 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 3 2018-10-05 12:22:31 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 4 2018-10-05 12:38:19 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 5 2018-10-05 12:38:21 PDT Comment hidden (obsolete)
Devin Rousso
Comment 6 2018-10-05 13:24:23 PDT
Created attachment 351694 [details] Patch
Joseph Pecoraro
Comment 7 2018-10-05 14:13:50 PDT
Comment on attachment 351694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351694&action=review r=me if all tests are green. > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:139 > + let frameResource = this._addNewResourceToFrameOrTarget(framePayload.url, framePayload.id, { > + loaderIdentifier: framePayload.loaderId, > + }, { > + name: framePayload.name, > + securityOrigin: framePayload.securityOrigin, > + }); Can we style this like: let resourceOptions = { ... }; let frameOptions = { ... }; let frameResource = this._addNewResourceToFrameOrTarget(...); > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:-28 > - constructor(url, loaderIdentifier, targetId, requestIdentifier, requestHeaders, requestData, timestamp, walltime, requestSentTimestamp, initiatorSourceCodeLocation) It seems concerning that this takes a subset of Resource initialization options and so might need to be manually added back if used later.
Devin Rousso
Comment 8 2018-10-05 14:51:58 PDT
Comment on attachment 351694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351694&action=review >> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:-28 >> - constructor(url, loaderIdentifier, targetId, requestIdentifier, requestHeaders, requestData, timestamp, walltime, requestSentTimestamp, initiatorSourceCodeLocation) > > It seems concerning that this takes a subset of Resource initialization options and so might need to be manually added back if used later. One of the nice things about using an object-bag is that it makes it MUCH easier/cleaner to re-add these if needed. Updating both the constructor and any call-sites is much simpler to keep track of. :)
Devin Rousso
Comment 9 2018-10-05 14:59:00 PDT
Created attachment 351700 [details] Patch
WebKit Commit Bot
Comment 10 2018-10-05 17:21:01 PDT
Comment on attachment 351700 [details] Patch Clearing flags on attachment: 351700 Committed r236891: <https://trac.webkit.org/changeset/236891>
WebKit Commit Bot
Comment 11 2018-10-05 17:21:02 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2018-10-05 17:22:32 PDT
<rdar://problem/45060404>
Note You need to log in before you can comment on or make changes to this bug.