1282727 - Display grid gaps in the CSS Grids highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

In the CSS Grids highlighter we're working on in bug 1282726, it would be useful to display grid gaps differently than tracks, maybe greyed-out, maybe with a pattern background.

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[Patrick Brosset <:pbro>]

The new chrome-only API el.getGridFragments has landed in m-c and contains enough information to determine gaps in the grid.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1282727.patch [1.0]

Comment 5

[Patrick Brosset <:pbro>]

I haven't had the time to review this yet, but did look quickly at the code, and I would like to suggest caching a canvas fill pattern like we do here:
http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/Graphs.js#1273-1308
This might be useful for performance.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1282727.patch [2.0]

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8791490 [details] [diff] [review]
1282727.patch [2.0]

Review of attachment 8791490 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/highlighters/css-grid.js
@@ +38,5 @@
>  
> +// px
> +const GRID_GAP_PATTERN_WIDTH = 14;
> +const GRID_GAP_PATTERN_HEIGHT = 14;
> +const GRID_GAP_PATTERN_LINE_DISH = [5, 3];

GRID_GAP_PATTERN_LINE_DASH instead of GRID_GAP_PATTERN_LINE_DISH ?

@@ +45,5 @@
> +/**
> + * Cached used by `CssGridHighlighter.getGridGapPattern`.
> + * @type {Map}
> + */
> +const gCachedGridPattern = new Map();

This cache is never cleared. So to avoid memory leaks (and because you don't need to iterate over the map), you can use a WeakMap instead.

@@ +226,5 @@
> +   * @param  {String} dimensionType
> +   *         The grid dimension type which is either the constant COLUMNS or ROWS.
> +   * @return {CanvasPattern} grid gap pattern.
> +   */
> +  getGridGapPattern(dimensionType) {

This function doesn't need to be in the class' prototype, it doesn't need to access `this`, so please move it outside the class, as a simple helper function. And in fact you could move the gCachedGridPattern variable next to it:

const gCachedGridPattern = new WeakMap();
function getGridGapPattern(dimensionType, win) {
  ...
}

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1282727.patch [3.0]

(In reply to Patrick Brosset <:pbro> from comment #7)
> Comment on attachment 8791490 [details] [diff] [review]
> 1282727.patch [2.0]
> 
> Review of attachment 8791490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/server/actors/highlighters/css-grid.js
> @@ +38,5 @@
> >  
> > +// px
> > +const GRID_GAP_PATTERN_WIDTH = 14;
> > +const GRID_GAP_PATTERN_HEIGHT = 14;
> > +const GRID_GAP_PATTERN_LINE_DISH = [5, 3];
> 
> GRID_GAP_PATTERN_LINE_DASH instead of GRID_GAP_PATTERN_LINE_DISH ?
> 
> @@ +45,5 @@
> > +/**
> > + * Cached used by `CssGridHighlighter.getGridGapPattern`.
> > + * @type {Map}
> > + */
> > +const gCachedGridPattern = new Map();
> 
> This cache is never cleared. So to avoid memory leaks (and because you don't
> need to iterate over the map), you can use a WeakMap instead.
> 

I ran into the issue where WeakMap takes an object as the key, and it doesn't seem to hit the cache unless we store the object. So, I kept with using a Map(), but added a clear() in destroy().

> @@ +226,5 @@
> > +   * @param  {String} dimensionType
> > +   *         The grid dimension type which is either the constant COLUMNS or ROWS.
> > +   * @return {CanvasPattern} grid gap pattern.
> > +   */
> > +  getGridGapPattern(dimensionType) {
> 
> This function doesn't need to be in the class' prototype, it doesn't need to
> access `this`, so please move it outside the class, as a simple helper
> function. And in fact you could move the gCachedGridPattern variable next to
> it:
> 
> const gCachedGridPattern = new WeakMap();
> function getGridGapPattern(dimensionType, win) {
>   ...
> }

We actually do use 'this.win' in createNode. I decided to keep it instead the class' prototype instead of moving it out into a simple helper function and passing in the window since there shouldn't be any other outside callers to this function. 

Patrick, I am needinfo'ing you to inform you of the the decisions I made to your suggested changes, but will pushing it forward.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/0e0c46418b14a6ac0cfb1f1e6efbc0feac8c2ab7
Bug 1282727 - Display grid gaps in the CSS Grids highlighter r=pbro

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0e0c46418b14

Comment 11

[Patrick Brosset <:pbro>]

In reply to comment 8:

- About gCachedGridPattern: you're storing patterns globally on the module scope, which means that if we instantiate many grid highlighters, they will reuse the same pattern, which is good for performance. But then you clear it in the class' destroy method. So there's a mismatch of scopes here. I mean, it will work, but if one instance is destroyed before the other one, then the second one will have to re-create the patterns.
Note that instantiating several grid highlighters is not just an edge case IMHO, the Layout panel will allow to highlight several grids at once. And nesting grids is useful for more complex layouts.
So the WeakMap was very useful in that it didn't require clearing.

- About the getGridGapPattern function: I'm fine with keeping it in the prototype if you prefer this. The reason I asked to move it out isn't because I expect outside callers of this function to ever exist, but rather because it's a helper function that requires no knowledge of the instance (apart from this.win as you said, but that can easily be passed as a parameter, it's just a reference to a window, not something that represents data about the instance). I find it easier to read through small helper functions that have one input and one output, rather than collections of class methods that sometimes have nothing to do together and that might have side effects on things stored on `this`. It also makes the class' responsibility sometimes harder to understand.