Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2649653002:
[Devtools] Added padding to child nodes in network group experiment (Closed)
|
Created:
3 years, 11 months ago by allada Modified:
3 years, 10 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, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Added padding to child nodes in network group experiment
This patch adds 15px padding to child nodes recursively.
R=dgozman,luoe,caseq
BUG=664704
Review-Url: https://codereview.chromium.org/2649653002
Cr-Commit-Position: refs/heads/master@{#446798}
Committed: https://chromium.googlesource.com/chromium/src/+/bb786f1d99f2e0fc427e59c2dd6db9cd0b24b273
Patch Set 1 : [Devtools] Added padding to child nodes in network group experiment #
Total comments: 4
Patch Set 2 : changes #Patch Set 3 : changes #Patch Set 4 : changes #
Total comments: 4
Patch Set 5 : changes #Messages
Total messages: 25 (12 generated)
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
PTL
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2649653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2649653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:723: leftPadding += 15; What is 15 and what is it doing here? https://codereview.chromium.org/2649653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:725: cell.style.paddingLeft = leftPadding + 'px'; I don't think we should be assuming particular presentation constants in JS sources -- we use CSS to style things where possible.
Patchset #2 (id:40001) has been deleted
PTaL https://codereview.chromium.org/2649653002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2649653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:723: leftPadding += 15; On 2017/01/21 01:22:06, caseq wrote: > What is 15 and what is it doing here? This is similar to how this does it: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... https://codereview.chromium.org/2649653002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:725: cell.style.paddingLeft = leftPadding + 'px'; On 2017/01/21 01:22:06, caseq wrote: > I don't think we should be assuming particular presentation constants in JS > sources -- we use CSS to style things where possible. CSS can't count number of parents with class name (that I know of).
Another place that does this via CSS is for console groups: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... A message that is N levels deep gets N 'indent' elements before the actual content, each of which has a width.
case@ can I get your input, I talked to luoe@ offline and the trade off is that we have networkpanel do nesting differently than timeline does with datagrid or we make this implementation of how datagrid handles nesting work more like console. Keep in mind console does not use datagrid it uses it's own viewport stuff.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/2649653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2649653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:953: if (leftPadding) Why doesn't this.leftPadding work for you?
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
PTaL https://codereview.chromium.org/2649653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2649653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:953: if (leftPadding) On 2017/01/25 23:14:03, pfeldman wrote: > Why doesn't this.leftPadding work for you? I didn't know it existed. I was deferring to how timeline did it.
https://codereview.chromium.org/2649653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2649653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:953: if (leftPadding) > I didn't know it existed. I was deferring to how timeline did it. Could you point to where it is or better yet fix it as well?
https://codereview.chromium.org/2649653002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js (right): https://codereview.chromium.org/2649653002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:953: if (leftPadding) On 2017/01/26 00:29:31, pfeldman wrote: > > I didn't know it existed. I was deferring to how timeline did it. > > Could you point to where it is or better yet fix it as well? Here is where it's at: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... I will do it in a separate patch, since they are not related.
That's actually fine - it is not iterating the loop as you did.
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": 160001, "attempt_start_ts": 1485548645579850,
"parent_rev": "e776de897dff1a31c90406a08d063c6594be935c", "commit_rev":
"bb786f1d99f2e0fc427e59c2dd6db9cd0b24b273"}
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Added padding to child nodes in network group experiment This patch adds 15px padding to child nodes recursively. R=dgozman,luoe,caseq BUG=664704 ========== to ========== [Devtools] Added padding to child nodes in network group experiment This patch adds 15px padding to child nodes recursively. R=dgozman,luoe,caseq BUG=664704 Review-Url: https://codereview.chromium.org/2649653002 Cr-Commit-Position: refs/heads/master@{#446798} Committed: https://chromium.googlesource.com/chromium/src/+/bb786f1d99f2e0fc427e59c2dd6d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/chromium/src/+/bb786f1d99f2e0fc427e59c2dd6d... |
