Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2747293002:
[Devtools] Make network waterfall draw in layers (Closed)
|
Created:
3 years, 9 months ago by allada Modified:
3 years, 9 months ago Reviewers:
caseq 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/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Make network waterfall draw in layers
This patch finishes the move to make the waterfall column in the network
panel draw in layers. This ensures it looks good when grouping.
R=caseq
BUG=664704
Review-Url: https://codereview.chromium.org/2747293002
Cr-Commit-Position: refs/heads/master@{#457934}
Committed: https://chromium.googlesource.com/chromium/src/+/595249cfd33d23fc10a9615b1252afa09ad35153
Patch Set 1 : [Devtools] Make network waterfall draw in layers #
Total comments: 14
Patch Set 2 : changes #Patch Set 3 : unbase changes #
Messages
Total messages: 28 (16 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
PTL
https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:87: _buildRequestTimeRangeStyle() { let's make it static https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:109: _buildResourceTypeStyle() { ditto. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:128: if (!color) console.assert(color) before this? https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:159: _setupPathDrawOrder() { rename to something like _clearPaths() or _resetPaths() and move this._pathForStyle.clear() here -- these make much more sense together. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:422: for (var value of this._pathForStyle) { s/value/entry/ -- entry is [key, value], so using value instead is a bit confusing. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:510: var waitingStyle = /** @type {!Network.NetworkWaterfallColumn._LayerStyle} */ ( nit: if the case is here just to keep closure compiler from complaining, this could as well be '... || {}' instead. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:516: var downloadingStyle = /** @type {!Network.NetworkWaterfallColumn._LayerStyle} */ ( ditto.
PTaL https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:87: _buildRequestTimeRangeStyle() { On 2017/03/15 00:41:30, caseq wrote: > let's make it static Done. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:109: _buildResourceTypeStyle() { On 2017/03/15 00:41:29, caseq wrote: > ditto. Done. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:128: if (!color) On 2017/03/15 00:41:29, caseq wrote: > console.assert(color) before this? There are a quite a few that are not colored and default to "other". I can assert it and add them to the list above, but I think defaulting to "other" is easier for both readability and error checking. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:159: _setupPathDrawOrder() { On 2017/03/15 00:41:29, caseq wrote: > rename to something like _clearPaths() or _resetPaths() and move > this._pathForStyle.clear() here -- these make much more sense together. Done. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:422: for (var value of this._pathForStyle) { On 2017/03/15 00:41:30, caseq wrote: > s/value/entry/ -- entry is [key, value], so using value instead is a bit > confusing. Done. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:510: var waitingStyle = /** @type {!Network.NetworkWaterfallColumn._LayerStyle} */ ( On 2017/03/15 00:41:30, caseq wrote: > nit: if the case is here just to keep closure compiler from complaining, this > could as well be '... || {}' instead. Done. https://codereview.chromium.org/2747293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:516: var downloadingStyle = /** @type {!Network.NetworkWaterfallColumn._LayerStyle} */ ( On 2017/03/15 00:41:30, caseq wrote: > ditto. 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: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2747293002/#ps80001 (title: "unbase changes")
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
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
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
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
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
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
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": 1489799323266660,
"parent_rev": "c5523a13172a6973a608f64ed6b250a8aef014aa", "commit_rev":
"595249cfd33d23fc10a9615b1252afa09ad35153"}
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Make network waterfall draw in layers This patch finishes the move to make the waterfall column in the network panel draw in layers. This ensures it looks good when grouping. R=caseq BUG=664704 ========== to ========== [Devtools] Make network waterfall draw in layers This patch finishes the move to make the waterfall column in the network panel draw in layers. This ensures it looks good when grouping. R=caseq BUG=664704 Review-Url: https://codereview.chromium.org/2747293002 Cr-Commit-Position: refs/heads/master@{#457934} Committed: https://chromium.googlesource.com/chromium/src/+/595249cfd33d23fc10a9615b1252... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/595249cfd33d23fc10a9615b1252... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
