Display grid gaps in the CSS Grids highlighter

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Inspector
P2
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pbro, Assigned: gl)

Tracking

(Blocks: 1 bug)

49 Branch
Firefox 51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
(Reporter)

Comment 2

2 years ago
The new chrome-only API el.getGridFragments has landed in m-c and contains enough information to determine gaps in the grid.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1297506
(Assignee)

Updated

2 years ago
Assignee: nobody → gl
(Assignee)

Comment 4

2 years ago
Created attachment 8789916 [details] [diff] [review]
1282727.patch [1.0]
Attachment #8789916 - Flags: review?(pbrosset)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Blocks: 1302197
(Reporter)

Comment 5

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8789916 - Flags: review?(pbrosset)
(Assignee)

Comment 6

2 years ago
Created attachment 8791490 [details] [diff] [review]
1282727.patch [2.0]
Attachment #8791490 - Flags: review?(pbrosset)
(Assignee)

Updated

2 years ago
Attachment #8789916 - Attachment is obsolete: true
(Reporter)

Comment 7

2 years ago
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) {
  ...
}
Attachment #8791490 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8792105 [details] [diff] [review]
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.
Flags: needinfo?(pbrosset)
Attachment #8792105 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8791490 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0e0c46418b14
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Reporter)

Comment 11

2 years ago
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.
Flags: needinfo?(pbrosset)
You need to log in before you can comment on or make changes to this bug.