1022724 - Display a devtool overlay for the system app

Status: RESOLVED FIXED

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

Description

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

It would be nice to finally be able to see the devtools overlay for the system app.

Comment 1

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

Attachment: devtools.system.patch

Jan,

I'm not sure that moving the SettingsListener.observe call here is something you expect ?

Comment 2

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

Attachment: devtools.system

Just realized that I introduced a new !important and didn't fix up the test.

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8437037 [details] [diff] [review]
devtools.system

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

Thanks a lot for this feature!

Horrible nit-monster strikes again:

::: apps/settings/elements/developer_hud.html
@@ +26,5 @@
>          </li>
> +        <li>
> +          <label class="pack-checkbox">
> +            <input type="checkbox" name="devtools.overlay.system">
> +            <span data-l10n-id="devtools-overlay-system">Show System overlay</span>

(please reflect properties nits)

::: apps/settings/locales/settings.en-US.properties
@@ +993,5 @@
>  debugger-disabled=Disabled
>  debugger-adb-only=ADB only
>  debugger-adb-and-devtools=ADB and DevTools
>  devtools-overlay=Show devtools overlay
> +devtools-overlay-system=Show System overlay

Over-nits: Please,
- remove the unused "devtools-overlay" property,
- rename "devtools-overlay-system" to "hud-system", and "Show System overlay" to "Show system HUD" (with lowercase "system"),
- move both "hud-system" and "jank-threshold" properties up with the rest of the "hud-" properties.

::: apps/system/js/devtools/developer_hud.js
@@ +17,5 @@
> +
> +      SettingsListener.observe('devtools.overlay.system',
> +                               false, (function(value) {
> +        this.toggleSystemOverlay(value);
> +      }).bind(this));

Nit: Please bind this.toggleSystemHUD in the DeveloperHUD() constructor, and use it directly as an observer instead of creating and anonymous wrapper function.

@@ +26,5 @@
>      stop: function() {
>        window.removeEventListener('developer-hud-update', this);
>      },
>  
> +    _showSystemOverlay: false,

Nit: Please rename to "_showSystemHUD", and move to top of prototype.

@@ +27,5 @@
>        window.removeEventListener('developer-hud-update', this);
>      },
>  
> +    _showSystemOverlay: false,
> +    toggleSystemOverlay: function(enabled) {

Nit: Please rename to "toggleSystemHUD".

@@ +59,2 @@
>        if (!appwindow) {
> +        if (target === window && this._showSystemOverlay) {

Nit: _showSystemHUD

@@ +65,3 @@
>        }
>  
> +      var overlay = appwindow.querySelector(':scope > .developer-hud');

Why do you use ":scope >" here?

::: apps/system/style/zindex.css
@@ +25,5 @@
>  }
>  
> +/* System developer HUD should be above everything */
> +#screen > .developer-hud {
> +  z-index: 2147483647;

Nit: I guess this doesn't cover the home gesture panel for now, but I'd feel more confident if z-index was one less.

@@ +32,5 @@
>  #screen > *[data-z-index-level="poweroff-splash"] {
>    z-index: 65536;
>  }
>  
>  #screen > [data-z-index-level="debug-grid"],

Nit: The grid was removed, please remove this useless selector.

Comment 4

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

Thanks for the review.

 
> @@ +26,5 @@
> >      stop: function() {
> >        window.removeEventListener('developer-hud-update', this);
> >      },
> >  
> > +    _showSystemOverlay: false,
> 
> Nit: Please rename to "_showSystemHUD", and move to top of prototype.
>

Renamed but this is a good practice to keep variables close to where they are declared. This makes obvious the link between this property and the code that toggles it. So I kept it here.

> 
> @@ +65,3 @@
> >        }
> >  
> > +      var overlay = appwindow.querySelector(':scope > .developer-hud');
> 
> Why do you use ":scope >" here?
> 

:scope is like |this| of css in this case.

I needed to select the direct child of the container. Otherwise since the developer HUD for the main window is contained into #screen, the code will have returned 2 HUD or more based on the numbers of developer HUDs.

> ::: apps/system/style/zindex.css
> @@ +25,5 @@
> >  }
> >  
> > +/* System developer HUD should be above everything */
> > +#screen > .developer-hud {
> > +  z-index: 2147483647;
> 
> Nit: I guess this doesn't cover the home gesture panel for now, but I'd feel
> more confident if z-index was one less.
> 


I would like the System stuff to works even for fullscreen apps.

Comment 5

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

Attachment: devtools.system

Comment 6

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

Attachment: devtools.system

Oups. Forgot to remove grid from zindex.css

Comment 7

[Jan Keromnes [:janx]]

Comment on attachment 8437593 [details] [diff] [review]
devtools.system

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

r+ with a nit you forgot to fix :)

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #4)
> 
> Renamed but this is a good practice to keep variables close to where they
> are declared. This makes obvious the link between this property and the code
> that toggles it. So I kept it here.

Good point.

> > Why do you use ":scope >" here?
> > 
> 
> :scope is like |this| of css in this case.
> 
> I needed to select the direct child of the container. Otherwise since the
> developer HUD for the main window is contained into #screen, the code will
> have returned 2 HUD or more based on the numbers of developer HUDs.

Well actually `querySelector` would just return the first HUD it finds, but I guess :scope makes it cleaner.

::: apps/system/js/devtools/developer_hud.js
@@ +10,5 @@
>    function DeveloperHUD() {
> +    SettingsListener.observe('devtools.overlay.system',
> +                             false, (function(value) {
> +      this.toggleSystemHUD(value);
> +    }).bind(this));

Please, change `(function(value) { this.toggleSystemHUD(value); }).bind(this)` to `this.toggleSystemHUD.bind(this)`.

Comment 8

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

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