Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2726183004:
[Devtools] Finished inlining function for _drawSimplifiedBars (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/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Finished inlining function for _drawSimplifiedBars
This is a followup to https://codereview.chromium.org/2730633002/ to
finish inlining the function.
R=caseq
BUG=697668
Review-Url: https://codereview.chromium.org/2726183004
Cr-Commit-Position: refs/heads/master@{#455976}
Committed: https://chromium.googlesource.com/chromium/src/+/b9bca9acea99f9dae1e01260405509edba7271d7
Patch Set 1 #
Total comments: 4
Patch Set 2 : changes #Patch Set 3 : changes #Patch Set 4 : Merge branch 'z-round1-commitable-4-split' into z-round1-commitable-5-split #Messages
Total messages: 18 (11 generated)
PTL
https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:488: const midBarX = ranges.start + (ranges.mid - ranges.start) / 2 - leftLabelWidth / 2; nit: this could really be midBarX = (ranges.start + ranges.mid - leftLabelWidth) / 2; https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:508: this._textLayers.push({text: labels.right, x: endX + barDotLineLength + 1, y: y + this._fontSize}); ditto. actually, it would perhaps be nice to dedupe this branch wrt the one above.
PTaL https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js (right): https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:488: const midBarX = ranges.start + (ranges.mid - ranges.start) / 2 - leftLabelWidth / 2; On 2017/03/02 23:30:23, caseq wrote: > nit: this could really be > > midBarX = (ranges.start + ranges.mid - leftLabelWidth) / 2; Done. https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:508: this._textLayers.push({text: labels.right, x: endX + barDotLineLength + 1, y: y + this._fontSize}); On 2017/03/02 23:30:23, caseq wrote: > ditto. actually, it would perhaps be nice to dedupe this branch wrt the one > above. These branches do quite different things. They both decide weather the right text can fit in the right block, left text can fit in left block and if they cant they are not sown.
On 2017/03/03 04:07:43, allada wrote: > PTaL > > https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... > File > third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js > (right): > > https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:488: > const midBarX = ranges.start + (ranges.mid - ranges.start) / 2 - leftLabelWidth > / 2; > On 2017/03/02 23:30:23, caseq wrote: > > nit: this could really be > > > > midBarX = (ranges.start + ranges.mid - leftLabelWidth) / 2; > > Done. Not really what I was suggesting, but it was a nit picking anyway, so lgtm.
On 2017/03/06 17:16:49, caseq wrote: > On 2017/03/03 04:07:43, allada wrote: > > PTaL > > > > > https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... > > File > > third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js > > (right): > > > > > https://codereview.chromium.org/2726183004/diff/1/third_party/WebKit/Source/d... > > > third_party/WebKit/Source/devtools/front_end/network/NetworkWaterfallColumn.js:488: > > const midBarX = ranges.start + (ranges.mid - ranges.start) / 2 - > leftLabelWidth > > / 2; > > On 2017/03/02 23:30:23, caseq wrote: > > > nit: this could really be > > > > > > midBarX = (ranges.start + ranges.mid - leftLabelWidth) / 2; > > > > Done. > > Not really what I was suggesting, but it was a nit picking anyway, so lgtm. forgot to adjust that one. Fixed
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 allada@chromium.org
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/2726183004/#ps60001 (title: "Merge branch 'z-round1-commitable-4-split' into z-round1-commitable-5-split")
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": 60001, "attempt_start_ts": 1489112982812280,
"parent_rev": "128ef2d7248bd291e08ddfa30c8444dc26abc9b8", "commit_rev":
"b9bca9acea99f9dae1e01260405509edba7271d7"}
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Finished inlining function for _drawSimplifiedBars This is a followup to https://codereview.chromium.org/2730633002/ to finish inlining the function. R=caseq BUG=697668 ========== to ========== [Devtools] Finished inlining function for _drawSimplifiedBars This is a followup to https://codereview.chromium.org/2730633002/ to finish inlining the function. R=caseq BUG=697668 Review-Url: https://codereview.chromium.org/2726183004 Cr-Commit-Position: refs/heads/master@{#455976} Committed: https://chromium.googlesource.com/chromium/src/+/b9bca9acea99f9dae1e012604055... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b9bca9acea99f9dae1e012604055... |
