971008 - Don't cause System App reflow noise in the Developer HUD

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P2)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

devtools-layers currently use <div>counter</div>. Obviously changing the number create artificial reflows which may ends up into an infinite loop once there is something to display reflows into the system app.

Using canvas should solve the issue.

Comment 1

[Jan Keromnes [:janx]]

Could be fixed in one along with bug #982066.

Comment 2

[Dave Huseby [:huseby]]

perf team tags

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: devtools.canvas.patch

I didn't tried yet the patch on the device. Mostly on b2g-desktop trying to reproduce the same output. It's hard to predict if that will be exactly the same on the device due the font configuration beiing different for now.

Comment 4

[Jan Keromnes [:janx]]

Comment on attachment 8434108 [details] [diff] [review]
devtools.canvas.patch

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

Thanks for addressing this issue! I was working a different approach (only updating parts of a canvas with offset calculation and invalidations) but repainting the whole canvas seems like an easier fix.

::: apps/system/js/devtools/developer_hud.js
@@ +57,5 @@
> +      var ctx = null;
> +
> +      if (!canvas) {
> +        canvas = document.createElement('canvas');
> +        canvas.classList.add('widget');

Nit: This CSS class name is wrong, because the canvas is not a single widget. Maybe you could rename the class to "widgets"?

@@ +58,5 @@
> +
> +      if (!canvas) {
> +        canvas = document.createElement('canvas');
> +        canvas.classList.add('widget');
> +        canvas.height = 30;

I believe we have an edge case when widgets take up more than the width of the screen. The current behavior with flex is to wrap widgets to a second line. You're changing this behavior by drawing outside of the canvas.

I don't consider this edge case very important to fix, as I have never seen it in real usage, but please make sure to test with many widgets.

@@ +73,5 @@
> +      ctx.textBaseline = 'top';
> +
> +      ctx.save();
> +
> +      // Widgets are positioned starting from the left side of the screen.

Currently, widgets are aligned on the right of the screen. Why change to left alignment?

@@ +98,3 @@
>  
> +        // Draw widget text centered both horizontally and vertically.
> +        ctx.fillStyle = 'black';

Please change text color to white.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jan Keromnes [:janx] from comment #4)
> Comment on attachment 8434108 [details] [diff] [review]
> devtools.canvas.patch
> 
> Review of attachment 8434108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for addressing this issue! I was working a different approach (only
> updating parts of a canvas with offset calculation and invalidations) but
> repainting the whole canvas seems like an easier fix.
> 
> ::: apps/system/js/devtools/developer_hud.js
> @@ +57,5 @@
> > +      var ctx = null;
> > +
> > +      if (!canvas) {
> > +        canvas = document.createElement('canvas');
> > +        canvas.classList.add('widget');
> 
> Nit: This CSS class name is wrong, because the canvas is not a single
> widget. Maybe you could rename the class to "widgets"?
> 

|widgets| sounds good.

> @@ +58,5 @@
> > +
> > +      if (!canvas) {
> > +        canvas = document.createElement('canvas');
> > +        canvas.classList.add('widget');
> > +        canvas.height = 30;
> 
> I believe we have an edge case when widgets take up more than the width of
> the screen. The current behavior with flex is to wrap widgets to a second
> line. You're changing this behavior by drawing outside of the canvas.
> 
> I don't consider this edge case very important to fix, as I have never seen
> it in real usage, but please make sure to test with many widgets.
> 

This is a bit tricky to fix with canvas. We will have to makes canvas takes 2 lines. I will open a followup.

> @@ +73,5 @@
> > +      ctx.textBaseline = 'top';
> > +
> > +      ctx.save();
> > +
> > +      // Widgets are positioned starting from the left side of the screen.
> 
> Currently, widgets are aligned on the right of the screen. Why change to
> left alignment?
> 

Probably because I can't distinguish my left from my right and made a mistake in the comment :)

> @@ +98,3 @@
> >  
> > +        // Draw widget text centered both horizontally and vertically.
> > +        ctx.fillStyle = 'black';
> 
> Please change text color to white.

Oups!

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: devtools.canvas.patch

Patch with addressed comments. Still need to fix the tests. Will not be fun with canvas.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: devtools.canvas.patch

Comment 8

[Jan Keromnes [:janx]]

Comment on attachment 8436902 [details] [diff] [review]
devtools.canvas.patch

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

Works like a charm, and nice tests! Thank you very much for the drive-by coverage increase.

I'd love that one day the tests make sure the metrics appear in a specific order, but that's not very important, and probably not worth the effort right now.

Please address the few minor issues below, open a pull request, and I'll be happy to r+ :)

::: apps/system/js/devtools/developer_hud.js
@@ +67,5 @@
> +      // has been rotated. This does not cause a reflow as long as the width
> +      // is the same.
> +      canvas.width = window.innerWidth;
> +
> +      var ctx = canvas.getContext('2d');

`ctx` was already defined line 57.

::: apps/system/style/devtools/developer_hud.css
@@ +19,1 @@
>    opacity: 0.8;

If I can ask for a favour, please drive-by fix the opacity to 0.6 so that we can better see what's behind the HUD (e.g. the "Send" button in SMS app).

::: apps/system/test/unit/developer_hud_test.js
@@ +70,5 @@
> +        ctx.fillStyle = 'white';
> +        ctx.fillText(widget.value,
> +                     (widgetWidth - textWidth) / 2,
> +                     canvas.height / 4);
> +      }).bind(this));

I don't think you need to bind this function.

@@ +83,5 @@
>          {name: 'bugs', value: 42}
>        ]);
>        var view = getDeveloperHUD();
>        assert.isDefined(view);
> +      var widget = view.querySelector('.widgets');

Nit: s/var widget/var widgets/, plus the single occurrence below.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: devtools.canvas.patch

PR: https://github.com/vingtetun/gaia/compare/devtools.canvas?expand=1

And the same patch with nits fixed, thanks for the r- :)

Comment 10

[Jan Keromnes [:janx]]

Comment on attachment 8436980 [details] [diff] [review]
devtools.canvas.patch

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

r+! Thanks a lot for this awesome patch. And sorry for the r-, removing the review would probably have been nicer!

Also I was wondering why you didn't open a pull request to link to this bug instead of uploading a patch, but this works too.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jan Keromnes [:janx] from comment #10)
> Comment on attachment 8436980 [details] [diff] [review]
> devtools.canvas.patch
> 
> Review of attachment 8436980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+! Thanks a lot for this awesome patch. And sorry for the r-, removing the
> review would probably have been nicer!
> 

No worries. I actually appreciated the r-.

> Also I was wondering why you didn't open a pull request to link to this bug
> instead of uploading a patch, but this works too.

I don't like github review process.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I just realized that we need that for the speccompliant csp too. The code was using some inline background-color style that are removed by this patch.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/a4a0cdf06568f0224956ea556cd2459e232a5d5a