Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2523633003:
[Devtools] Prepare network log view for grouping support (Closed)
|
Created:
4 years ago by allada Modified:
3 years, 11 months ago Reviewers:
dgozman CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Prepare network log view for grouping support
This patch prepares the network log view and waterfall for grouping support.
R=dgozman
BUG=666971
Patch Set 1 : Merge branch 'master' into NETWORK_GROUP_SUPPORT_1 #
Total comments: 28
Patch Set 2 : changes #
Total comments: 12
Patch Set 3 : changes #
Total comments: 8
Patch Set 4 : changes #
Total comments: 2
Patch Set 5 : Merge branch 'REMOVE_LOG_ENTRIES' into NETWORK_GROUP_SUPPORT_1 #Patch Set 6 : fixed request === null and possible filter out issue #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 37 (28 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
PTL
Interesting patch! I believe we need some test coverage now. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:274: firstRequest() { Why firstRequest, and not a request? https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:852: dataGrid.insertChild(node); This should go outside of the for loop. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:933: this._nodesByRequestId.set(originalRequest.requestId, originalRequestNode); I believe this is incorrect now - only a single requestid should be reassigned. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1181: for (var node of this._nodesByRequestId.values()) Let's have a helper method which does this. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1264: for (node of this.flatNodesList()) { var node https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1301: var isMatchingSearchQuerySymbol = Network.NetworkLogView._isMatchingSearchQuerySymbol; Inline it back :-) https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1302: for (var node of this.flatNodesList()) { Is this really an equivalent of what was here before? I think one is "visible" while the other is "all". https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1304: // TODO(allada) This should properly support groupped requests. definitely! https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1365: for (var request of node.requests()) { This is incorrect: if one request matches, we should leave the whole node. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:141: var request = /** @type {!SDK.NetworkRequest} */ (logEntry.firstRequest()); Why do you cast to non-nullable here and check for null everywhere else? https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:364: // TODO(allada) Move this into NetworkLogEntry. Not sure this is a good idea. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:385: // TODO(allada) Move this into NetworkLogEntry. Ditto. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:455: if (!logEntry.isGroup() && logEntry === this._hoveredLogEntry) { Why checking for group? https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js (right): https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:1105: while (request && !this._initiatorChainCache.has(request)) { Nice one!
Patchset #2 (id:60001) has been deleted
PTL https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:274: firstRequest() { On 2016/11/23 01:57:30, dgozman wrote: > Why firstRequest, and not a request? Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:852: dataGrid.insertChild(node); On 2016/11/23 01:57:30, dgozman wrote: > This should go outside of the for loop. Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:933: this._nodesByRequestId.set(originalRequest.requestId, originalRequestNode); On 2016/11/23 01:57:30, dgozman wrote: > I believe this is incorrect now - only a single requestid should be reassigned. Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1181: for (var node of this._nodesByRequestId.values()) On 2016/11/23 01:57:30, dgozman wrote: > Let's have a helper method which does this. Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1264: for (node of this.flatNodesList()) { On 2016/11/23 01:57:30, dgozman wrote: > var node Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1301: var isMatchingSearchQuerySymbol = Network.NetworkLogView._isMatchingSearchQuerySymbol; On 2016/11/23 01:57:30, dgozman wrote: > Inline it back :-) Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1302: for (var node of this.flatNodesList()) { On 2016/11/23 01:57:30, dgozman wrote: > Is this really an equivalent of what was here before? I think one is "visible" > while the other is "all". Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1304: // TODO(allada) This should properly support groupped requests. On 2016/11/23 01:57:30, dgozman wrote: > definitely! Acknowledged. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1365: for (var request of node.requests()) { On 2016/11/23 01:57:30, dgozman wrote: > This is incorrect: if one request matches, we should leave the whole node. Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:141: var request = /** @type {!SDK.NetworkRequest} */ (logEntry.firstRequest()); On 2016/11/23 01:57:30, dgozman wrote: > Why do you cast to non-nullable here and check for null everywhere else? Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:364: // TODO(allada) Move this into NetworkLogEntry. On 2016/11/23 01:57:30, dgozman wrote: > Not sure this is a good idea. Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:385: // TODO(allada) Move this into NetworkLogEntry. On 2016/11/23 01:57:30, dgozman wrote: > Ditto. Done. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:455: if (!logEntry.isGroup() && logEntry === this._hoveredLogEntry) { On 2016/11/23 01:57:30, dgozman wrote: > Why checking for group? Because this will cause all child grouped items to gain numbers and it'd clutter everything. https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js (right): https://codereview.chromium.org/2523633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/NetworkRequest.js:1105: while (request && !this._initiatorChainCache.has(request)) { On 2016/11/23 01:57:30, dgozman wrote: > Nice one! Acknowledged.
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:276: for (var node of this.flatenChildren()) { This looks so inefficient. https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:646: get hasChildren() {} No getters please... https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:343: flatenChildren() { These are definitely not children of log view. flatEntries? https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1131: requests = requests.concat(node.requests()); Should this be node.request() instead? https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1232: var requests = logEntry.request() ? [logEntry.request()] : []; I don't get this line. https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js (right): https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js:298: flatenChildren() { Let's land this in a separate patch with a test.
Patchset #2 (id:80001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
PTL https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:276: for (var node of this.flatenChildren()) { On 2016/11/28 18:39:51, dgozman wrote: > This looks so inefficient. This call is cached so it should not be bad. https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:646: get hasChildren() {} On 2016/11/28 18:39:51, dgozman wrote: > No getters please... Done. https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:343: flatenChildren() { On 2016/11/28 18:39:51, dgozman wrote: > These are definitely not children of log view. flatEntries? Done. https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1131: requests = requests.concat(node.requests()); On 2016/11/28 18:39:51, dgozman wrote: > Should this be node.request() instead? Done. https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1232: var requests = logEntry.request() ? [logEntry.request()] : []; On 2016/11/28 18:39:51, dgozman wrote: > I don't get this line. Done. https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js (right): https://codereview.chromium.org/2523633003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js:298: flatenChildren() { On 2016/11/28 18:39:51, dgozman wrote: > Let's land this in a separate patch with a test. Done.
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #7 (id:280001) has been deleted
Patchset #6 (id:260001) has been deleted
Patchset #5 (id:240001) has been deleted
Patchset #4 (id:220001) has been deleted
Patchset #3 (id:200001) has been deleted
PTAL
https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:846: for (var request of node.requests()) { Isn't this n^2, since we return all requests in a subtree? Should we make requests() only return the immediate children instead? https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1130: requests = requests.concat(node.requests()); Ditto. https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1235: requests.concat(node.requests()); This is n^2 as well. https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:83: if (!this._hoveredNode || !request) Just !request
PTaL https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:846: for (var request of node.requests()) { On 2016/11/30 00:25:26, dgozman wrote: > Isn't this n^2, since we return all requests in a subtree? > Should we make requests() only return the immediate children instead? Done. https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1130: requests = requests.concat(node.requests()); On 2016/11/30 00:25:26, dgozman wrote: > Ditto. Done. https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1235: requests.concat(node.requests()); On 2016/11/30 00:25:26, dgozman wrote: > This is n^2 as well. Done. https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2523633003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:83: if (!this._hoveredNode || !request) On 2016/11/30 00:25:26, dgozman wrote: > Just !request Done.
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Can we split out the removal of NetworkLogEntry from the changes in NetworkLogView? I feel like this patch tries to handle all the aspects which leads to errors over and over again. I propose the following plan: - remove log entry; - introduce experiment which will group requests by 10 (or something); - support grouping in various functionality aspects one by one. https://codereview.chromium.org/2523633003/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/2523633003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:856: while (matched && parent && parent[Network.NetworkLogView._isMatchingSearchQuerySymbol] === false) { "=== false" may be error-prone if that symbol is not set. https://codereview.chromium.org/2523633003/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:1133: return this.flattenAllNodes().map(node => node.request()); There could be nulls in this array which you should filter out.
Description was changed from ========== [Devtools] Prepare network log view for grouping support This patch prepares the network log view and waterfall for grouping support. R=dgozman BUG=666971 ========== to ========== [Devtools] Prepare network log view for grouping support This patch prepares the network log view and waterfall for grouping support. R=dgozman BUG=666971 ========== |
