969827 - Map a preference to a Gaia setting to configure the jank threshold

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

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

Attachment: bug969827.gaia.patch

Comment 2

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

Attachment: bug969827.gecko.patch

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8372821 [details] [diff] [review]
bug969827.gecko.patch

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

At some point we'll need to redesign the developer panel. It's such a mess now...

Comment 4

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

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Comment on attachment 8372821 [details] [diff] [review]
> bug969827.gecko.patch
> 
> Review of attachment 8372821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> At some point we'll need to redesign the developer panel. It's such a mess
> now...

I agree with you, hopefully Janx is working a bit on that.

Comment 5

[Jan Keromnes [:janx]]

Comment on attachment 8372820 [details] [diff] [review]
bug969827.gaia.patch

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

Awesome! Thanks for the patch.

I am indeed working on redesigning part of the developer menu in bug #968220.

::: apps/settings/elements/developer.html
@@ +60,4 @@
>            </label>
>          </li>
>          <li>
> +          <p data-l10n-id="janks-threshold">Janks Threshold</p>

Nit: Could we rename this to "Jank Threshold"? I believe the plural is unnecessary.

@@ +64,5 @@
> +          <span class="button icon icon-dialog">
> +            <select name="devtools.janks.threshold" data-value-type="integer">
> +              <option value="20">20</option>
> +              <option value="50">50</option>
> +              <option value="100" selected>100</option>

Is the `selected` useful? I thought the current setting value automatically selected the right option.

::: build/config/common-settings.json
@@ +44,4 @@
>     "deviceinfo.update_channel": "",
>     "device.storage.writable.name": "sdcard",
>     "devtools.overlay": false,
> +   "devtools.janks.threshold": 100,

You are starting with a pretty low jank threshold. I think that's a good idea, but I seem to remember that you suggested starting at a high number (e.g. 500ms) and I'm curious about the decision.

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8372821 [details] [diff] [review]
bug969827.gecko.patch

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

::: b2g/chrome/content/settings.js
@@ +235,5 @@
>    }
>  });
>  
> +SettingsListener.observe('devtools.janks.threshold', 100, function(value) {
> +  Services.prefs.setIntPref('devtools.janks.threshold', value);

Nit: 'devtools.jank.threshold'?

Comment 7

[Paul Rouget [:paul]]

(we try to use "eventlooplag" instead of "jank" in the devtools code)

Comment 8

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

(In reply to Jan Keromnes [:janx] from comment #5)
> 
> You are starting with a pretty low jank threshold. I think that's a good
> idea, but I seem to remember that you suggested starting at a high number
> (e.g. 500ms) and I'm curious about the decision.

I'm mostly interested in big numbers for tests purpose. That should be easier to land something with a big threshold. And once it is landed, nobody can go beyond this virtual barrier, and the product can be enhanced by lowering it.

Comment 9

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

(In reply to Paul Rouget [:paul] from comment #7)
> (we try to use "eventlooplag" instead of "jank" in the devtools code)

I renamed all the |jank| in the preferences to |eventlooplag|.

Comment 10

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

https://github.com/mozilla-b2g/gaia/commit/23435b4f1c7bf8f2a24b8fce154e21933cec4eba

Gaia part. It looks not pretty in the new HUD but since I'm going to land the platform part to configure it in a sec it worth having it imho.

Comment 11

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

Gecko part: https://hg.mozilla.org/integration/mozilla-inbound/rev/30b62226a4d8

Comment 12

[Jan Keromnes [:janx]]

(In reply to Paul Rouget [:paul] from comment #7)
> (we try to use "eventlooplag" instead of "jank" in the devtools code)

I'm curious what lead to this decision. Sure "eventlooplag" is more precise, but "jank" seems to be the common term used by web developers, and it's shorter. I think we should keep using "jank" in user-facing interfaces like the Developer HUD.

Comment 13

[Jan Keromnes [:janx]]

Vivien, I think you forgot to reply to this question:

(In reply to Jan Keromnes [:janx] from comment #5)
> @@ +64,5 @@
> > +          <span class="button icon icon-dialog">
> > +            <select name="devtools.janks.threshold" data-value-type="integer">
> > +              <option value="20">20</option>
> > +              <option value="50">50</option>
> > +              <option value="100" selected>100</option>
> 
> Is the `selected` useful? I thought the current setting value automatically
> selected the right option.

It seems redundant and error-prone to maintain default values in two different locations.

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/30b62226a4d8

Comment 15

[Paul Rouget [:paul]]

(In reply to Jan Keromnes [:janx] from comment #12)
> (In reply to Paul Rouget [:paul] from comment #7)
> > (we try to use "eventlooplag" instead of "jank" in the devtools code)
> 
> I'm curious what lead to this decision. Sure "eventlooplag" is more precise,
> but "jank" seems to be the common term used by web developers, and it's
> shorter. I think we should keep using "jank" in user-facing interfaces like
> the Developer HUD.

Ok.

Comment 16

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

(In reply to Jan Keromnes [:janx] from comment #13)
> Vivien, I think you forgot to reply to this question:
> 
> (In reply to Jan Keromnes [:janx] from comment #5)
> > @@ +64,5 @@
> > > +          <span class="button icon icon-dialog">
> > > +            <select name="devtools.janks.threshold" data-value-type="integer">
> > > +              <option value="20">20</option>
> > > +              <option value="50">50</option>
> > > +              <option value="100" selected>100</option>
> > 
> > Is the `selected` useful? I thought the current setting value automatically
> > selected the right option.
> 
> It seems redundant and error-prone to maintain default values in two
> different locations.

It's probably not very useful.