1409970 - Adds an initial FlexboxHighlighter boilerplate

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

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

Attachment: Bug 1409970 - Part 1: Refactor the canvas position utility code to a separate file.

Review commit: https://reviewboard.mozilla.org/r/190988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190988/

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8920012 [details]
Bug 1409970 - Part 1: Refactor the canvas position utility code to a separate file.

https://reviewboard.mozilla.org/r/190988/#review196368

::: devtools/server/actors/highlighters/flexbox.js:26
(Diff revision 1)
> +  isIdentity,
> +  getNodeTransformationMatrix,
> +} = require("devtools/shared/layout/dom-matrix-2d");
> +
> +// We create a <canvas> element that has always 4096x4096 physical pixels, to displays
> +// our grid's overlay.

s/grid/flexbox

::: devtools/server/actors/highlighters/flexbox.js:86
(Diff revision 1)
> +    // We use a <canvas> element so that we can draw an arbitrary number of lines
> +    // which wouldn't be possible with HTML or SVG without having to insert and remove
> +    // the whole markup on every update.

What do you think the highlighter will look like in the future?
If you already have a clear idea of what this is going to be, then please let me know, so I can think about whether canvas really is the right idea here or not.

::: devtools/server/actors/highlighters/flexbox.js:113
(Diff revision 1)
> +   * viewport's size.
> +   * It's called when a page's scroll is detected.
> +   *
> +   * @return {Boolean} `true` if the <canvas> position was updated, `false` otherwise.
> +   */
> +  calculateCanvasPosition() {

This, and other parts of this class are copied from css-grid.js.
In fact, the whole virtual canvas logic has been duplicated here. 
This is particularly complex code, so if we need to maintain it, we really must have it in just one place, not 2.

Can you work on a way to move this code to a central place?
Either having helper functions in a utils module somewhere, or creating a parent class that css-grid and flexbox would both inherit from? I would vote for the latter.

Another way would be to use composition instead of inheritance, this way the flexbox highlighter could use composition to add the autorefresh *and* virtual canvas behavior in a nice way. But that's a lot of changes.

Comment 3

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

Attachment: Bug 1409970 - Part 2: Adds an initial FlexboxHighlighter boilerplate.

Review commit: https://reviewboard.mozilla.org/r/192348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192348/

Comment 4

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

Comment on attachment 8920012 [details]
Bug 1409970 - Part 1: Refactor the canvas position utility code to a separate file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190988/diff/1-2/

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8920012 [details]
Bug 1409970 - Part 1: Refactor the canvas position utility code to a separate file.

https://reviewboard.mozilla.org/r/190988/#review197568

Looks good thanks. Please updates the comments as listed below.

::: devtools/server/actors/highlighters/css-grid.js:81
(Diff revision 2)
>  // We create a <canvas> element that has always 4096x4096 physical pixels, to displays
>  // our grid's overlay.
>  // Then, we move the element around when needed, to give the perception that it always
>  // covers the screen (See bug 1345434).
>  //
>  // This canvas size value is the safest we can use because most GPUs can handle it.
>  // It's also far from the maximum canvas memory allocation limit (4096x4096x4 is
>  // 67.108.864 bytes, where the limit is 500.000.000 bytes, see:
>  // http://searchfox.org/mozilla-central/source/gfx/thebes/gfxPrefs.h#401).
>  //
>  // Note:
>  // Once bug 1232491 lands, we could try to refactor this code to use the values from
>  // the displayport API instead.
>  //
>  // Using a fixed value should also solve bug 1348293.
>  const CANVAS_SIZE = 4096;

Is this whole comment and const still needed in this file now that many functions have moved somewhere else?
If we still need to access this value here, then better export it out of canvas.js instead. And remove this whole comment since it's now duplicated.

::: devtools/server/actors/highlighters/utils/canvas.js:20
(Diff revision 2)
> +// We create a <canvas> element that has always 4096x4096 physical pixels, to displays
> +// our grid's overlay.
> +// Then, we move the element around when needed, to give the perception that it always
> +// covers the screen (See bug 1345434).

This comment used to make sense when it was in css-grid.js because we knew we were creating a canvas for rendering the grid lines.
Here we need to change it a little so people opening this util file will know what it's for.
I suggest adding a preliminary comment before that:

// A set of utility functions for highlighters that render their content to a <canvas> element.

::: devtools/server/actors/highlighters/utils/canvas.js:406
(Diff revision 2)
> +  });
> +}
> +
> +/**
> + * Updates the <canvas> element's style in accordance with the current window's
> + * devicePixelRatio, and the position calculated in `calculateCanvasPosition`; it also

calculateCanvasPosition does not exist anymore, please update this comment to the right function name.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8921344 [details]
Bug 1409970 - Part 2: Adds an initial FlexboxHighlighter boilerplate.

https://reviewboard.mozilla.org/r/192348/#review197570

::: devtools/server/actors/highlighters/flexbox.js:21
(Diff revision 1)
> +// We create a <canvas> element that has always 4096x4096 physical pixels, to displays
> +// our flexbox's overlay.
> +// Then, we move the element around when needed, to give the perception that it always
> +// covers the screen (See bug 1345434).
> +//
> +// This canvas size value is the safest we can use because most GPUs can handle it.
> +// It's also far from the maximum canvas memory allocation limit (4096x4096x4 is
> +// 67.108.864 bytes, where the limit is 500.000.000 bytes, see:
> +// http://searchfox.org/mozilla-central/source/gfx/thebes/gfxPrefs.h#401).
> +//
> +// Note:
> +// Once bug 1232491 lands, we could try to refactor this code to use the values from
> +// the displayport API instead.
> +//
> +// Using a fixed value should also solve bug 1348293.
> +const CANVAS_SIZE = 4096;

Ok, so you do need the CANVAS_SIZE in this file (and in css-grid.js). So please, instead of duplicating it and duplicating this long comment, add an `export.CANVAS_SIZE` inside the new utils/canvas.js file and get it from there.

::: devtools/server/actors/highlighters/flexbox.js:85
(Diff revision 1)
> +    // We use a <canvas> element so that we can draw an arbitrary number of lines
> +    // which wouldn't be possible with HTML or SVG without having to insert and remove
> +    // the whole markup on every update.

This was copied from the grid highlighter function. Please update to make it more specific to the flexbox highlighter.
In particular, it should say that we use a <canvas> because there is an arbitrary number of items to draw, and potential text to draw, etc.

::: devtools/server/actors/highlighters/flexbox.js:173
(Diff revision 1)
> +      this.hide();
> +    }
> +  }
> +
> +  /**
> +   * Called when the page will-navigate. Used to hide the grid highlighter and clear

s/grid/flexbox

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/523ff288f14d
Part 1: Refactor the canvas position utility code to a separate file. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/da9f271dcedd
Part 2: Adds an initial FlexboxHighlighter boilerplate. r=pbro

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/523ff288f14d
https://hg.mozilla.org/mozilla-central/rev/da9f271dcedd