Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1842323002:
[DevTools] Fix stale requests in Network panel not being refreshed (Closed)
|
Created:
4 years, 8 months ago by Matt Zeunert Modified:
4 years, 8 months ago Reviewers:
paulirish, pfeldman, pfeldman 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, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Fix stale requests in Network panel not being refreshed
Fix bug where changes to network requests wouldn't be reflected in the
Network panel. For example, a finished request would show as "pending"
until a refresh was forced by filtering the request out and then
resetting the filter.
BUG=599404
Committed: https://crrev.com/35e85822440aa0729ff35b81dda5fcd61878f242
Cr-Commit-Position: refs/heads/master@{#384685}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Tweak code based on code review comments. #
Total comments: 2
Patch Set 3 : Tweak style. #Patch Set 4 : Add name to AUTHORS file. #Messages
Total messages: 24 (9 generated)
Description was changed from ========== [DevTools] Fix stale requests in Network panel not being refreshed Fix bug where changes to network requests wouldn't be reflected in the Network panel. For example, a finished request would show as "pending" until a refresh was forced by filtering the request out and then resetting the filter. BUG=599404 ========== to ========== [DevTools] Fix stale requests in Network panel not being refreshed Fix bug where changes to network requests wouldn't be reflected in the Network panel. For example, a finished request would show as "pending" until a refresh was forced by filtering the request out and then resetting the filter. BUG=599404 ==========
matt@mostlystatic.com changed reviewers: + paulirish@chromium.com, pfeldman@chromium.com
PTAL
On 2016/03/31 at 10:26:59, matt wrote:
> PTAL
No need for a for loop. You have an array so `nodesToRefresh.forEach(item => {
item.refresh(); })` would clean this up quite a bit.
On 2016/03/31 13:03:24, Jonathan Garbee wrote:
> On 2016/03/31 at 10:26:59, matt wrote:
> > PTAL
>
> No need for a for loop. You have an array so `nodesToRefresh.forEach(item => {
> item.refresh(); })` would clean this up quite a bit.
Happy to change that if necessary. I tried to be consistent with the code around
it, so if I use forEach it would make sense to replace the other for loops in
that function as well.
On 2016/04/01 at 07:54:12, matt wrote:
> On 2016/03/31 13:03:24, Jonathan Garbee wrote:
> > On 2016/03/31 at 10:26:59, matt wrote:
> > > PTAL
> >
> > No need for a for loop. You have an array so `nodesToRefresh.forEach(item =>
{
> > item.refresh(); })` would clean this up quite a bit.
>
> Happy to change that if necessary. I tried to be consistent with the code
around it, so if I use forEach it would make sense to replace the other for
loops in that function as well.
No. Just change what you are adding. If you want to do a cleanup of other things
that should be it's own CL. It is fine to use new features as new things are
developed, then as things need to be touched later they can be updated with new
technology.
> No. Just change what you are adding. If you want to do a cleanup of other things > that should be it's own CL. It is fine to use new features as new things are > developed, then as things need to be touched later they can be updated with new > technology. I'd leave this up to the patch author - we value consistency just as much as clarity.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
Strictly speaking, this requires a test, but our testing harness is not yet contributor-friendly. Do you build chrome or use a lightweight front-end contribution process? https://codereview.chromium.org/1842323002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/1842323002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:965: var nodesToRefresh = []; You should annotate this array as !Array<WebInspector.NetworkDataGridNode>. You can also fix the one above as a drive-by. https://codereview.chromium.org/1842323002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:980: if (!isFilteredOut) { This should be in the else branch - otherwise you are pushing the same node both into nodesToInsert and nodesToRefresh. https://codereview.chromium.org/1842323002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:997: node.refresh(); We are now using "for of" for non performance critical code, so you could do: for (var node of nodesToRefresh) node.refresh() I personally prefer it to forEach since you don't create a new function.
https://codereview.chromium.org/1842323002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/1842323002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:965: var nodesToRefresh = []; On 2016/04/01 17:47:13, pfeldman wrote: > You should annotate this array as !Array<WebInspector.NetworkDataGridNode>. You > can also fix the one above as a drive-by. Done. https://codereview.chromium.org/1842323002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:980: if (!isFilteredOut) { On 2016/04/01 17:47:13, pfeldman wrote: > This should be in the else branch - otherwise you are pushing the same node both > into nodesToInsert and nodesToRefresh. I removed the refresh call from inside the nodesToInsert loop, so the nodes still need to be refreshed after being inserted. Otherwise request data won't be updated if the request was filtered out while the request data changed. By pushing the node into both nodesToInsert and nodesToRefresh the node will be updated when the filter is removed. https://codereview.chromium.org/1842323002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:997: node.refresh(); On 2016/04/01 17:47:13, pfeldman wrote: > We are now using "for of" for non performance critical code, so you could do: > > for (var node of nodesToRefresh) > node.refresh() > > I personally prefer it to forEach since you don't create a new function. Done.
Regarding tests, I'm currently using the lightweight process using just the front-end code. I tried building in the past and didn't have much luck, so I don't think I'll be able to write a test for this in a reasonable amount of time.
looks good % comments. fix them and we can land this, thanks! https://codereview.chromium.org/1842323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (right): https://codereview.chromium.org/1842323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:964: var nodesToInsert = /** @type {!Array<!WebInspector.NetworkDataGridNode> } */ ([]); /** @type {!Array<!WebInspector.NetworkDataGridNode> } */ var nodesToInsert = []; would be the right declaration. https://codereview.chromium.org/1842323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:980: if (!isFilteredOut) { style nit: drop the {}
Cool, I've made those changes. Thanks for your help!
lgtm I did not find your name in the src/AUTHORS. Please add it there so that we could land. Only needs to be done once.
Sure, done.
The CQ bit was checked by matt@mostlystatic.com
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1842323002/#ps60001 (title: "Add name to AUTHORS file.")
The CQ bit was unchecked by matt@mostlystatic.com
The CQ bit was checked by matt@mostlystatic.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1842323002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1842323002/60001
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Fix stale requests in Network panel not being refreshed Fix bug where changes to network requests wouldn't be reflected in the Network panel. For example, a finished request would show as "pending" until a refresh was forced by filtering the request out and then resetting the filter. BUG=599404 ========== to ========== [DevTools] Fix stale requests in Network panel not being refreshed Fix bug where changes to network requests wouldn't be reflected in the Network panel. For example, a finished request would show as "pending" until a refresh was forced by filtering the request out and then resetting the filter. BUG=599404 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Fix stale requests in Network panel not being refreshed Fix bug where changes to network requests wouldn't be reflected in the Network panel. For example, a finished request would show as "pending" until a refresh was forced by filtering the request out and then resetting the filter. BUG=599404 ========== to ========== [DevTools] Fix stale requests in Network panel not being refreshed Fix bug where changes to network requests wouldn't be reflected in the Network panel. For example, a finished request would show as "pending" until a refresh was forced by filtering the request out and then resetting the filter. BUG=599404 Committed: https://crrev.com/35e85822440aa0729ff35b81dda5fcd61878f242 Cr-Commit-Position: refs/heads/master@{#384685} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/35e85822440aa0729ff35b81dda5fcd61878f242 Cr-Commit-Position: refs/heads/master@{#384685} |
