Comment on attachment 8933867 [details] Bug 1422470 - Display flex container differently from flex items https://reviewboard.mozilla.org/r/204802/#review210900 The flexbox highlighter actually breaks when navigating and re-enabling the flexbox highlighter. You can see an error for can't access dead objects, which is a result from navigation. We need to clear the cache on navigation. See https://searchfox.org/mozilla-central/source/devtools/server/actors/highlighters/css-grid.js#399 ::: devtools/server/actors/highlighters/flexbox.js:42 (Diff revision 1) > > +// px > +const FLEXBOX_GAP_PATTERN_WIDTH = 14; > +const FLEXBOX_GAP_PATTERN_HEIGHT = 14; > +const FLEXBOX_GAP_PATTERN_LINE_DISH = [5, 3]; > +const FLEXBOX_GAP_PATTERN_STROKE_STYLE = "#9370DB"; We don't need this because we have DEFAULT_COLOR ::: devtools/server/actors/highlighters/flexbox.js:50 (Diff revision 1) > + * Cached used by `FlexboxHighlighter.getFlexboxGapPattern`. > + * @type {Map} > + */ > +const gCachedFlexboxPattern = new Map(); > + > +const COLUMNS = "cols"; This constant doesn't make sense in this context for flexbox. Just have a generic constant like const FLEXBOX = "flexbox" ::: devtools/server/actors/highlighters/flexbox.js:145 (Diff revision 1) > + * > + * @param {String} dimensionType > + * The grid dimension type which is either the constant COLUMNS or ROWS. > + * @return {CanvasPattern} grid gap pattern. > + */ > + getFlexboxGapPattern(dimensionType) { We should stop saying grid gap/gap pattern since this isn't related to flexbox. I would rename this to getFlexboxContainerPattern, and we name the JSDocs appropriately. ::: devtools/server/actors/highlighters/flexbox.js:190 (Diff revision 1) > + * The end position of the cross side of the grid line. > + * @param {Number} breadth > + * The grid line breadth value. > + */ > + > + clearGridGap(linePos, startPos, endPos, breadth) { This function name should be clearFlexItem since we are basically rendering a clear rectangle around the flex item. We should also rename the JSDoc to make sure it is appropriate for this context. ::: devtools/server/actors/highlighters/flexbox.js (Diff revision 1) > for (let flexItem of flexItems) { > let quads = getAdjustedQuads(this.win, flexItem, "content"); > if (!quads.length) { > continue; > } > - Don't remove this new line.

Comment on attachment 8933867 [details] Bug 1422470 - Display flex container differently from flex items https://reviewboard.mozilla.org/r/204802/#review211756

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/affa7f97ff91 Display flex container differently from flex items r=gl

Backed out for ESlint failure /builds/worker/checkouts/gecko/devtools/server/actors/highlighters/flexbox.js:14:3 r=backout a=backout on a CLOSED TREE Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=affa7f97ff916198343f2ead9a5c3362efd70c20 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=150433577&repo=mozilla-inbound Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb27c8569d7e94e2fb215d17acf9ca8fb2e4903

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c35ad676551a Display flex container differently from flex items r=gl