Description
Devin Rousso
2018-09-13 17:17:39 PDT
Created attachment 349738 [details]
Patch
Comment on attachment 349738 [details] Patch Attachment 349738 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9211146 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/model/frame-extra-scripts.html inspector/model/script-resource-relationship.html http/tests/inspector/network/resource-initiatorNode.html inspector/model/stack-trace.html inspector/canvas/requestNode.html inspector/canvas/create-context-webgl.html Created attachment 349745 [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
Comment on attachment 349738 [details] Patch Attachment 349738 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9211230 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/model/frame-extra-scripts.html inspector/model/script-resource-relationship.html http/tests/inspector/network/resource-initiatorNode.html inspector/model/stack-trace.html inspector/canvas/requestNode.html inspector/canvas/create-context-webgl.html Created attachment 349747 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 349738 [details] Patch Attachment 349738 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9211334 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/model/frame-extra-scripts.html inspector/model/script-resource-relationship.html http/tests/inspector/network/resource-initiatorNode.html http/tests/media/clearkey/collect-webkit-media-session.html inspector/model/stack-trace.html inspector/canvas/requestNode.html inspector/canvas/create-context-webgl.html Created attachment 349749 [details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 349771 [details]
[Image] After Patch is applied
Comment on attachment 349738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349738&action=review > Source/WebCore/platform/network/ResourceRequestBase.h:239 > + RefPtr<Node> m_initiatorNode; As discussed in person, having ResourceRequest reference a WebCore::Node is a layering violation. The platform layer shouldn't know about WebCore types. Created attachment 349858 [details]
Patch
Some of these `InspectorInstrumentation` calls are probably unnecessary, so I'll try removing them one by one later. This is mainly to see if the tests pass on EWS, although it is still reviewable.
Comment on attachment 349858 [details] Patch Attachment 349858 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9228999 New failing tests: inspector/model/stack-trace.html inspector/model/frame-extra-scripts.html inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html inspector/model/script-resource-relationship.html Created attachment 349861 [details]
Archive of layout-test-results from ews100 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 349858 [details] Patch Attachment 349858 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9229097 New failing tests: inspector/model/stack-trace.html inspector/model/frame-extra-scripts.html inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html inspector/model/script-resource-relationship.html Created attachment 349862 [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
Comment on attachment 349858 [details] Patch Attachment 349858 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9229112 New failing tests: inspector/model/stack-trace.html inspector/model/frame-extra-scripts.html inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html inspector/model/script-resource-relationship.html Created attachment 349863 [details]
Archive of layout-test-results from ews116 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 349913 [details]
Patch
Comment on attachment 349913 [details] Patch Attachment 349913 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9245738 New failing tests: inspector/model/stack-trace.html inspector/model/frame-extra-scripts.html inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html inspector/model/script-resource-relationship.html Created attachment 349918 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 349913 [details] Patch Attachment 349913 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9245768 New failing tests: inspector/model/stack-trace.html inspector/model/frame-extra-scripts.html inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html inspector/model/script-resource-relationship.html Created attachment 349920 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 349913 [details] Patch Attachment 349913 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9245806 New failing tests: inspector/model/stack-trace.html inspector/model/frame-extra-scripts.html inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html inspector/model/script-resource-relationship.html Created attachment 349930 [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
Created attachment 349967 [details]
Patch
Fixed a typo that was causing the unexpected test failures.
Comment on attachment 349967 [details] Patch Attachment 349967 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9250145 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html Created attachment 349980 [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
Comment on attachment 349967 [details] Patch Attachment 349967 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9250440 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html Created attachment 349987 [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
Comment on attachment 349967 [details] Patch Attachment 349967 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9250615 New failing tests: inspector/canvas/create-context-bitmaprenderer.html inspector/canvas/create-context-webgl.html Created attachment 349988 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 350230 [details]
Patch
The canvas test failures were caused by `InspectorCanvas::buildObjectForCanvas` eagerly binding the canvas' node in `InspectorDOMAgent`, which will not be removed if the canvas' node is detached from the DOM since we unbind nodes in `InspectorDOMAgent` when they are removed from their parent.
Created attachment 350239 [details]
Patch
Created attachment 350463 [details]
Patch
localizedStrings :|
Comment on attachment 350463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350463&action=review Here is a first pass, I didn't review the NetworkTable changes closely, but I skimmed the rest of the patch to try and get a high level idea. > LayoutTests/http/tests/inspector/network/har/har-basic.html:-39 > - const loaderIdentifier = undefined; > - const targetId = undefined; > - const requestIdentifier = undefined; why not keep these, they would still be undefined below, right? > Source/WebCore/ChangeLog:21 > + Handles initial (e.g. DNT) resource requests. What is DNT here? Do Not Track? > Source/WebCore/ChangeLog:29 > + (MediaResourceLoader::requestResource): > + Handles byte-range requests. I wonder why the other locations don't set the requester like here: request.setRequester(ResourceRequest::Requester::Media) > Source/WebCore/ChangeLog:71 > + Don't try to push the canvas' node to the frontend, as this will create a dangling node in > + `InspectorDOMAgent` if the canvas' node is detached from the DOM. I think you can mention you intend to add this back in the future? I think you had mentioned this might be the case after updating DOM.NodeId to uniquely identify nodes through the entire node's lifetime. > Source/WebCore/platform/network/ResourceRequestBase.h:239 > + int m_initiatorNodeIdentifier; All of these others have an initial value, you should set that here as well: int m_initatorNodeIdentifier { 0 }; > Source/WebInspectorUI/ChangeLog:85 > + * UserInterface/Images/Range.svg: Added. I think `ByteRange.svg` might be a better name than `Range.svg`. I had always thought a [#] might be better than [R]. Another idea I've had for other byte-wise data was something like: _______ | 0 1 | | 1 0 | ⎺⎺⎺⎺⎺⎺⎺ JonDavis / mattbaker / xenon may know what it takes to create a new such icon path from a previously unused character. I have never created a new one myself! > Source/WebInspectorUI/UserInterface/Images/Range.svg:2 > +<!-- Copyright © 2013 Apple Inc. All rights reserved. --> 2013 or 2018? > Source/WebInspectorUI/UserInterface/Models/Resource.js:78 > + this._target = targetIdentifier ? WI.targetManager.targetForIdentifier(targetIdentifier) : WI.mainTarget; We use the identifier `targetId` everywhere in the frontend, and never `targetIdentifier`. I'm not sure we'd want to change that here. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:101 > +.network-table li.selected .cell.domain > .lock { > + filter: invert(); > +} I'm not sure this is needed, we shouldn't ever be showing the whole row as selected in the Network Tab. (Maybe if we bring this Network Table to the Timeline Tab it would be needed). Comment on attachment 350463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350463&action=review >> LayoutTests/http/tests/inspector/network/har/har-basic.html:-39 >> - const requestIdentifier = undefined; > > why not keep these, they would still be undefined below, right? Since `WI.Resource` now uses an object-bag, these values are unnecessary as they will effectively get set to undefined when the object-bag gets destructured. Additionally, this makes the test a bit easier to read because now it's clear what's been specified and what's left "blank". >> Source/WebCore/ChangeLog:21 >> + Handles initial (e.g. DNT) resource requests. > > What is DNT here? Do Not Track? Yup! >> Source/WebCore/ChangeLog:71 >> + `InspectorDOMAgent` if the canvas' node is detached from the DOM. > > I think you can mention you intend to add this back in the future? I think you had mentioned this might be the case after updating DOM.NodeId to uniquely identify nodes through the entire node's lifetime. Yeah, I'll add a comment linking to one of these: - <https://webkit.org/b/178282> - <https://webkit.org/b/189687> - <https://webkit.org/b/189776> >> Source/WebInspectorUI/UserInterface/Images/Range.svg:2 >> +<!-- Copyright © 2013 Apple Inc. All rights reserved. --> > > 2013 or 2018? I would think 2013 since this file is a direct copy of one of the other [R] icons, but I'm not the best person to ask about copyright 😅 >> Source/WebInspectorUI/UserInterface/Models/Resource.js:78 >> + this._target = targetIdentifier ? WI.targetManager.targetForIdentifier(targetIdentifier) : WI.mainTarget; > > We use the identifier `targetId` everywhere in the frontend, and never `targetIdentifier`. I'm not sure we'd want to change that here. I think my main motivation was to match `loaderIdentifier` and `requestIdentifier`, but it's unnecessary. I'll revert. >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:101 >> +} > > I'm not sure this is needed, we shouldn't ever be showing the whole row as selected in the Network Tab. (Maybe if we bring this Network Table to the Timeline Tab it would be needed). When looking at this patch alone, it is, because it's allowed for the user to select a node's row. The followup <https://webkit.org/b/189773> removes this functionality by replacing the rest of the columns with the timeline. Created attachment 351480 [details]
Patch
Comment on attachment 351480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review > Source/WebCore/ChangeLog:11 > + Add extra arguments to functions that create `ResourceRequest` objets for media resources so Nit: objects > Source/WebCore/html/track/LoadableTextTrack.cpp:-85 > - if (!m_loader->load(m_url, m_trackElement->mediaElementCrossOriginAttribute(), m_trackElement->isInUserAgentShadowTree())) Why did this change? > Source/WebCore/inspector/InspectorCanvas.cpp:-253 > - else { I don't quite understand why this change belongs in this patch. > Source/WebCore/loader/TextTrackLoader.cpp:147 > +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element) ... Oh, i see. Jer, is this a layering violation for this part of the code? > Source/WebCore/platform/network/ResourceRequestBase.h:176 > + int initiatorNodeIdentifier() const { return m_initiatorNodeIdentifier; } Please use Identified.h templates for this. It should be std::optional too. Comment on attachment 351480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review >> Source/WebCore/inspector/InspectorCanvas.cpp:-253 >> - else { > > I don't quite understand why this change belongs in this patch. It's more-so that this was never used by the frontend. Since this patch adds calls to `WI.domManager.ensureDocument()` whenever the document changes, it would effect this code. The change to "inspector/canvas/requestNode.html" is for the same reason. >> Source/WebCore/loader/TextTrackLoader.cpp:147 >> +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element) > > ... Oh, i see. Jer, is this a layering violation for this part of the code? I might need some clarity as to what we consider a layering violation. `TextTrackLoader` holds a reference to a `TextTrackLoaderClient`, which I think is always a `LoadableTextTrack`, which has access to its `HTMLTrackElement`. Not sure if that is a similar concept to our "delegate" in WebInspector, but if so I can probably reach the `HTMLTrackElement` via the client. >> Source/WebCore/platform/network/ResourceRequestBase.h:176 >> + int initiatorNodeIdentifier() const { return m_initiatorNodeIdentifier; } > > Please use Identified.h templates for this. It should be std::optional too. I've never used `Identified` before. Based on what I can understand from the code, I'm not sure that it's what we want here. The reason this member exists is so that we can relate a specific request to the "node" that effectively triggered that request. In this patch, this value is set to `InspectorDOMAgentt`s id for the node, which can then be properly propagated until it reaches `InspectorNetworkAgent`. This value isn't supposed to be an identifier for the request itself, but instead used as a way for the inspector to relate this request to something else it has already identified. Making it an `optional` seem's like a good idea though. (In reply to Brian Burg from comment #38) > Comment on attachment 351480 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351480&action=review > > > Source/WebCore/loader/TextTrackLoader.cpp:147 > > +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element) > > ... Oh, i see. Jer, is this a layering violation for this part of the code? I don't think so; there's already a bunch of code in WebCore/loader than references HTMLMediaElements (as well as other HTML element types). Comment on attachment 351480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review >>> Source/WebCore/loader/TextTrackLoader.cpp:147 >>> +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element) >> >> ... Oh, i see. Jer, is this a layering violation for this part of the code? > > I might need some clarity as to what we consider a layering violation. > > `TextTrackLoader` holds a reference to a `TextTrackLoaderClient`, which I think is always a `LoadableTextTrack`, which has access to its `HTMLTrackElement`. Not sure if that is a similar concept to our "delegate" in WebInspector, but if so I can probably reach the `HTMLTrackElement` via the client. Right. I'm not sure if it's okay in this code for a Loader to hold on to an Element subclass instance, or if they are supposed to be abstracted away with a *Client interface. >>> Source/WebCore/platform/network/ResourceRequestBase.h:176 >>> + int initiatorNodeIdentifier() const { return m_initiatorNodeIdentifier; } >> >> Please use Identified.h templates for this. It should be std::optional too. > > I've never used `Identified` before. Based on what I can understand from the code, I'm not sure that it's what we want here. The reason this member exists is so that we can relate a specific request to the "node" that effectively triggered that request. In this patch, this value is set to `InspectorDOMAgentt`s id for the node, which can then be properly propagated until it reaches `InspectorNetworkAgent`. This value isn't supposed to be an identifier for the request itself, but instead used as a way for the inspector to relate this request to something else it has already identified. > > Making it an `optional` seem's like a good idea though. I read into this more, and I think Identified would be a great replacement for `int`, but it would require significant code changes in InspectorDOMAgent. Perhaps let's defer this change til when you rework that agent? In the meantime, I would guess that other engineers prefer at least a more informative typedef for the ResourceRequest member, like Inspector::NodeIdentifier. > Source/WebInspectorUI/ChangeLog:10 > + node that initiated the load (if appliable). When grouped by node, a tree-like layout of the Nit: if applicable > Source/WebInspectorUI/ChangeLog:20 > + Whenever the frame navigates, re-reqeust the document so that `NetworkAgent` is able to send Nit: re-request > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:157 > + mainResource = new WI.Resource(framePayload.url, { I like the new style, but please make a separate patch for it. It's kind of messy and this patch is already large. > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:573 > + _addNewResourceToFrameOrTarget(requestIdentifier, frameIdentifier, loaderIdentifier, url, type, requestMethod, requestHeaders, requestData, elapsedTime, walltime, frameName, frameSecurityOrigin, initiatorSourceCodeLocation, initiatorNode, originalRequestWillBeSentTimestamp, targetId) Thought: it's regrettable that this function signature is so coupled to WI.Resource. Maybe the function should just take a WI.Resource and register it, someday. > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:-570 > - resource = new WI.Resource(url, null, type, loaderIdentifier, targetId, requestIdentifier, requestMethod, requestHeaders, requestData, elapsedTime, walltime, initiatorSourceCodeLocation, originalRequestWillBeSentTimestamp); Thought: similarly, there is a bunch of weird knowledge here. I wonder if these code paths are only triggered by one or a few callers. Comment on attachment 351480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review This is almost ready to land. Here's what's left to do: - Fix localization issue for byte range - Move WI.Resource constructor refactor to another patch - Address transferSize vs resourceSize computation for initiator node entries >>>>> Source/WebCore/loader/TextTrackLoader.cpp:147 >>>>> +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element) >>>> >>>> ... Oh, i see. Jer, is this a layering violation for this part of the code? >>> >>> I might need some clarity as to what we consider a layering violation. >>> >>> `TextTrackLoader` holds a reference to a `TextTrackLoaderClient`, which I think is always a `LoadableTextTrack`, which has access to its `HTMLTrackElement`. Not sure if that is a similar concept to our "delegate" in WebInspector, but if so I can probably reach the `HTMLTrackElement` via the client. >> >> I don't think so; there's already a bunch of code in WebCore/loader than references HTMLMediaElements (as well as other HTML element types). > > Right. I'm not sure if it's okay in this code for a Loader to hold on to an Element subclass instance, or if they are supposed to be abstracted away with a *Client interface. Ok, cool. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:389 > + let setTextContent = (accessor) => { Very cool. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:390 > + let uniqueValues = this._uniqueValuesForDOMNodeEntry(entry, accessor); Thought: It would be a little more definitive to do: let entryRepresentsAnInitiator = !entry.resource // or some other condition if (entryRepresentsAnInitiator) { let uniqueValues = this._uniqueValuesForInitiatorEntry(entry, accessor); if (uniqueValues.size() > 1) ... We may want to expose non-node initiators (i.e., script) in the future and display them the same way. This would codify how initiators ought to be displayed when Group By Node (sic) is enabled. EDIT: looking at the screenshot, it seems I'm confusing initiatorNode with the script location that's labeled 'Initiator'. Let's keep this as-is; in the future it would be worth exploring this idea (initiator tree in network table). It would be tricky to represent multi-level initiator chains without running out of room. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:443 > + var resourceSize = entry.resourceSize; Any reason to not use `let` and introduce a block scope? > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:446 > + resourceSize = resourceEntries.reduce((accumulator, current) => accumulator + (current.transferSize || 0), 0); Why does this use transferSize instead of resourceSize? This seems wrong. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:449 > case "transferSize": Does the transferSize case work correctly when some of the things are (memory) cached? EDIT: I see, you added support in _populateTransferSizeCell. Nice. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:519 > + nameElement.textContent = WI.UIString("Byte Range %s").format(resource.requestHeaders["Range"].replace("bytes=", "")); I have a feeling this is not going to localize well. - This is a raw header, the value might be garbage. - In the English localization, it should use an en-dash to represent a range of numbers. - In other localizations, the range may be represented with different punctuation. I'd recommend to extract the two numbers separately then format them into a localized string like "Byte Range %d–%d". Maybe the resource itself could expose the byte range as a getter that returns null or an object {start: end:}. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:-669 > - comparator = comparator = (a, b) => a.startTime - b.startTime; Haha > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:823 > + this._entriesSortComparator = (a, b) => reverseFactor * comparator(substitute(a, b), substitute(b, a)); This really needs a comment and/or a better name than 'substitute'. I'm caffeinated and still going crosseyed trying to understand it. I think it's replacing `entry` with the first resource that shares the same initiator node if it has one and `other` does not have the same initiator node. But the net effect isn't obvious. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1033 > + Nit: extra lines > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1107 > + function updateEntry(existing) { Capturing 'entry' here isn't making this any clearer. Maybe updateExistingFromEntry(existing, entry). > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1407 > + _entryForDOMNode(domNode, requiredFields) What's requiredFields for? I see we pass Object.keys(resourceEntry) but nothing happens with it. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1409 > + return { I got a little confused that DOM node "entries" are completely different from resource entries, which are part of the Table API. Comment on attachment 351480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:446 >> + resourceSize = resourceEntries.reduce((accumulator, current) => accumulator + (current.transferSize || 0), 0); > > Why does this use transferSize instead of resourceSize? This seems wrong. I think this is a copypasta mistake 😅 >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1107 >> + function updateEntry(existing) { > > Capturing 'entry' here isn't making this any clearer. Maybe updateExistingFromEntry(existing, entry). If I do that, then we have a shadowed variable `entry`. I'll think of a better name. >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1407 >> + _entryForDOMNode(domNode, requiredFields) > > What's requiredFields for? I see we pass Object.keys(resourceEntry) but nothing happens with it. Leftover from a previous version. >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1409 >> + return { > > I got a little confused that DOM node "entries" are completely different from resource entries, which are part of the Table API. I don't think the Table API says anything about "entries", or really even knows about it. It just asks for a number of rows and then tells us which `rowIndex` it's populating when it tries to render. This follows that "pattern" in the sense that this is an "entry", just not for a resource. Created attachment 351728 [details]
Patch
Comment on attachment 351728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351728&action=review r=me, great work! > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:818 > + // If the entry has an `initiatorNode`, use that node's "first" resource as the value of Thanks. > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:821 > + // next to eachother, as they will all effectively be sorted by the first resource. Nit: each other Created attachment 351785 [details]
Patch
Comment on attachment 351785 [details] Patch Clearing flags on attachment: 351785 Committed r236927: <https://trac.webkit.org/changeset/236927> All reviewed patches have been landed. Closing bug. Looks like the new test http/tests/inspector/network/resource-initiatorNode.html added in https://trac.webkit.org/changeset/236927/webkit is a flakey timeout. History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Finspector%2Fnetwork%2Fresource-initiatorNode.html It is highly intermittent and only showed up recently. |