[System2][Statusbar] Implement AppStatusBar of AppWindow subcomponent

RESOLVED FIXED

Status

Firefox OS
Gaia::System
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: alive, Assigned: alive)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:+)

Details

Attachments

(1 attachment)

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.
Summary: [System2][Statusbar] Implement ShadowStatusBar → [System2][Statusbar] Implement ShadowStatusBar of AppWindow subcomponent
blocking-b2g: --- → backlog
Created attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

Lemme know what you think.
Attachment #8522018 - Flags: feedback?(kgrandon)
Attachment #8522018 - Flags: feedback?(etienne)
Depends on: 1098168
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?
Attachment #8522018 - Flags: feedback?(kgrandon)
Attachment #8522018 - Flags: feedback?(gmarty)
Attachment #8522018 - Flags: feedback+
(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 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.
Attachment #8522018 - Flags: feedback?(gmarty) → feedback+
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.
Attachment #8522018 - Flags: feedback?(etienne)
Assignee: nobody → alive
tracking-b2g: --- → +
Comment on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

Ready for review
Attachment #8522018 - Flags: review?(kgrandon)
Attachment #8522018 - Flags: review?(etienne)
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+!
Attachment #8522018 - Flags: review?(kgrandon) → review+
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!
Attachment #8522018 - Flags: review?(etienne)
(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 on attachment 8522018 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/26095

Let's see if this one works
Attachment #8522018 - Flags: review?(etienne)
Summary: [System2][Statusbar] Implement ShadowStatusBar of AppWindow subcomponent → [System2][Statusbar] Implement AppStatusBar of AppWindow subcomponent
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
Attachment #8522018 - Flags: review?(etienne) → review+
(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)
(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.
(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.
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()
(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.
Tree is weird, awaiting ..
https://github.com/mozilla-b2g/gaia/commit/fc4d17e49259ff799e60f6b70649fe6c8804dadc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.