Closed Bug 971008 Opened 8 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S4 (20june)

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

(Keywords: perf, Whiteboard: [c=devtools p= s=2014.06.20.t u=])

Attachments

(1 file, 3 obsolete files)

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.
Could be fixed in one along with bug #982066.
No longer blocks: devtools-layers
Depends on: 982066
Summary: Use canvas instead of div for devtool-layers → Don't cause System App reflow noise in the Developer HUD
perf team tags
Keywords: perf
Whiteboard: [c=devtools p= s= u=]
Priority: -- → P2
Attached patch devtools.canvas.patch (obsolete) — Splinter Review
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.
Attachment #8434108 - Flags: feedback?(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.
Attachment #8434108 - Flags: feedback?(janx) → feedback+
(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!
Attached patch devtools.canvas.patch (obsolete) — Splinter Review
Patch with addressed comments. Still need to fix the tests. Will not be fun with canvas.
Attachment #8434108 - Attachment is obsolete: true
Attached patch devtools.canvas.patch (obsolete) — Splinter Review
Attachment #8434902 - Attachment is obsolete: true
Attachment #8436902 - Flags: review?(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.
Attachment #8436902 - Flags: review?(janx) → review-
PR: https://github.com/vingtetun/gaia/compare/devtools.canvas?expand=1

And the same patch with nits fixed, thanks for the r- :)
Attachment #8436902 - Attachment is obsolete: true
Attachment #8436980 - Flags: review?(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.
Attachment #8436980 - Flags: review?(janx) → review+
(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.
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.
Blocks: 968907
https://github.com/mozilla-b2g/gaia/commit/a4a0cdf06568f0224956ea556cd2459e232a5d5a
Assignee: nobody → 21
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [c=devtools p= s= u=] → [c=devtools p= s=2014.06.20.t u=]
Target Milestone: --- → 2.0 S4 (20june)
You need to log in before you can comment on or make changes to this bug.