1252955 - Treemap visualization for review

Status: RESOLVED DUPLICATE of bug 1238695

(DevTools :: Memory, defect, P1)

Description

[Greg Tatum [:gregtatum]]

This is a bug for reviewing the treemap visualization code from here: https://github.com/TatumCreative/memory-treemap before migrating it into the devtools codebase.

Comment 1

[Greg Tatum [:gregtatum]]

Attachment: Treemap visualization for review

The attached patch is for review only, before actually migrating it into the Firefox
codebase. It was developed in an outside repo.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: treemapscroll.gif

One thing that immediately jumped out at me was that scroll-zooming is a little funky. I would expect it to keep zooming in to where my mouse is, and at first it does that, but after zooming in a little bit, if I move my mouse and continue zooming in, then the viewport flies past where my mouse was.

See the attached gif.

I think we could play with the color pallete as well, but that is definitely not something that needs to block the initial landing, and I suspect that Helen has opinions there.

Comment 3

[Greg Tatum [:gregtatum]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #2)

> One thing that immediately jumped out at me was that scroll-zooming is a
> little funky. I would expect it to keep zooming in to where my mouse is, and
> at first it does that, but after zooming in a little bit, if I move my mouse
> and continue zooming in, then the viewport flies past where my mouse was.

Ah, so looking at it the scroll works as intended when you are fully zoomed out, then scroll and keep the mouse in the same place. Once you move the mouse after you've zoomed in is where it appears to start behaving poorly. The math is a little funky with the way I've done things with CSS transforms. I'll see if I can figure that out.

Comment 4

[Victor Porof [:vporof][:vp]]

Comment on attachment 8725787 [details] [diff] [review]
Treemap visualization for review

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

Needs another pass and I would love to see some tests. Very nice overall.

::: devtools/client/memory/components/treemap/canvases.js
@@ +1,1 @@
> +/**

Should think about the licensing for all these files. Maybe just copy paste the boilerplate comment from the other files in the devtools codebase. Same for all files in this lib. Nit: might want to "use strict" at the top too.

Nit: canvas-utils might be a better name for this file.

@@ +7,5 @@
> +const DEBOUNCE_RATE = 100;
> +const FULLSCREEN_STYLE = {
> +  width: '100%',
> +  height: '100%',
> +  position: 'absolute',

Why do we need absolute positioning? Shouldn't rely on this for canvas dimensions, but rather make sure that the width and height percentages are properly calculated based on parent elements (which seems to be the case anyway). AFAICT we don't expect other nodes to be canvas siblings, so layout won't be affected anyway.

Later edit: oh I see that there are two canvases, one for zoomed in portions, one for full-res renderings. Might be worthwhile explaining all of this in a comment.

@@ +8,5 @@
> +const FULLSCREEN_STYLE = {
> +  width: '100%',
> +  height: '100%',
> +  position: 'absolute',
> +};

What's the benefit of inlining this css as opposed to keeping it in a shared styles file?

@@ +18,5 @@
> + * @param  {Window} window
> + * @return {HTMLDivElement}
> + */
> +function createContainingDiv (window) {
> +  let div = window.document.createElement('div');

Should use createElementNS everywhere since the passed in window might be from a XUL document. XUL canvases are quite different than HTML canvases.

@@ +19,5 @@
> + * @return {HTMLDivElement}
> + */
> +function createContainingDiv (window) {
> +  let div = window.document.createElement('div');
> +  Object.assign(div.style, FULLSCREEN_STYLE);

Why get the styling from a global vs. just passing it in as a param? Or, as I asked above, why not simply define these styles in a css file somewhere?

@@ +33,5 @@
> + * @param  {HTMLDivElement} div
> + * @param  {String} className
> + * @return {Object} { canvas, ctx }
> + */
> +function createCanvas(window, div, className) {

No need to pass in a window here right? We could just `.ownerDocument.defaultView` on the div.

@@ +37,5 @@
> +function createCanvas(window, div, className) {
> +  let canvas = document.createElement('canvas');
> +  div.appendChild(canvas);
> +  canvas.width = window.innerWidth * window.devicePixelRatio;
> +  canvas.height = window.innerHeight * window.devicePixelRatio;

What if the div is smaller than it's window? Isn't it safe to assume it isn't? Maybe it'd be better to getBoundingClientRect on the div. Or better yet, pass in the width and height as params.

There's functions that do similar things in widgets/Graphs.js. Should consolidate everything in a followup.

@@ +44,5 @@
> +  Object.assign(canvas.style, FULLSCREEN_STYLE, {
> +    pointerEvents: 'none'
> +  });
> +
> +  const ctx = canvas.getContext('2d');

Why `let` in some cases and `const` in others?

@@ +57,5 @@
> + * @param  {Window} window
> + * @param  {Object} canvases
> + */
> +function handleResizes (window, canvases) {
> +  function resize() {

Nit: this event listener seems to never get removed. Probably not a big deal.

@@ +59,5 @@
> + */
> +function handleResizes (window, canvases) {
> +  function resize() {
> +    let width = window.innerWidth * window.devicePixelRatio;
> +    let height = window.innerHeight * window.devicePixelRatio;

Ditto here on the dimensions. Probably not safe to assume container sizes.

Furthermore, if this visualization won't live in its own iframe, then the innermost window's widths and heights would be larger than what's actually displayed (think of the memory tool's sidebar, toolbar etc.). Should handle all of this in a better way. Maybe by getBoundingClientRect-ing.

@@ +64,5 @@
> +
> +    canvases.main.canvas.width = width;
> +    canvases.main.canvas.height = height;
> +    canvases.zoom.canvas.width = width;
> +    canvases.zoom.canvas.height = height;

Why not just pass in the new width and height to the onresize callback, and let consumers do whatever they deem appropriate.

@@ +66,5 @@
> +    canvases.main.canvas.height = height;
> +    canvases.zoom.canvas.width = width;
> +    canvases.zoom.canvas.height = height;
> +
> +    canvases.onResize();

Wouldn't it better to have `canvases` an event emitter, rather than calling a method directly on it? Or, possibly an issue: it seems that it both serves as a data store and a controller of sorts. Might be a bit clunky.

@@ +93,5 @@
> +    container,
> +    main,
> +    zoom,
> +    onResize: function noop() {}
> +  };

I'd make this a constructor and an event emitter.

::: devtools/client/memory/components/treemap/colorCoarseType.js
@@ +1,4 @@
> +/**
> + * Color the boxes in the treemap
> + */
> + 

Nit: trailing whitespace.

@@ +20,5 @@
> + *         index
> + */
> +function findCoarseTypeIndex (node) {
> +  let index = TYPES.indexOf(node.name);
> +  if(node.parent) {

Nit: space after `if`. Here and everywhere else.

@@ +23,5 @@
> +  let index = TYPES.indexOf(node.name);
> +  if(node.parent) {
> +    return index === -1 ? findCoarseTypeIndex(node.parent) : index;
> +  } else {
> +    return TYPES.indexOf("other");

Are root nodes always supposed to be `other`? Why not just return the index in this case as well, if it's not -1?

@@ +46,5 @@
> + * @param  {Object} node
> + * @return {Number}
> + */
> +function depthColor (node) {
> +  return Math.min(1, node.depth / DEPTH_FACTOR);

Well, this doesn't actually return a color value, does it? It returns a number that is used to compute lightness.

@@ +56,5 @@
> + * @param  {Object} node
> + * @return {Number}
> + */
> +function typeColor (node) {
> +  return findCoarseTypeIndex(node) / TYPE_FACTOR;

Ditto here, for hue. Maybe just be more specific in the comment or function names.

::: devtools/client/memory/components/treemap/dragZoom.js
@@ +26,5 @@
> + *
> + * @param {Window} window
> + * @param {HTMLElement} container
> + * @param {Object} dragZoom
> + *        The values that represent the current dragZoom state

I'd love to see a description here of how `dragZoom`s properties look like, or what they do. This param is a bit opaque without knowing how everything works. Either that, or create a constructor somewhere else with descriptions of what it's supposed to be used for.

@@ +32,5 @@
> +function createUpdateLoop (window, container, dragZoom) {
> +  let isLooping = false;
> +
> +  function update () {
> +    let scrollChanging = Math.abs(dragZoom.smoothZoom - dragZoom.zoom) > ZOOM_EPSILON;

Hard to immediately understand what `smoothZoom` and `zoom` are supposed to be.

@@ +55,5 @@
> +      dragZoom.smoothTranslateY = dragZoom.translateY;
> +    }
> +
> +    let zoom = 1 + dragZoom.smoothZoom;
> +    container.style.transform = `translate(${dragZoom.smoothTranslateX}px, ${dragZoom.smoothTranslateY}px) scale(${zoom})`;

Very nice.

@@ +76,5 @@
> + *
> + * @param  {Window} window
> + * @param  {Object} dragZoom
> + */
> +function keepInView (window, dragZoom) {

This mutates `dragZoom`. Might want to specify this in the docs.

@@ +78,5 @@
> + * @param  {Object} dragZoom
> + */
> +function keepInView (window, dragZoom) {
> +  let overdrawX = (dragZoom.width - window.innerWidth) / 2;
> +  let overdrawY = (dragZoom.height - window.innerHeight) / 2;

I'd assume that the view is defined by the canvas boundaries, not the window. Seems like a bad idea to me to assume otherwise.

@@ +84,5 @@
> +  dragZoom.translateX = Math.max(-overdrawX, Math.min(overdrawX, dragZoom.translateX));
> +  dragZoom.translateY = Math.max(-overdrawY, Math.min(overdrawY, dragZoom.translateY));
> +
> +  dragZoom.offsetX = dragZoom.width - window.innerWidth - dragZoom.translateX * 2;
> +  dragZoom.offsetY = dragZoom.height - window.innerHeight - dragZoom.translateY * 2;

Ditto for `window`. Something about passing `window` everywhere seems gross.

@@ +110,5 @@
> +    container.style.cursor = 'grab';
> +  }
> +
> +  function drag (event) {
> +    event.preventDefault()

Why preventDefault? What do we expect to happen and want to suppress? Might want to comment this here.

@@ +116,5 @@
> +    let prevMouseX = dragZoom.mouseX;
> +    let prevMouseY = dragZoom.mouseY;
> +
> +    dragZoom.mouseX = event.clientX;
> +    dragZoom.mouseY = event.clientY;

This dragging behavior might not handle the case where dragging *outside* the event container's bounds. This can easily happen inside a small toolbox.

@@ +132,5 @@
> +    update();
> +  }
> +
> +  container.addEventListener('mousedown', startDrag, false);
> +  container.addEventListener('touchstart', startDrag, false);

Eh. I wouldn't worry about touch events, but sure.

@@ +154,5 @@
> + */
> +function getScrollDelta (window, event) {
> +  if(event.deltaMode === LINE_SCROLL_MODE) {
> +    let fontSize = parseFloat(
> +      window.getComputedStyle(window.document.body).getPropertyValue('line-height')

It's nice to do this, but I wonder if it's really necessary. Just having a const scalar somewhere might be enough. Or, rather, just use deltaY every time. Your choice, I personally wouldn't bother. Less code is usually nice.

@@ +175,5 @@
> + * @param  {Function} update
> + */
> +function setScrollHandlers(window, dragZoom, changed, update) {
> +
> +  window.addEventListener('wheel', function mouseWheel (event) {

Nit: event listener never getting unbound.

@@ +186,5 @@
> +    // Update the zoom level
> +    let scrollDelta = getScrollDelta(window, event);
> +    let prevZoom = dragZoom.zoom;
> +    dragZoom.zoom = Math.max(0, dragZoom.zoom - scrollDelta * ZOOM_SPEED);
> +    let deltaZoom = dragZoom.zoom - prevZoom;

All of this modifying of `dragZoom` is a bit opaque and hard to follow. I'd really consider turning this in to a proper instance of a constructor at least.

@@ +192,5 @@
> +    // Calculate the updated width and height
> +    let prevHeight = window.innerHeight * (1 + prevZoom);
> +    let prevWidth = window.innerWidth * (1 + prevZoom);
> +    dragZoom.height = window.innerHeight * (1 + dragZoom.zoom);
> +    dragZoom.width = window.innerWidth * (1 + dragZoom.zoom);

Windows, windows, windows. See above comments :P

@@ +225,5 @@
> + *         The main dragZoom state
> + */
> +module.exports = function startDraggingAndZooming (window, container) {
> +
> +  let dragZoom = {

Oh, here it is! Definitely would love to see this featured more prominently, somewhere at the top or in its own file maybe?

@@ +253,5 @@
> +    // This is the callback after the dragZoom values have been changed, to be
> +    // set by the consuming code
> +    onChange: function noop () {},
> +
> +    // The smoothed values that are animated and eventuallu match the target

Typo: eventually.

::: devtools/client/memory/components/treemap/draw.js
@@ +119,5 @@
> +function drawTruncatedName (ctx, x, y, innerWidth, name) {
> +  let truncated = name.substr(0, Math.floor(name.length / 2));
> +  let formatted = truncated + ELLIPSIS;
> +
> +  if (ctx.measureText(formatted).width > innerWidth) {

This `measureText` method is very costly. I recall telling you a few weeks ago, have you looked into how the performance flame graphs optimizes/caches those things? Or maybe we don't care about performance here?

::: devtools/client/memory/components/treemap/index.js
@@ +20,5 @@
> +
> +  let canvases = createCanvases(window);
> +  let dragZoom = startDragZoom(window, canvases.container);
> +
> +	let draw = setupDraw(

Nit: weird indenting.