Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
333073005:
DevTools: Draw multiple textures for layers. (Closed)
|
Created:
6 years, 6 months ago by malch Modified:
6 years, 6 months ago Reviewers:
caseq CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionDraw multiple textures for layers.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176496
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixes. #
Total comments: 4
Patch Set 3 : Add tile type annotation. #Messages
Total messages: 12 (0 generated)
Please take a look
https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... File Source/devtools/front_end/timeline/Layers3DView.js (right): https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timeline/Layers3DView.js:50: this._tilesForLayer = {}; Technically, these are pictures, not tiles. https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timeline/Layers3DView.js:412: * @return {!{color: !Array.<number>, borderColor: !Array.<number>}} Now that it grows bigger, let's extract into a class? https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timeline/Layers3DView.js:427: return {color: color, borderColor: borderColor, width: width}; s/width/borderWidth/? https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timeline/Layers3DView.js:534: vertices = this._calculateVerticesForQuad(quad, this._depthByLayerId[layer.id()] * WebInspector.Layers3DView.LayerSpacing); let's extract depth into a var and reuse it in other places within this method.
https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... File Source/devtools/front_end/timeline/Layers3DView.js (right): https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timeline/Layers3DView.js:50: this._tilesForLayer = {}; On 2014/06/18 12:16:39, caseq wrote: > Technically, these are pictures, not tiles. Done. https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timeline/Layers3DView.js:412: * @return {!{color: !Array.<number>, borderColor: !Array.<number>}} On 2014/06/18 12:16:39, caseq wrote: > Now that it grows bigger, let's extract into a class? Done. https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timeline/Layers3DView.js:427: return {color: color, borderColor: borderColor, width: width}; On 2014/06/18 12:16:39, caseq wrote: > s/width/borderWidth/? Done. https://codereview.chromium.org/333073005/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timeline/Layers3DView.js:534: vertices = this._calculateVerticesForQuad(quad, this._depthByLayerId[layer.id()] * WebInspector.Layers3DView.LayerSpacing); On 2014/06/18 12:16:39, caseq wrote: > let's extract depth into a var and reuse it in other places within this method. Done.
lgtm https://codereview.chromium.org/333073005/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/timeline/Layers3DView.js (right): https://codereview.chromium.org/333073005/diff/20001/Source/devtools/front_en... Source/devtools/front_end/timeline/Layers3DView.js:188: * @param {!Array.<!Object>} tiles Can we be more specific than just object? https://codereview.chromium.org/333073005/diff/20001/Source/devtools/front_en... Source/devtools/front_end/timeline/Layers3DView.js:197: * @param {!Object} tile ditto.
https://codereview.chromium.org/333073005/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/timeline/Layers3DView.js (right): https://codereview.chromium.org/333073005/diff/20001/Source/devtools/front_en... Source/devtools/front_end/timeline/Layers3DView.js:188: * @param {!Array.<!Object>} tiles On 2014/06/18 12:56:19, caseq wrote: > Can we be more specific than just object? Done. https://codereview.chromium.org/333073005/diff/20001/Source/devtools/front_en... Source/devtools/front_end/timeline/Layers3DView.js:197: * @param {!Object} tile On 2014/06/18 12:56:19, caseq wrote: > ditto. Done.
The CQ bit was checked by caseq@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/malch@chromium.org/333073005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by caseq@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/malch@chromium.org/333073005/40001
Message was sent while issue was closed.
Change committed as 176496 |
