Comment on attachment 8944288 [details] [diff] [review] 1432035.patch Review of attachment 8944288 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/highlighters/flexbox.js @@ +258,5 @@ > if (isTopLevel) { > this.hide(); > } > } > + renderFlexAlignItems() { Rename this to renderAlignItemLine @@ +263,5 @@ > + if (!this.currentQuads.content || !this.currentQuads.content[0]) { > + return; > + } > + > + let computedStyle = getComputedStyle(this.currentNode); Move the declaration of computedStyle and flexLines below the bounds assignment in line 283. @@ +287,5 @@ > + > + switch (computedStyle.getPropertyValue("align-items")) { > + case "flex-start": > + case "start": > + if (computedStyle.getPropertyValue("flex-direction") === "column" || Maybe we can simplify the check for "column" and "column-reverse" by using computedStyle.getPropertyValue("flex-direction").startsWith("column") @@ +295,5 @@ > + this.ctx.stroke(); > + } else { > + drawRect(this.ctx, 0, crossStart, bounds.width, crossStart, > + this.currentMatrix); > + this.ctx.stroke(); We can simply move this.ctx.stroke() outside the if statement so we can just call it once. @@ +301,5 @@ > + > + break; > + case "flex-end": > + case "end": > + if (computedStyle.getPropertyValue("flex-direction") === "column" || computedStyle.getPropertyValue("flex-direction").startsWith("column") @@ +305,5 @@ > + if (computedStyle.getPropertyValue("flex-direction") === "column" || > + computedStyle.getPropertyValue("flex-direction") === "column-reverse") { > + drawRect(this.ctx, crossStart + crossSize, 0, crossStart + crossSize, > + bounds.height, this.currentMatrix); > + this.ctx.stroke(); Move this.ctx.stroke() out. @@ +315,5 @@ > + > + break; > + case "center": > + case "normal": > + if (computedStyle.getPropertyValue("flex-direction") === "column" || computedStyle.getPropertyValue("flex-direction").startsWith("column") @@ +323,5 @@ > + this.ctx.stroke(); > + } else { > + drawRect(this.ctx, 0, crossStart + crossSize / 2, bounds.width, > + crossStart + crossSize / 2, this.currentMatrix); > + this.ctx.stroke(); Move this.ctx.stroke() out.

I did not apply the if statement changes since the bug 1432036 does not fit the change.

Comment on attachment 8946102 [details] [diff] [review] 1432035.patch Review of attachment 8946102 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/highlighters/flexbox.js @@ +248,5 @@ > * the cached gap patterns and avoid using DeadWrapper obejcts as gap patterns the > * next time. > */ > onWillNavigate({ isTopLevel }) { > this.clearCache(); Don't remove this line @@ +279,5 @@ > + > + for (let flexLine of flexLines) { > + let { crossStart, crossSize } = flexLine; > + > + switch (computedStyle.getPropertyValue("align-items")) { We can save the align-items property value in a variable after computedStyle to avoid getting this multiple times. @@ +282,5 @@ > + > + switch (computedStyle.getPropertyValue("align-items")) { > + case "flex-start": > + case "start": > + if (computedStyle.getPropertyValue("flex-direction").startsWith("column")) { Same as above we can add a isColumn variable @@ +330,5 @@ > > renderFlexContainerBorder() { > if (!this.currentQuads.content || !this.currentQuads.content[0]) { > return; > } Don't remove this line @@ +512,5 @@ > // Update the current matrix used in our canvas' rendering > let { currentMatrix, hasNodeTransformations } = getCurrentMatrix(this.currentNode, > this.win); > this.currentMatrix = currentMatrix; > this.hasNodeTransformations = hasNodeTransformations; Don't remove this line @@ +522,1 @@ > this._showFlexbox(); Don't remove this line

Comment on attachment 8949231 [details] [diff] [review] 1432035.patch Review of attachment 8949231 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/highlighters/flexbox.js @@ +270,5 @@ > + let canvasY = Math.round(this._canvasPosition.y * devicePixelRatio); > + > + this.ctx.save(); > + this.ctx.translate(offset - canvasX, offset - canvasY); > + this.ctx.setLineDash(FLEXBOX_LINES_PROPERTIES.alignItems.lineDash); FLEXBOX_LINES_PROPERTIES.alignItems doesn't exist. So, the patch doesn't actually work. @@ +283,5 @@ > + for (let flexLine of flexLines) { > + let { crossStart, crossSize } = flexLine; > + > + switch (alignItemsType) { > + case "flex-start": We need to handle all the align items type. See https://developer.mozilla.org/en-US/docs/Web/CSS/align-items. @@ +313,5 @@ > + case "center": > + case "normal": > + if (isColumn) { > + drawRect(this.ctx, crossStart + crossSize / 2, 0, > + crossStart + crossSize / 2, bounds.height, this.currentMatrix); Indent this @@ +317,5 @@ > + crossStart + crossSize / 2, bounds.height, this.currentMatrix); > + this.ctx.stroke(); > + } else { > + drawRect(this.ctx, 0, crossStart + crossSize / 2, bounds.width, > + crossStart + crossSize / 2, this.currentMatrix); Indent this

Comment on attachment 8949633 [details] [diff] [review] 1432035.patch Looks like this patch is for the wrong bug. Clearing the review

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a2139264a3 Visualize the align-items in the flexbox highlighter. r=gl