Comment on attachment 8944076 [details] [diff] [review] 1431841.patch Review of attachment 8944076 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/highlighters/flexbox.js @@ +31,1 @@ > alpha: 1, Global alpha actually defaults to 1. So, I think we can actually remove all the alpha and all the globalAlpha that we set. @@ +31,3 @@ > alpha: 1, > }, > + "line": { This won't be needed because of the above. @@ +288,5 @@ > this.ctx.stroke(); > this.ctx.restore(); > } > > + renderFlexContainerBorder() { Move this function above renderFlexContainerFill. We actually order these functions alphabetically. @@ +317,5 @@ > + > + renderFlexLines() { > + if (!this.currentQuads.content || !this.currentQuads.content[0]) { > + return; > + } Add a new line after if statement blocks @@ +319,5 @@ > + if (!this.currentQuads.content || !this.currentQuads.content[0]) { > + return; > + } > + let container = this.currentNode; > + let containerStyle = getComputedStyle(container); s/containerStyle/computedStyle Just use this.currentNode instead of assigning it to "container. @@ +321,5 @@ > + } > + let container = this.currentNode; > + let containerStyle = getComputedStyle(container); > + let flexLines = container.getAsFlexContainer().getLines(); > + if(!flexLines) { Add a space after "if" @@ +339,5 @@ > + this.ctx.lineWidth = lineWidth; > + this.ctx.strokeStyle = DEFAULT_COLOR; > + > + let { bounds } = this.currentQuads.content[0]; > + let flexItems = this.currentNode.children; This is never used. @@ +341,5 @@ > + > + let { bounds } = this.currentQuads.content[0]; > + let flexItems = this.currentNode.children; > + > + if (containerStyle.getPropertyValue("flex-direction") === "column" || containerStyle.getPropertyValue("flex-direction") === "column-reverse") { This is eslint error since it runs over 90 characters in a line. @@ +342,5 @@ > + let { bounds } = this.currentQuads.content[0]; > + let flexItems = this.currentNode.children; > + > + if (containerStyle.getPropertyValue("flex-direction") === "column" || containerStyle.getPropertyValue("flex-direction") === "column-reverse") { > + for (let flexLineItem of flexLines) { s/flexLineItem/flexLine @@ +343,5 @@ > + let flexItems = this.currentNode.children; > + > + if (containerStyle.getPropertyValue("flex-direction") === "column" || containerStyle.getPropertyValue("flex-direction") === "column-reverse") { > + for (let flexLineItem of flexLines) { > + clearRect(this.ctx, flexLineItem.crossStart, 0, flexLineItem.crossStart + flexLineItem.crossSize, bounds.height, this.currentMatrix); We can destructure flexLine and get the crossStart and crossSize. const { crossStart, crossSize } = flexLine; @@ +346,5 @@ > + for (let flexLineItem of flexLines) { > + clearRect(this.ctx, flexLineItem.crossStart, 0, flexLineItem.crossStart + flexLineItem.crossSize, bounds.height, this.currentMatrix); > + > + drawRect(this.ctx, flexLineItem.crossStart, 0, flexLineItem.crossStart, bounds.height, this.currentMatrix); > + this.ctx.stroke(); I am wondering if we can just call stroke once. Double check for me. @@ +351,5 @@ > + drawRect(this.ctx, flexLineItem.crossStart + flexLineItem.crossSize, 0, flexLineItem.crossStart + flexLineItem.crossSize, bounds.height, this.currentMatrix); > + this.ctx.stroke(); > + } > + } else { > + for (let flexLineItem of flexLines) { We can probably just check the computedStyle.getPropertyValue("flex-direction") inside the for loop so we can just have one for loop for (let flexLine of flexLines) { if (column) { .... } else { .... } } @@ +355,5 @@ > + for (let flexLineItem of flexLines) { > + clearRect(this.ctx, 0,flexLineItem.crossStart, bounds.width, flexLineItem.crossStart + flexLineItem.crossSize, this.currentMatrix); > + > + drawRect(this.ctx, 0, flexLineItem.crossStart, bounds.width, flexLineItem.crossStart, this.currentMatrix); > + this.ctx.stroke(); Probably can just call stroke once() as above. @@ +363,5 @@ > + } > + > + this.ctx.restore(); > + > + console.log(flexLines) Remove this console.log

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/82427fc508cd Show the flex lines in the flexbox highlighter. r=gl

Note to sheriff, I think the eslint errors in Bug 1431941.