1097503 - [System2][Statusbar] Implement AppStatusBar of AppWindow subcomponent

Status: RESOLVED FIXED

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

Description

[(inactive after 6/18) Alive Kuo [:alive]]

There's amount of logic in statusbar which is related to a specific app window now.
We should extract them out into AppChrome or ShadowStatusBar.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/26095

Lemme know what you think.

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

I think this is a very good start, statsubar.js is getting way too big. I don't see any immediate design problems. I had some performance concerns around how we're dispatching another event on every touch event, but if the animation is smooth then that works for me.

We definitely need to do a very thorough review of this to ensure we don't break anything before landing. Also flagging Guillaume as he's done a lot of statusbar work recently and should be kept in the loop. Any thoughts here?

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #2)
> Comment on attachment 8522018 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/26095
> 
> I think this is a very good start, statsubar.js is getting way too big. I
> don't see any immediate design problems. I had some performance concerns
> around how we're dispatching another event on every touch event, but if the
> animation is smooth then that works for me.

The animation is smooth. Maybe etienne has more opinions - he is always sensitive to performance issue.
I am fine if we just bypass the event by app.handleTouch() without recreating an event.

> 
> We definitely need to do a very thorough review of this to ensure we don't
> break anything before landing. Also flagging Guillaume as he's done a lot of
> statusbar work recently and should be kept in the loop. Any thoughts here?

Comment 4

[Guillaume Marty [:gmarty]]

Comment on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

Yay! A lighter statusbar.js!
I left a comment on Github. Also while testing I got the div.statusbar created twice in a window. This was because the WebIDE restarted the System app so though not something that would usually happen, I thought I'd let you know tho.

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

Can't set f+ yet because I think the integration test are failing for a good reason :) 
(ie. couldn't get the utility tray to show from a fullscreen app while testing manually on my phone.)
But I like the approach very much!

Made some comments on github.

Don't think we will have performance issue with the broadcasting. But since we have the app object I wouldn't mind having something like `app.statusBarTouchHandler(evt, height)` proxyfy-ing the calls instead. It is really specific and adding surface to the big app object, but I feel like for this use case we really don't want any other sub component listening on the broadcast so to speak.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

Ready for review

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

I left some comments on github, but this seems to work well and I can't find any major issues. You have my R+!

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

Looks like this is causing the fullscreen_statusbar test to go intermittent :/

It might be because we have a bit of tricky code calling evt.stopImmediatePropagation to prevent the UtilityTray from interfering. This code got move but is know calling stopImmediatePropagation on the broadcasted _statusbartouch event...

Testing on device I do feel like we're indeed missing gestures (might be placebo).

Anyway, it's enough for me to push against the broadcast, we should expose a method on the appWindow and pass the real event to it (and the height).

Also think Kevin's nits are pretty important :)

Anyway with these changes I'm confident we'll have a stable try and we'll be ready to land!

Comment 9

[Kevin Grandon :kgrandon]

(In reply to Etienne Segonzac (:etienne) from comment #8)
> Comment on attachment 8522018 [details] [review]
> Testing on device I do feel like we're indeed missing gestures (might be
> placebo).

To be honest I noticed this also, but I think this may have been present on master as well. I can get it into a state where it immediately brings down the utility tray, instead of showing the statusbar. I'm not convinced that this patch is causing these issues, just that we're giving it some extra attention during the review, and that these are issues that exist today :)

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

Let's see if this one works

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

I'm happy :)

There's 2 small (but blocking) issues left (feel free to flag me again if they require big changes):
- there's a worrying python test failure that might not be an intermittent (the python tests are usually super stable)
- when you boot, the homescreen statusbar will be black instead of transparent: you need to scroll down then up again for it to switch to transparent

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Etienne Segonzac (:etienne) from comment #11)
> - there's a worrying python test failure that might not be an intermittent
> (the python tests are usually super stable)

it's something else :) (see bug 1098118)

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Etienne Segonzac (:etienne) from comment #11)
> Comment on attachment 8522018 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/26095
> 
> I'm happy :)
> 
> There's 2 small (but blocking) issues left (feel free to flag me again if
> they require big changes):
> - when you boot, the homescreen statusbar will be black instead of
> transparent: you need to scroll down then up again for it to switch to
> transparent

It seems happens w/ lockscreen, looking.

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #13)
> (In reply to Etienne Segonzac (:etienne) from comment #11)
> > Comment on attachment 8522018 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/26095
> > 
> > I'm happy :)
> > 
> > There's 2 small (but blocking) issues left (feel free to flag me again if
> > they require big changes):
> > - when you boot, the homescreen statusbar will be black instead of
> > transparent: you need to scroll down then up again for it to switch to
> > transparent
> 
> It seems happens w/ lockscreen, looking.

Weird... it seems appChrome of homescreen does not get the first metachange event.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

WOW... during debugging I found that statusbar is triggering homescreen ensure when device is locked each time there is network activity!

"[HomescreenWindow][Homescreen][homescreen][1053427.086] ensuring homescreen...," app_window.js:1052
console.trace(): homescreen_window.js:189
hw_ensure() homescreen_window.js:189
hl_getHomescreen() homescreen_launcher.js:238
getHomescreen() homescreen_window_manager.js:98
awm_getActiveApp() app_window_manager.js:88
exports.Service.currentApp() service.js:304
LayoutManager.prototype.width() layout_manager.js:77
sb_getMaximizedStatusBarWidth() statusbar.js:657
sb_updateIconVisibility() statusbar.js:704
sb_updateNetworkActivity() statusbar.js:1020
sb_handleEvent()

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #13)
> > (In reply to Etienne Segonzac (:etienne) from comment #11)
> > > Comment on attachment 8522018 [details] [review]
> > > https://github.com/mozilla-b2g/gaia/pull/26095
> > > 
> > > I'm happy :)
> > > 
> > > There's 2 small (but blocking) issues left (feel free to flag me again if
> > > they require big changes):
> > > - when you boot, the homescreen statusbar will be black instead of
> > > transparent: you need to scroll down then up again for it to switch to
> > > transparent
> > 
> > It seems happens w/ lockscreen, looking.
> 
> Weird... it seems appChrome of homescreen does not get the first metachange
> event.

The root cause is homescreenWindow is not registering mozbrowsermetachange event due to the constructor change. fixed.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

Tree is weird, awaiting ..

Comment 18

[(inactive after 6/18) Alive Kuo [:alive]]

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