Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647803002:
[Devtools] Fixed network to sort items as they are received. (Closed)
|
Created:
3 years, 11 months ago by allada Modified:
3 years, 11 months ago 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] Fixed network to sort items as they are received.
This patch fixes a bug where if an item is received in network panel it
was always puting it at the bottom even if it was not sorted in that
manner. This patch re-sorts the grid properly.
This patch also handles the preservation of the sriping by anchoring it
to the top item in the list (keeps the strips from flipping if items are
received and put above your current view port list).
R=dgozman,luoe,caseq
BUG=682861
Review-Url: https://codereview.chromium.org/2647803002
Cr-Commit-Position: refs/heads/master@{#445232}
Committed: https://chromium.googlesource.com/chromium/src/+/1c4bf390cfc5619a04b3b67fdc9930234e407fea
Patch Set 1 : [Devtools] Fixed network to sort items as they are received. #Patch Set 2 : Merge branch 'GROUP_CONNECTION_ID_DEFAULT' into GROUP_SCROLL_PROPERLY #
Total comments: 12
Patch Set 3 : changes #Patch Set 4 : changes #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 31 (23 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
PTL
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: This issue passed the CQ dry run.
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: This issue passed the CQ dry run.
NLogViewColumns looks good to me! Please see comments for the other files. https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js (right): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:215: this._previousTopOffset = offset; Even/oddness could be checked by relying on the class of the previous visibleNodes[0], e.g. visibleNodes[0].element().classList.has('odd'). If even/oddness is determined by the viewport, perhaps having viewportState could store something like previousFirstVislbeNodeWasOdd, instead of putting this on every node? Having _isOdd on every NetworkNode sounds like more than is needed. What do you think? https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (left): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:846: this._columns.dataChanged(); Is this the only call to dataChanged? If so let's remove the method def too. https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:630: if (isEvenRow) Looks safe, but it used to check if row was odd. Is this intentional?
PTaL https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js (right): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:215: this._previousTopOffset = offset; On 2017/01/20 18:05:48, luoe wrote: > Even/oddness could be checked by relying on the class of the previous > visibleNodes[0], e.g. visibleNodes[0].element().classList.has('odd'). If > even/oddness is determined by the viewport, perhaps having viewportState could > store something like previousFirstVislbeNodeWasOdd, instead of putting this on > every node? > > Having _isOdd on every NetworkNode sounds like more than is needed. What do you > think? I didn't want to force element to be constructed. Although it's extremely likely it is created, it feels wrong. https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js (left): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:846: this._columns.dataChanged(); On 2017/01/20 18:05:48, luoe wrote: > Is this the only call to dataChanged? If so let's remove the method def too. Done. https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:630: if (isEvenRow) On 2017/01/20 18:05:49, luoe wrote: > Looks safe, but it used to check if row was odd. Is this intentional? Yes, it's not really "odd" or "even" any more because it anchors on whatever the top item in the viewport is. really this is just a way to know if it should be alternated colored.
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js (right): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:43: this._previousTopOffset = 0; /** @type {boolean} */ this._firstVisibleIsStriped https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:210: var index = nodes.findIndex(node => visibleNodes[0] === node); nodes.indexOf(visibleNodes[0]) https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:50: return this._isOdd; - Let's make this a part of base class. - isStriped
The CQ bit was checked by allada@chromium.org to run a CQ dry run
PTaL https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js (right): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:43: this._previousTopOffset = 0; On 2017/01/20 22:14:40, dgozman wrote: > /** @type {boolean} */ > this._firstVisibleIsStriped Done. https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:210: var index = nodes.findIndex(node => visibleNodes[0] === node); On 2017/01/20 22:14:40, dgozman wrote: > nodes.indexOf(visibleNodes[0]) Done. https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2647803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:50: return this._isOdd; On 2017/01/20 22:14:40, dgozman wrote: > - Let's make this a part of base class. > - isStriped Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Devtools] Fixed network to sort items as they are received. This patch fixes a bug where if an item is received in network panel it was always puting it at the bottom even if it was not sorted in that manner. This patch re-sorts the grid properly. This patch also handles the preservation of the sriping by anchoring it to the top item in the list (keeps the strips from flipping if items are received and put above your current view port list). R=dgozman,luoe,caseq BUG=682861 ========== to ========== [Devtools] Fixed network to sort items as they are received. This patch fixes a bug where if an item is received in network panel it was always puting it at the bottom even if it was not sorted in that manner. This patch re-sorts the grid properly. This patch also handles the preservation of the sriping by anchoring it to the top item in the list (keeps the strips from flipping if items are received and put above your current view port list). R=dgozman,luoe,caseq BUG=682861 ==========
allada@chromium.org changed reviewers: + paulirish@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by allada@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484956097624810,
"parent_rev": "b5ac0ebfe4e03b8e1da707fa38f29550022a94ea", "commit_rev":
"1c4bf390cfc5619a04b3b67fdc9930234e407fea"}
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Fixed network to sort items as they are received. This patch fixes a bug where if an item is received in network panel it was always puting it at the bottom even if it was not sorted in that manner. This patch re-sorts the grid properly. This patch also handles the preservation of the sriping by anchoring it to the top item in the list (keeps the strips from flipping if items are received and put above your current view port list). R=dgozman,luoe,caseq BUG=682861 ========== to ========== [Devtools] Fixed network to sort items as they are received. This patch fixes a bug where if an item is received in network panel it was always puting it at the bottom even if it was not sorted in that manner. This patch re-sorts the grid properly. This patch also handles the preservation of the sriping by anchoring it to the top item in the list (keeps the strips from flipping if items are received and put above your current view port list). R=dgozman,luoe,caseq BUG=682861 Review-Url: https://codereview.chromium.org/2647803002 Cr-Commit-Position: refs/heads/master@{#445232} Committed: https://chromium.googlesource.com/chromium/src/+/1c4bf390cfc5619a04b3b67fdc99... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1c4bf390cfc5619a04b3b67fdc99... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
