Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
166273018:
Added showing slow scroll rectangles in Layers panel. (Closed)
|
Created:
6 years, 10 months ago by malch Modified:
6 years, 9 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+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, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdded showing slow scroll rectangles in Layers panel.
Review URL: https://codereview.chromium.org/166273018
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169735
Patch Set 1 #
Total comments: 5
Patch Set 2 : Style fixes. Moved refreshing logic from model to view. #
Total comments: 6
Patch Set 3 : More fixes. #
Total comments: 3
Patch Set 4 : Few more fixes. #
Total comments: 14
Patch Set 5 : And more fixes. #Patch Set 6 : Added test. #
Total comments: 18
Patch Set 7 : More fixes. Added one more test. #
Total comments: 16
Patch Set 8 : Fixes and updates. #
Total comments: 16
Patch Set 9 : And more fixes. #
Total comments: 6
Patch Set 10 : Final fixes. #
Total comments: 8
Patch Set 11 : Moved ScrollRects into Layer structure. #
Total comments: 14
Patch Set 12 : Fixes. #
Total comments: 14
Patch Set 13 : Fixes. #
Total comments: 16
Patch Set 14 : Fixes. Added showing all layers under content root. #
Total comments: 16
Patch Set 15 : New fixes. #
Total comments: 12
Patch Set 16 : Modified scroll rects vector checking on the frontend. Fixed test. #
Total comments: 11
Patch Set 17 : Fixes. #
Total comments: 8
Patch Set 18 : Fixed indentation. #Patch Set 19 : Few more fixes. #
Total comments: 4
Patch Set 20 : Final fixes. #
Total comments: 2
Patch Set 21 : After rebase (temporary). #Patch Set 22 : Fixed layer nodeId issue. #
Total comments: 14
Patch Set 23 : Fixes. #
Total comments: 2
Patch Set 24 : Small fix. #Patch Set 25 : Fixed test. #Messages
Total messages: 66 (0 generated)
Draft version of patch.
https://codereview.chromium.org/166273018/diff/1/Source/devtools/front_end/La... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/1/Source/devtools/front_end/La... Source/devtools/front_end/LayerTreeModel.js:162: _areScrollRectsEqual:function (first, second) { Mind the style please: { => next line, function(args) LayerTreeModel._scrollRectsEqual = function() .... https://codereview.chromium.org/166273018/diff/1/Source/devtools/front_end/La... Source/devtools/front_end/LayerTreeModel.js:173: var scrollRectsByLayerId = {}; newScollRectsByLayerId so we can better distinguish them form old ones in this._scrollRectsByLayerId https://codereview.chromium.org/166273018/diff/1/Source/devtools/front_end/La... Source/devtools/front_end/LayerTreeModel.js:182: if (scrollRect.layerId in scrollRectsByLayerId) { style: no need for { } here https://codereview.chromium.org/166273018/diff/1/Source/devtools/front_end/La... Source/devtools/front_end/LayerTreeModel.js:199: this._scrollRectsByLayerId[layerId][i].needsUpdate = true; extract something like var rect = this._scrollRectsByLayerId[layerId][i] https://codereview.chromium.org/166273018/diff/1/Source/devtools/front_end/La... Source/devtools/front_end/LayerTreeModel.js:199: this._scrollRectsByLayerId[layerId][i].needsUpdate = true; I don't like needsUpdate
Made fixes.
https://codereview.chromium.org/166273018/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/60001/Source/devtools/front_en... Source/devtools/front_end/LayerTreeModel.js:198: this._scrollRectsByLayerId[layerId].push(newScrollRectsByLayerId[layerId][i]); This could be merged with the other branch of this if. https://codereview.chromium.org/166273018/diff/60001/Source/devtools/front_en... Source/devtools/front_end/LayerTreeModel.js:204: this._scrollRectsByLayerId[layerId].length - newScrollRectsByLayerId[layerId].length); drop this argument https://codereview.chromium.org/166273018/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/60001/Source/devtools/front_en... Source/devtools/front_end/Layers3DView.js:90: NonFastScrollable: "repaints on scroll", Use WebInspector.UIString() https://codereview.chromium.org/166273018/diff/60001/Source/devtools/front_en... Source/devtools/front_end/Layers3DView.js:310: var color = WebInspector.Color.fromRGBA([0xB2, 0, 0, 0.2]); Do this as part of scroll-rect CSS class instead. https://codereview.chromium.org/166273018/diff/60001/Source/devtools/front_en... Source/devtools/front_end/Layers3DView.js:344: if (this._scrollRectElementsByLayerId[layerId].length > this._model._scrollRectsByLayerId[layerId].length) { No need for this if. https://codereview.chromium.org/166273018/diff/60001/Source/devtools/front_en... Source/devtools/front_end/Layers3DView.js:347: this._scrollRectElementsByLayerId[layerId].splice(this._model._scrollRectsByLayerId[layerId].length, How about this._scrollRectElementsByLayerId[layerId].splice(this._model._scrollRectsByLayerId[layerId].length).forEach(removeElement) function removeElement(element) { element.remove() }
More fixes.
https://codereview.chromium.org/166273018/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/120001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:58: return (first.x === second.x) && (first.y === second.y) && No need for parenthesis here. https://codereview.chromium.org/166273018/diff/120001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:199: this._scrollRectsByLayerId[layerId][i] = newScrollRectsByLayerId[layerId][i]; style: this line needs to go into a { block } because the if is split in tow lines. https://codereview.chromium.org/166273018/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/120001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:334: if (i >= this._scrollRectElementsByLayerId[layerId].length) { Extract to var scrollRectElements = this._scrollRectElementsByLayerId[layerId]; outside of the loop.
Few more fixes.
https://codereview.chromium.org/166273018/diff/190001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.h (right): https://codereview.chromium.org/166273018/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.h:87: bool buildLayerTree(TypeBuilder::Array<TypeBuilder::LayerTree::Layer>& layers, style: drop self-obvious argument names from the method declaration. https://codereview.chromium.org/166273018/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.h:88: TypeBuilder::Array<TypeBuilder::LayerTree::ScrollRect>& scrollRects); ditto https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:197: if (i >= this._scrollRectsByLayerId[layerId].length || extract this._scrollRectsByLayerId into a var. https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:303: this._elementsByLayerId[this._model.contentRoot().id()]; Let's split into something like var parentLayerId = this._model._layersById[scrollRect.layerId].nodeId() ? scrollRect.layerId : this._model.contentRoot().id(); var parentLayerElement = this._elementsByLayerId[parentLayerId] https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:327: _paintScrollRects: function() s/paint/update/ https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... File Source/devtools/front_end/layersPanel.css (right): https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... Source/devtools/front_end/layersPanel.css:135: border: inherit solid rgb(178, 0, 0); looks weird. should it be something like border-width: inherit; border: ... ? https://codereview.chromium.org/166273018/diff/190001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/166273018/diff/190001/Source/devtools/protoco... Source/devtools/protocol.json:3866: { "name": "type", "type": "string", "enum": ["NonFastScrollable", "TouchEventHandler", "WheelEventHandler"], "description": "Rectangle type." } They're all non-fast scrollable, so this looks a bit weird. RepaintsOnScroll perhaps?
https://codereview.chromium.org/166273018/diff/190001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.h (right): https://codereview.chromium.org/166273018/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.h:87: bool buildLayerTree(TypeBuilder::Array<TypeBuilder::LayerTree::Layer>& layers, On 2014/02/20 16:30:45, caseq wrote: > style: drop self-obvious argument names from the method declaration. Done. https://codereview.chromium.org/166273018/diff/190001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.h:88: TypeBuilder::Array<TypeBuilder::LayerTree::ScrollRect>& scrollRects); On 2014/02/20 16:30:45, caseq wrote: > ditto Done. https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:197: if (i >= this._scrollRectsByLayerId[layerId].length || On 2014/02/20 16:30:45, caseq wrote: > extract this._scrollRectsByLayerId into a var. Done. https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:303: this._elementsByLayerId[this._model.contentRoot().id()]; On 2014/02/20 16:30:45, caseq wrote: > Let's split into something like > var parentLayerId = this._model._layersById[scrollRect.layerId].nodeId() ? > scrollRect.layerId : this._model.contentRoot().id(); > var parentLayerElement = this._elementsByLayerId[parentLayerId] Done. https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:327: _paintScrollRects: function() On 2014/02/20 16:30:45, caseq wrote: > s/paint/update/ Done. https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... File Source/devtools/front_end/layersPanel.css (right): https://codereview.chromium.org/166273018/diff/190001/Source/devtools/front_e... Source/devtools/front_end/layersPanel.css:135: border: inherit solid rgb(178, 0, 0); On 2014/02/20 16:30:45, caseq wrote: > looks weird. should it be something like > border-width: inherit; > border: ... > ? Done. https://codereview.chromium.org/166273018/diff/190001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/166273018/diff/190001/Source/devtools/protoco... Source/devtools/protocol.json:3866: { "name": "type", "type": "string", "enum": ["NonFastScrollable", "TouchEventHandler", "WheelEventHandler"], "description": "Rectangle type." } On 2014/02/20 16:30:45, caseq wrote: > They're all non-fast scrollable, so this looks a bit weird. RepaintsOnScroll > perhaps? Done.
Added test for slow scroll rectangles.
https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... File LayoutTests/inspector/layer-scroll-rects.html (right): https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:9: {layerId: 1, offsetX: 0, offsetY: 0, width: 0, height: 0, paintCount: 0}, please make layerIds strings to match the protocol. https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:18: {layerId: 3, x: 10, y: 10, width: 10, height: 10, type: "WheelEventHandler", unchanged: true}, unchanged? https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:37: for (var element = layerElement.firstElementChild; element; element = element.nextSibling) { This does not seem to recurse into sub-layers. https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:38: if (element.className !== "scroll-rect") Use root.querySelectorAll(".scroll-rect") https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:52: InspectorTest.addResult("First dump"); "Initial scroll rectangles" https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:57: InspectorTest.addResult("Second dump"); "Updated scroll rectangles" https://codereview.chromium.org/166273018/diff/310001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/310001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:179: // FIXME You can remove this once https://codereview.chromium.org/173763002/ lands https://codereview.chromium.org/166273018/diff/310001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/310001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:344: this._updateScrollRectsForLayer(this._model._scrollRectsByLayerId[layerId], We shouldn't access a "private" field in a class defined in different file. https://codereview.chromium.org/166273018/diff/310001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/166273018/diff/310001/Source/devtools/protoco... Source/devtools/protocol.json:3866: { "name": "type", "type": "string", "enum": ["RepaintsOnScroll", "TouchEventHandler", "WheelEventHandler"], "description": "Rectangle type." } s/Rectangle type/Reason for rectangle to be scrolled on the main thread/
Added one more test. https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... File LayoutTests/inspector/layer-scroll-rects.html (right): https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:9: {layerId: 1, offsetX: 0, offsetY: 0, width: 0, height: 0, paintCount: 0}, On 2014/02/24 07:18:06, caseq wrote: > please make layerIds strings to match the protocol. Done. https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:18: {layerId: 3, x: 10, y: 10, width: 10, height: 10, type: "WheelEventHandler", unchanged: true}, On 2014/02/24 07:18:06, caseq wrote: > unchanged? Done. https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:37: for (var element = layerElement.firstElementChild; element; element = element.nextSibling) { On 2014/02/24 07:18:06, caseq wrote: > This does not seem to recurse into sub-layers. Done. https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:38: if (element.className !== "scroll-rect") On 2014/02/24 07:18:06, caseq wrote: > Use root.querySelectorAll(".scroll-rect") Done. https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:52: InspectorTest.addResult("First dump"); On 2014/02/24 07:18:06, caseq wrote: > "Initial scroll rectangles" Done. https://codereview.chromium.org/166273018/diff/310001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects.html:57: InspectorTest.addResult("Second dump"); On 2014/02/24 07:18:06, caseq wrote: > "Updated scroll rectangles" Done. https://codereview.chromium.org/166273018/diff/310001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/310001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:179: // FIXME On 2014/02/24 07:18:06, caseq wrote: > You can remove this once https://codereview.chromium.org/173763002/ lands Done. https://codereview.chromium.org/166273018/diff/310001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/310001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:344: this._updateScrollRectsForLayer(this._model._scrollRectsByLayerId[layerId], On 2014/02/24 07:18:06, caseq wrote: > We shouldn't access a "private" field in a class defined in different file. Done. https://codereview.chromium.org/166273018/diff/310001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/166273018/diff/310001/Source/devtools/protoco... Source/devtools/protocol.json:3866: { "name": "type", "type": "string", "enum": ["RepaintsOnScroll", "TouchEventHandler", "WheelEventHandler"], "description": "Rectangle type." } On 2014/02/24 07:18:06, caseq wrote: > s/Rectangle type/Reason for rectangle to be scrolled on the main thread/ Done.
Generally looks good, few more nits left. https://codereview.chromium.org/166273018/diff/400001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/layers-test.js (right): https://codereview.chromium.org/166273018/diff/400001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:107: // InspectorTest.addResult("scrollRect: " + JSON.stringify(element.__scrollRect)); please remove https://codereview.chromium.org/166273018/diff/400001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:116: for (var i = 0, element; element = scrollRectElements[i]; ++i) { scrollRectElements.forEach()? https://codereview.chromium.org/166273018/diff/400001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:124: // InspectorTest.addResult("scrollRectsByLayerId: " + JSON.stringify(InspectorTest._layerTreeModel._scrollRectsByLayerId)); Please remove. https://codereview.chromium.org/166273018/diff/400001/LayoutTests/inspector/l... File LayoutTests/inspector/layer-scroll-rects-get-expected.txt (right): https://codereview.chromium.org/166273018/diff/400001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects-get-expected.txt:5: type : "WheelEventHandler" I can haz other rect types? https://codereview.chromium.org/166273018/diff/400001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (left): https://codereview.chromium.org/166273018/diff/400001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:205: void InspectorLayerTreeAgent::gatherGraphicsLayers(GraphicsLayer* root, HashMap<int, int>& layerIdToNodeIdMap, RefPtr<TypeBuilder::Array<TypeBuilder::LayerTree::Layer> >& layers) Why did this move? https://codereview.chromium.org/166273018/diff/400001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/400001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:130: drop the extra line please https://codereview.chromium.org/166273018/diff/400001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/400001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:222: this._updateScrollRects(scrollRects); this._updateScrollRects(scrollRects || []); https://codereview.chromium.org/166273018/diff/400001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/400001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:343: this._scrollRectElementsByLayerId[layerId] = []; Please extract this to something like var scrollRects = this._model.scrollRectsByLayerId()[layerId] for better readability.
https://codereview.chromium.org/166273018/diff/400001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/layers-test.js (right): https://codereview.chromium.org/166273018/diff/400001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:107: // InspectorTest.addResult("scrollRect: " + JSON.stringify(element.__scrollRect)); On 2014/02/24 14:38:56, caseq wrote: > please remove Done. https://codereview.chromium.org/166273018/diff/400001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:116: for (var i = 0, element; element = scrollRectElements[i]; ++i) { On 2014/02/24 14:38:56, caseq wrote: > scrollRectElements.forEach()? https://coderwall.com/p/jcmzxw https://codereview.chromium.org/166273018/diff/400001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:124: // InspectorTest.addResult("scrollRectsByLayerId: " + JSON.stringify(InspectorTest._layerTreeModel._scrollRectsByLayerId)); On 2014/02/24 14:38:56, caseq wrote: > Please remove. Done. https://codereview.chromium.org/166273018/diff/400001/LayoutTests/inspector/l... File LayoutTests/inspector/layer-scroll-rects-get-expected.txt (right): https://codereview.chromium.org/166273018/diff/400001/LayoutTests/inspector/l... LayoutTests/inspector/layer-scroll-rects-get-expected.txt:5: type : "WheelEventHandler" On 2014/02/24 14:38:56, caseq wrote: > I can haz other rect types? Done. https://codereview.chromium.org/166273018/diff/400001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (left): https://codereview.chromium.org/166273018/diff/400001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:205: void InspectorLayerTreeAgent::gatherGraphicsLayers(GraphicsLayer* root, HashMap<int, int>& layerIdToNodeIdMap, RefPtr<TypeBuilder::Array<TypeBuilder::LayerTree::Layer> >& layers) On 2014/02/24 14:38:56, caseq wrote: > Why did this move? Done. https://codereview.chromium.org/166273018/diff/400001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/400001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:130: On 2014/02/24 14:38:56, caseq wrote: > drop the extra line please Done. https://codereview.chromium.org/166273018/diff/400001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/400001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:222: this._updateScrollRects(scrollRects); On 2014/02/24 14:38:56, caseq wrote: > this._updateScrollRects(scrollRects || []); Done. https://codereview.chromium.org/166273018/diff/400001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/400001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:343: this._scrollRectElementsByLayerId[layerId] = []; On 2014/02/24 14:38:56, caseq wrote: > Please extract this to something like > var scrollRects = this._model.scrollRectsByLayerId()[layerId] > for better readability. Done.
https://codereview.chromium.org/166273018/diff/490001/LayoutTests/inspector/l... File LayoutTests/inspector/layers/layer-scroll-rects-get.html (right): https://codereview.chromium.org/166273018/diff/490001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-get.html:23: <div id='touchable' style="-webkit-transform:translateZ(100px);height:20px;width:200px;overflow:scroll;"> nit: we use double quotes by default. https://codereview.chromium.org/166273018/diff/490001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-get.html:28: element.addEventListener('touchstart', function(evt) {}, false); ditto https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:298: _createScrollRectElement: function(scrollRect) Please annotate. https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:302: var parentLayerid = parentLayerId https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:316: _updateScrollRectsForLayer: function(newScrollRects, oldScrollRects) Ditto. https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:323: oldScrollRects[i] = this._createScrollRectElement(newScrollRects[i]); oldScrollRects is a poor choice of name -- it contains elements and we're updating it, so perhaps just scrollRectElements? https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:346: this._scrollRectElementsByLayerId[layerId] Why is this line not a part of _updateScrollRectsForLayer()? https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... File Source/devtools/front_end/layersPanel.css (right): https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/layersPanel.css:137: background-color: rgba(178, 0, 0, 0.2); Please make them more visible, alpha of 0.4 looks ok to me.
https://codereview.chromium.org/166273018/diff/490001/LayoutTests/inspector/l... File LayoutTests/inspector/layers/layer-scroll-rects-get.html (right): https://codereview.chromium.org/166273018/diff/490001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-get.html:23: <div id='touchable' style="-webkit-transform:translateZ(100px);height:20px;width:200px;overflow:scroll;"> On 2014/02/25 16:38:41, caseq wrote: > nit: we use double quotes by default. Done. https://codereview.chromium.org/166273018/diff/490001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-get.html:28: element.addEventListener('touchstart', function(evt) {}, false); On 2014/02/25 16:38:41, caseq wrote: > ditto Done. https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:298: _createScrollRectElement: function(scrollRect) On 2014/02/25 16:38:41, caseq wrote: > Please annotate. Done. https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:302: var parentLayerid = On 2014/02/25 16:38:41, caseq wrote: > parentLayerId Done. https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:316: _updateScrollRectsForLayer: function(newScrollRects, oldScrollRects) On 2014/02/25 16:38:41, caseq wrote: > Ditto. Done. https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:323: oldScrollRects[i] = this._createScrollRectElement(newScrollRects[i]); On 2014/02/25 16:38:41, caseq wrote: > oldScrollRects is a poor choice of name -- it contains elements and we're > updating it, so perhaps just scrollRectElements? Done. https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:346: this._scrollRectElementsByLayerId[layerId] On 2014/02/25 16:38:41, caseq wrote: > Why is this line not a part of _updateScrollRectsForLayer()? Done. https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... File Source/devtools/front_end/layersPanel.css (right): https://codereview.chromium.org/166273018/diff/490001/Source/devtools/front_e... Source/devtools/front_end/layersPanel.css:137: background-color: rgba(178, 0, 0, 0.2); On 2014/02/25 16:38:41, caseq wrote: > Please make them more visible, alpha of 0.4 looks ok to me. Done.
lgtm https://codereview.chromium.org/166273018/diff/510001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/layers-test.js (right): https://codereview.chromium.org/166273018/diff/510001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:114: Array.prototype.forEach.call(root.querySelectorAll('.scroll-rect'), function(element) { nit: use InspectorTest.dumpViewScrollRect.bind(InspectorTest) instead of function literal. https://codereview.chromium.org/166273018/diff/510001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/510001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:134: addRegionObjects(webLayer->nonFastScrollableRegion(), nit: you don't have to wrap this call, we don't have line width limitations. https://codereview.chromium.org/166273018/diff/510001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:138: addRegionObjects(webLayer->touchEventHandlerRegion(), ditto.
The CQ bit was checked by malch@chromium.org
The CQ bit was unchecked by malch@chromium.org
https://codereview.chromium.org/166273018/diff/510001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/layers-test.js (right): https://codereview.chromium.org/166273018/diff/510001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:114: Array.prototype.forEach.call(root.querySelectorAll('.scroll-rect'), function(element) { On 2014/02/26 11:21:10, caseq wrote: > nit: use InspectorTest.dumpViewScrollRect.bind(InspectorTest) instead of > function literal. Done. https://codereview.chromium.org/166273018/diff/510001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/510001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:134: addRegionObjects(webLayer->nonFastScrollableRegion(), On 2014/02/26 11:21:10, caseq wrote: > nit: you don't have to wrap this call, we don't have line width limitations. Done. https://codereview.chromium.org/166273018/diff/510001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:138: addRegionObjects(webLayer->touchEventHandlerRegion(), On 2014/02/26 11:21:10, caseq wrote: > ditto. Done.
https://codereview.chromium.org/166273018/diff/530001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/530001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:130: static void addScrollRectsForLayer(GraphicsLayer* graphicsLayer, We typically call these buildObjectFor https://codereview.chromium.org/166273018/diff/530001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/530001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:56: WebInspector.LayerTreeModel._scrollRectsEqual = function(first, second) _scrollRectsAreEqual https://codereview.chromium.org/166273018/diff/530001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/166273018/diff/530001/Source/devtools/protoco... Source/devtools/protocol.json:3857: "id": "ScrollRect", Can ScrollRect contain Rect + type instead? https://codereview.chromium.org/166273018/diff/530001/Source/devtools/protoco... Source/devtools/protocol.json:3865: { "name": "layerId", "$ref": "LayerId", "description": "Parent layer id." }, Instead of this binding, could you make it a part of a layer?
https://codereview.chromium.org/166273018/diff/530001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/530001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:130: static void addScrollRectsForLayer(GraphicsLayer* graphicsLayer, On 2014/02/26 11:49:40, pfeldman wrote: > We typically call these buildObjectFor Done. https://codereview.chromium.org/166273018/diff/530001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/530001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:56: WebInspector.LayerTreeModel._scrollRectsEqual = function(first, second) On 2014/02/26 11:49:40, pfeldman wrote: > _scrollRectsAreEqual Done. https://codereview.chromium.org/166273018/diff/530001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/166273018/diff/530001/Source/devtools/protoco... Source/devtools/protocol.json:3857: "id": "ScrollRect", On 2014/02/26 11:49:40, pfeldman wrote: > Can ScrollRect contain Rect + type instead? Done. https://codereview.chromium.org/166273018/diff/530001/Source/devtools/protoco... Source/devtools/protocol.json:3865: { "name": "layerId", "$ref": "LayerId", "description": "Parent layer id." }, On 2014/02/26 11:49:40, pfeldman wrote: > Instead of this binding, could you make it a part of a layer? Done.
https://codereview.chromium.org/166273018/diff/550001/LayoutTests/inspector/l... File LayoutTests/inspector/layers/layer-scroll-rects-update.html (right): https://codereview.chromium.org/166273018/diff/550001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-update.html:11: scrollRects: [{rect: {x: 0, y: 10, width: 10, height: 10}, type: "RepaintsOnScroll", unchanged: true}]}, please format similar to protocol.json https://codereview.chromium.org/166273018/diff/550001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.h (right): https://codereview.chromium.org/166273018/diff/550001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.h:91: bool buildLayerTree(TypeBuilder::Array<TypeBuilder::LayerTree::Layer>&); I think you can revert this to original signature now? https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:415: this._scrollRects = []; We could have this set unconditionally in _reset(); https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:417: this._layerPayload.scrollRects = []; Why is this necessary? Touching the original payload looks like a bad idea. https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:419: var newRects = this._layerPayload.scrollRects; hint: var newRects = this._layerPayload.scrollRects || []; https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:238: if (!this._scrollRectElementsByLayerId[layer.id()]) weird indent https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:240: var newScrollRects = layer.scrollRects(), scrollRectElements = this._scrollRectElementsByLayerId[layer.id()]; please split into two lines
https://codereview.chromium.org/166273018/diff/550001/LayoutTests/inspector/l... File LayoutTests/inspector/layers/layer-scroll-rects-update.html (right): https://codereview.chromium.org/166273018/diff/550001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-update.html:11: scrollRects: [{rect: {x: 0, y: 10, width: 10, height: 10}, type: "RepaintsOnScroll", unchanged: true}]}, On 2014/02/27 13:46:55, caseq wrote: > please format similar to protocol.json Done. https://codereview.chromium.org/166273018/diff/550001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.h (right): https://codereview.chromium.org/166273018/diff/550001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.h:91: bool buildLayerTree(TypeBuilder::Array<TypeBuilder::LayerTree::Layer>&); On 2014/02/27 13:46:55, caseq wrote: > I think you can revert this to original signature now? Done. https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:415: this._scrollRects = []; On 2014/02/27 13:46:55, caseq wrote: > We could have this set unconditionally in _reset(); Done. https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:417: this._layerPayload.scrollRects = []; On 2014/02/27 13:46:55, caseq wrote: > Why is this necessary? Touching the original payload looks like a bad idea. Done. https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:419: var newRects = this._layerPayload.scrollRects; On 2014/02/27 13:46:55, caseq wrote: > hint: var newRects = this._layerPayload.scrollRects || []; Done. https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:238: if (!this._scrollRectElementsByLayerId[layer.id()]) On 2014/02/27 13:46:55, caseq wrote: > weird indent Done. https://codereview.chromium.org/166273018/diff/550001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:240: var newScrollRects = layer.scrollRects(), scrollRectElements = this._scrollRectElementsByLayerId[layer.id()]; On 2014/02/27 13:46:55, caseq wrote: > please split into two lines Done.
lgtm. Pavel? https://codereview.chromium.org/166273018/diff/570001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/layers-test.js (right): https://codereview.chromium.org/166273018/diff/570001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:120: for (layerId in InspectorTest._layerTreeModel._layersById) { Nit: use forEachLayer()?
https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:76: static void buildRegionObjects(const blink::WebVector<blink::WebRect>& regions, Builders return objects: we have buildObjectFor and buildArrayFor that return object and array. This one is rather populateRects, but I'd simply do buildScrollRect(rect, type) and populate single list below using two for loops. (Region is a CSS term, you probably want a different name.) https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:79: const TypeBuilder::LayerTree::LayerId& layerId) Looks like this is unused. https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:101: RefPtr<TypeBuilder::DOM::Rect> rectObject = TypeBuilder::DOM::Rect::create() Then you would be able to reuse builder method here and would actually save on the number of lines. There is nothing wrong in building WebRect from the layer data for the sake of the call. https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:151: layerObject->setScrollRects(buildScrollRectsForLayer(graphicsLayer)); Why building twice, use scrollRects instead? Do you need a check at all? I thought we supported nullable arrays. https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:248: RefPtr<TypeBuilder::Array<TypeBuilder::LayerTree::Layer> >& layers) no need for new line here. https://codereview.chromium.org/166273018/diff/570001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/166273018/diff/570001/Source/devtools/protoco... Source/devtools/protocol.json:3839: "description": "Rectangle that should be scrolled on main thread.", Can you actually "scroll" a "rectangle"?
https://codereview.chromium.org/166273018/diff/570001/LayoutTests/http/tests/... File LayoutTests/http/tests/inspector/layers-test.js (right): https://codereview.chromium.org/166273018/diff/570001/LayoutTests/http/tests/... LayoutTests/http/tests/inspector/layers-test.js:120: for (layerId in InspectorTest._layerTreeModel._layersById) { On 2014/02/28 07:16:23, caseq wrote: > Nit: use forEachLayer()? Done. https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:76: static void buildRegionObjects(const blink::WebVector<blink::WebRect>& regions, On 2014/02/28 07:43:43, pfeldman wrote: > Builders return objects: we have buildObjectFor and buildArrayFor that return > object and array. This one is rather populateRects, but I'd simply do > buildScrollRect(rect, type) and populate single list below using two for loops. > > (Region is a CSS term, you probably want a different name.) Done. https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:79: const TypeBuilder::LayerTree::LayerId& layerId) On 2014/02/28 07:43:43, pfeldman wrote: > Looks like this is unused. Done. https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:101: RefPtr<TypeBuilder::DOM::Rect> rectObject = TypeBuilder::DOM::Rect::create() On 2014/02/28 07:43:43, pfeldman wrote: > Then you would be able to reuse builder method here and would actually save on > the number of lines. There is nothing wrong in building WebRect from the layer > data for the sake of the call. Done. https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:151: layerObject->setScrollRects(buildScrollRectsForLayer(graphicsLayer)); On 2014/02/28 07:43:43, pfeldman wrote: > Why building twice, use scrollRects instead? > > Do you need a check at all? I thought we supported nullable arrays. Done. https://codereview.chromium.org/166273018/diff/570001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:248: RefPtr<TypeBuilder::Array<TypeBuilder::LayerTree::Layer> >& layers) On 2014/02/28 07:43:43, pfeldman wrote: > no need for new line here. Done. https://codereview.chromium.org/166273018/diff/570001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/166273018/diff/570001/Source/devtools/protoco... Source/devtools/protocol.json:3839: "description": "Rectangle that should be scrolled on main thread.", On 2014/02/28 07:43:43, pfeldman wrote: > Can you actually "scroll" a "rectangle"? Done.
https://codereview.chromium.org/166273018/diff/590001/LayoutTests/inspector/l... File LayoutTests/inspector/layers/layer-scroll-rects-get.html (right): https://codereview.chromium.org/166273018/diff/590001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-get.html:16: WebInspector.showPanel("layers"); You don't need this line. https://codereview.chromium.org/166273018/diff/590001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-get.html:27: var element = document.getElementById('touchable'); 4 space indent https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:84: .setRect(rectObject) .release() https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:86: return scrollRectObject; return scrollRectObject.release(); https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:103: return scrollRects->length() ? scrollRects : nullptr; ditto https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:143: layerObject->setScrollRects(scrollRects); ditto https://codereview.chromium.org/166273018/diff/590001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/590001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:417: if (i >= this._scrollRects.length || What is going on here? Why aren't you rebuilding the scroll rects? https://codereview.chromium.org/166273018/diff/590001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/590001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:207: this._scrollRectElementsByLayerId[layerId].forEach(this._removeElement); Why aren't they removed along with the parent?
https://codereview.chromium.org/166273018/diff/590001/LayoutTests/inspector/l... File LayoutTests/inspector/layers/layer-scroll-rects-get.html (right): https://codereview.chromium.org/166273018/diff/590001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-get.html:16: WebInspector.showPanel("layers"); On 2014/03/04 09:45:59, pfeldman wrote: > You don't need this line. Done. https://codereview.chromium.org/166273018/diff/590001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-get.html:27: var element = document.getElementById('touchable'); On 2014/03/04 09:45:59, pfeldman wrote: > 4 space indent Done. https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:84: .setRect(rectObject) On 2014/03/04 09:45:59, pfeldman wrote: > .release() Done. https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:86: return scrollRectObject; On 2014/03/04 09:45:59, pfeldman wrote: > return scrollRectObject.release(); Done. https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:103: return scrollRects->length() ? scrollRects : nullptr; On 2014/03/04 09:45:59, pfeldman wrote: > ditto Done. https://codereview.chromium.org/166273018/diff/590001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:143: layerObject->setScrollRects(scrollRects); On 2014/03/04 09:45:59, pfeldman wrote: > ditto Done. https://codereview.chromium.org/166273018/diff/590001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/590001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:417: if (i >= this._scrollRects.length || On 2014/03/04 09:45:59, pfeldman wrote: > What is going on here? Why aren't you rebuilding the scroll rects? Done. https://codereview.chromium.org/166273018/diff/590001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/590001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:207: this._scrollRectElementsByLayerId[layerId].forEach(this._removeElement); On 2014/03/04 09:45:59, pfeldman wrote: > Why aren't they removed along with the parent? Done.
https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:425: if (rectsChanged) { no need for {} here. https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:53: this._scrollRectElementsByLayerId = {} unused? https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:168: var contentRoot = this._model.contentRoot(); Do we really have to rename it here? https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:233: _removeElement: function(element) looks like you can define it within the only remaining function that uses it now. https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:246: var style = element.style; unused? https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:247: var parentLayerId = layer.id(); inline https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:277: if (newScrollRects != layerDetails.scrollRects) { We use !== https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:517: if (paintRectElement) I think you can omit this check.
https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:425: if (rectsChanged) { On 2014/03/11 11:14:38, caseq wrote: > no need for {} here. Done. https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:53: this._scrollRectElementsByLayerId = {} On 2014/03/11 11:14:38, caseq wrote: > unused? Done. https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:168: var contentRoot = this._model.contentRoot(); On 2014/03/11 11:14:38, caseq wrote: > Do we really have to rename it here? Done. https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:233: _removeElement: function(element) On 2014/03/11 11:14:38, caseq wrote: > looks like you can define it within the only remaining function that uses it > now. Done. https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:246: var style = element.style; On 2014/03/11 11:14:38, caseq wrote: > unused? Done. https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:247: var parentLayerId = layer.id(); On 2014/03/11 11:14:38, caseq wrote: > inline Done. https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:277: if (newScrollRects != layerDetails.scrollRects) { On 2014/03/11 11:14:38, caseq wrote: > We use !== Done. https://codereview.chromium.org/166273018/diff/610001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:517: if (paintRectElement) On 2014/03/11 11:14:38, caseq wrote: > I think you can omit this check. Done.
https://codereview.chromium.org/166273018/diff/630001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/630001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:100: blink::WebRect* webRect = new blink::WebRect(webLayer->position().x, webLayer->position().y, webLayer->bounds().width, webLayer->bounds().height); that's a memory leak. Why do you need a dynamic allocation here? https://codereview.chromium.org/166273018/diff/630001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:141: buildScrollRectsForLayer(graphicsLayer); nit: please don't wrap here, it's well within reasonable line length. https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:247: _updateScrollRectElement: function(elements, scrollRect, index) I don't think using forEach() justifies method signatures like this. It rather makes sense to have something like _updateScrollRectElement: function(element, rect) https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:278: layerDetails.scrollRects.forEach(this._updateScrollRectElement.bind(this, layerDetails.scrollRectElements)); Why would we update if the rects did not change? https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:312: this._model.forEachLayer(this._updateScrollRectsForLayer.bind(this)); Can we rather call updateScrollRectsForLayer from updateLayer? https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:363: var parentElement = isContentRoot ? this._rotatingContainerElement : (isRoot ? this._elementForLayer(contentRoot) : this._elementForLayer(layer.parent())); Two ternary operators per statement is a bit of overkill, please expands to if/else
https://codereview.chromium.org/166273018/diff/630001/Source/core/inspector/I... File Source/core/inspector/InspectorLayerTreeAgent.cpp (right): https://codereview.chromium.org/166273018/diff/630001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:100: blink::WebRect* webRect = new blink::WebRect(webLayer->position().x, webLayer->position().y, webLayer->bounds().width, webLayer->bounds().height); On 2014/03/11 12:04:09, caseq wrote: > that's a memory leak. Why do you need a dynamic allocation here? Done. https://codereview.chromium.org/166273018/diff/630001/Source/core/inspector/I... Source/core/inspector/InspectorLayerTreeAgent.cpp:141: buildScrollRectsForLayer(graphicsLayer); On 2014/03/11 12:04:09, caseq wrote: > nit: please don't wrap here, it's well within reasonable line length. Done. https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:247: _updateScrollRectElement: function(elements, scrollRect, index) On 2014/03/11 12:04:09, caseq wrote: > I don't think using forEach() justifies method signatures like this. It rather > makes sense to have something like > > _updateScrollRectElement: function(element, rect) Done. https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:278: layerDetails.scrollRects.forEach(this._updateScrollRectElement.bind(this, layerDetails.scrollRectElements)); On 2014/03/11 12:04:09, caseq wrote: > Why would we update if the rects did not change? Done. https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:312: this._model.forEachLayer(this._updateScrollRectsForLayer.bind(this)); On 2014/03/11 12:04:09, caseq wrote: > Can we rather call updateScrollRectsForLayer from updateLayer? Done. https://codereview.chromium.org/166273018/diff/630001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:363: var parentElement = isContentRoot ? this._rotatingContainerElement : (isRoot ? this._elementForLayer(contentRoot) : this._elementForLayer(layer.parent())); On 2014/03/11 12:04:09, caseq wrote: > Two ternary operators per statement is a bit of overkill, please expands to > if/else Done.
https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:404: this._scrollRects = this._layerPayload.scrollRects || []; Looks like this is not worth extracting. https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:275: for (var i = 0; i < layerDetails.scrollRects.length; ++i) { nit: drop {} https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:364: parentElement = this._rotatingContainerElement terminate line with ; https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:365: } else { this should be structured as if (...) { ... } else if (...) { ... } else { ... } https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:367: parentElement = this._elementForLayer(contentRoot) ditto
https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... File Source/devtools/front_end/LayerTreeModel.js (right): https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/LayerTreeModel.js:404: this._scrollRects = this._layerPayload.scrollRects || []; On 2014/03/11 13:40:32, caseq wrote: > Looks like this is not worth extracting. Done. https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:275: for (var i = 0; i < layerDetails.scrollRects.length; ++i) { On 2014/03/11 13:40:32, caseq wrote: > nit: drop {} Done. https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:364: parentElement = this._rotatingContainerElement On 2014/03/11 13:40:32, caseq wrote: > terminate line with ; Done. https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:365: } else { On 2014/03/11 13:40:32, caseq wrote: > this should be structured as > if (...) { > ... > } else if (...) { > ... > } else { > ... > } Done. https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:367: parentElement = this._elementForLayer(contentRoot) On 2014/03/11 13:40:32, caseq wrote: > ditto Done.
https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/650001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:369: parentElement = this._elementForLayer(layer.parent()); nit: parentElement = this._elementForLayer(isRoot ? contentRoot : layer.parent());
https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:260: layerDetails = this._elementsByLayerId[layer.id()].__layerDetails; var layerDetails = ... https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:272: layerDetails.scrollRects = newScrollRects; Do we have to keep them now? https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:276: this._updateScrollRectElement(layerDetails.scrollRects[i], layerDetails.scrollRectElements[i]); newScrollRects? https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:320: if (layer.nodeId()) Can we either avoid this check or at least move it into updatePaintRect?
https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:260: layerDetails = this._elementsByLayerId[layer.id()].__layerDetails; On 2014/03/11 13:57:03, caseq wrote: > var layerDetails = ... Done. https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:272: layerDetails.scrollRects = newScrollRects; On 2014/03/11 13:57:03, caseq wrote: > Do we have to keep them now? Done. https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:276: this._updateScrollRectElement(layerDetails.scrollRects[i], layerDetails.scrollRectElements[i]); On 2014/03/11 13:57:03, caseq wrote: > newScrollRects? Done. https://codereview.chromium.org/166273018/diff/670001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:320: if (layer.nodeId()) On 2014/03/11 13:57:03, caseq wrote: > Can we either avoid this check or at least move it into updatePaintRect? Done.
lgtm https://codereview.chromium.org/166273018/diff/710001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/710001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:270: layerDetails.scrollRectElements.forEach(removeElement); I thought the whole point of this was to put them into an element that you can removeChildren from. https://codereview.chromium.org/166273018/diff/710001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:359: if (isContentRoot) { Drop {}
https://codereview.chromium.org/166273018/diff/710001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/710001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:270: layerDetails.scrollRectElements.forEach(removeElement); On 2014/03/11 16:55:00, pfeldman wrote: > I thought the whole point of this was to put them into an element that you can > removeChildren from. Done. https://codereview.chromium.org/166273018/diff/710001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:359: if (isContentRoot) { On 2014/03/11 16:55:00, pfeldman wrote: > Drop {} 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/166273018/730001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel
https://codereview.chromium.org/166273018/diff/730001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/730001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:335: if (layer.nodeId()) { The element for a layer with given id is only created once. So if we don't have a node associated with a layer with given id and then have one, this will fail, as you will create and keep using "transparent" element. This may, for example, happen during navigation, when new layers have ids that match the layers in the old page. We currently do not handle navigation specifically above the model layer.
https://codereview.chromium.org/166273018/diff/730001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/730001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:335: if (layer.nodeId()) { On 2014/03/19 13:54:10, caseq wrote: > The element for a layer with given id is only created once. So if we don't have > a node associated with a layer with given id and then have one, this will fail, > as you will create and keep using "transparent" element. > This may, for example, happen during navigation, when new layers have ids that > match the layers in the old page. > We currently do not handle navigation specifically above the model layer. Done.
https://codereview.chromium.org/166273018/diff/770001/LayoutTests/inspector/l... File LayoutTests/inspector/layers/layer-scroll-rects-update.html (right): https://codereview.chromium.org/166273018/diff/770001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-update.html:70: InspectorTest.addResult("Updated scroll rectangles"); Extract this block into a function? https://codereview.chromium.org/166273018/diff/770001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-update.html:80: InspectorTest.requestLayers(onGotLayers); Do we have to do this? https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:363: if (element.__layerDetails.depth !== undefined) Can we encapsulate this check into LayerDetails class, i.e. isAboveContentRoot: function() { return this.depth === undefined; } The depth calculation may be encapsulated as well. https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:374: if (element.__layerDetails.depth !== undefined) Is this necessary? https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:406: if (!element) Ditto. https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:510: * @param {!Element=} paintRectElement ditto https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... File Source/devtools/front_end/layersPanel.css (right): https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/layersPanel.css:142: .layer-transparent .paint-rect { Let's show paint rects!
https://codereview.chromium.org/166273018/diff/770001/LayoutTests/inspector/l... File LayoutTests/inspector/layers/layer-scroll-rects-update.html (right): https://codereview.chromium.org/166273018/diff/770001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-update.html:70: InspectorTest.addResult("Updated scroll rectangles"); On 2014/03/20 15:29:10, caseq wrote: > Extract this block into a function? Done. https://codereview.chromium.org/166273018/diff/770001/LayoutTests/inspector/l... LayoutTests/inspector/layers/layer-scroll-rects-update.html:80: InspectorTest.requestLayers(onGotLayers); On 2014/03/20 15:29:10, caseq wrote: > Do we have to do this? Done. https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:363: if (element.__layerDetails.depth !== undefined) On 2014/03/20 15:29:10, caseq wrote: > Can we encapsulate this check into LayerDetails class, i.e. > isAboveContentRoot: function() > { > return this.depth === undefined; > } > > The depth calculation may be encapsulated as well. Done. https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:374: if (element.__layerDetails.depth !== undefined) On 2014/03/20 15:29:10, caseq wrote: > Is this necessary? Done. https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:406: if (!element) On 2014/03/20 15:29:10, caseq wrote: > Ditto. Done. https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:510: * @param {!Element=} paintRectElement On 2014/03/20 15:29:10, caseq wrote: > ditto Done. https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... File Source/devtools/front_end/layersPanel.css (right): https://codereview.chromium.org/166273018/diff/770001/Source/devtools/front_e... Source/devtools/front_end/layersPanel.css:142: .layer-transparent .paint-rect { On 2014/03/20 15:29:10, caseq wrote: > Let's show paint rects! Done.
lgtm https://codereview.chromium.org/166273018/diff/780001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/780001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:361: element.__layerDetails.depth = parentElement.__layerDetails.depth === undefined ? undefined : parentElement.__layerDetails.depth + 1; use isAboveContentRoot here()?
https://codereview.chromium.org/166273018/diff/780001/Source/devtools/front_e... File Source/devtools/front_end/Layers3DView.js (right): https://codereview.chromium.org/166273018/diff/780001/Source/devtools/front_e... Source/devtools/front_end/Layers3DView.js:361: element.__layerDetails.depth = parentElement.__layerDetails.depth === undefined ? undefined : parentElement.__layerDetails.depth + 1; On 2014/03/21 08:36:38, caseq wrote: > use isAboveContentRoot here()? 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/166273018/800001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on mac_blink_compile_dbg
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/166273018/820001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
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/166273018/820001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
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/166273018/820001
Message was sent while issue was closed.
Change committed as 169735 |
