1098168 - [Statusbar] Instantiatable icons per modules

Status: RESOLVED FIXED

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

Description

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

The most effort of statusbar is dealing with API stuff to represent the states.
My proposal is let each icon to do that and let Statusbar to manage the instantiated icons.

Comment 1

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

Attachment: WIP

All right, it is still a sketch and does not work yet.
I am still thinking about having a baseIcon inherited from baseUI to reduce the shared show/hide/updateLabel stuff.

Lemme know what you think.

Comment 2

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #1)
> Created attachment 8522197 [details]
> WIP
> 
> All right, it is still a sketch and does not work yet.
> I am still thinking about having a baseIcon inherited from baseUI to reduce
> the shared show/hide/updateLabel stuff.
> 
> Lemme know what you think.

Note: if we worry about the rendering of the icons, we could try to cache the render request in statusbar and append all of them at one time!

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8522197 [details]
WIP

1000xF+. This is much nicer than what we have today. I'll add some comments to the pull request, but I do think at a minimum all of the files should go in some js/icons/ folder.

I also wonder if we could do something nice with web components. You could potentially have each icon be a web component within the statusbar. This would restrict, but create a very clean contract of what icon code can/can't do. This could definitely be a follow-up investigation though, and is much easier to build on top of what's here.

Comment 4

[Etienne Segonzac (:etienne)]

Comment on attachment 8522197 [details]
WIP

We should get Guillaume's feedback too.
I'm afraid this is going to be very hard to maintain, but there's 2 big pieces that could change my mind:

* more code reuse between icon components
* something more elegant than calling private _methods on this.manager, maybe dispatching an `iconchange` event, or maybe just exposing a clean Statusbar.update that icon components would call

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8522197 [details]
WIP

I left some nits on github, but overall I think we need this as statusbar.js is getting too large to reason about effectively.

As you and others have mentioned, I do think we need more shared code between the icon classes (ie. BaseIcon). Specifically, having all the icons update methods call back into the statusbar seems unfortunate. Might make sense to have `handleEvent` only defined in the base icon class, and then have this function call this.update(optionalEventType) followed by this.manager.updateIconVisibility(). Then for the special cases like the Downloads module that don't update icons, they can override the handleEvent function itself.

Comment 6

[Guillaume Marty [:gmarty]]

Comment on attachment 8522197 [details]
WIP

Left some comments, but overall we're on the right track I think.
As :mhenretty and :etienne said, we need to maximise code reuse via BaseIcon.

Also, this is a massive change. How can we make sure it won't impact performance? Specifically start up time?

Comment 7

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

(In reply to Guillaume Marty [:gmarty] from comment #6)
> Comment on attachment 8522197 [details]
> WIP
> 
> Left some comments, but overall we're on the right track I think.
> As :mhenretty and :etienne said, we need to maximise code reuse via BaseIcon.
> 
> Also, this is a massive change. How can we make sure it won't impact
> performance? Specifically start up time?

A simple answer is we could measure - however I suspect what's the basis.

Startup time is a big topic, I don't think we care any start up time stuff for system app right now.
We could do something, but definitely not in this bug - otherwise I feel I need to ask everyone stops working and sit down to refine it before any new patch.

BTW, system app js optimization (bug 941969) could help - if it's the js file numbers you worry.

Comment 8

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

Status update:
Currently on v2 (see the user story).

The trouble I need to fix before ask reviewing:
* Icons should be rendered in certain order.
* To determine: Icons should be "enabled" by statusbar in certain events(ftu state + screen state); or maybe the icon should be enabled by its manager in certain events.
* Loading sequence issue if any.

Comment 9

[Guillaume Marty [:gmarty]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #7)
> BTW, system app js optimization (bug 941969) could help - if it's the js
> file numbers you worry.

My main concern was switching from static HTML icon elements to elements generated in JavaScript. But as you said, startup time should be less of important in the system app. I can't think of any impact on performance as the application is running.

I agree with you, let's measure and address potential performance issues later if/when they arise.

Comment 10

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

Attachment: V3, review round 1

Sorry take much time. Most icon works now.
* Implement BaseIcon/BaseIconCollection
* Have each module in system app takes care their own icon instead of statusbar
* Have FtuLauncher takes care ftu step instead of statusbar
* Have MobileConnectionIcon takes care sub icons: SignalIcon & RoamingIcon
* Use hierarchyManager in TimeCore to turn on/off
* Have ScreenManager be able to "screensave" some modules while screen is switched to on/off.

What's going on:
* Unit test

Want to have your feedback before the formal review!

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8531994 [details]
V3, review round 1

Comments on github, mostly about naming so encouraging f+!

Before moving forward I think we should:
* see if we can get a green-ish try
* run Eli's phone restart test to check that we're not introducing a perf regression here

Eli, do you have the basics documented somewhere?

I also agree that, like you said on IRC, that we should time the landing of this patch properly once we know more about 2.x.

Comment 12

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

Progress: Fixing the *plenty* test failures. 80% done. Plan to request review next week.

Comment 13

[:Eli Perelman]

Etienne, currently the tests aren't hosted anywhere to run. It is possible to get set up to run them locally, but at the moment the process is somewhat involved. If you REALLY want to be able to run this test locally, ping me on IRC and I'll write up the instructions.

Comment 14

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

Comment on attachment 8531994 [details]
V3, review round 1

So let's start the first formal review round :)
* Implement BaseIcon
* Lazy load all icons controller in its manager
** Radio - OperatorIcon, MobileConnectionIcon - SignalIcon, RoamingIcon
** TimeCore(NEW) - TimeIcon
** AlarmMonitor(NEW) - AlarmIcon
** SoundManager - MuteIcon, PlayingIcon, HeadphoneIcon
** Bluetooth - BluetoothIcon, BluetoothTransferIcon, BluetoothHeadphoneIcon
** NfcManager - NfcIcon
** DownloadManager - DownloadIcon
** CallForwaring - CallForwardingIcon
** BatteryOverlay - BatteryIcon
** DebuggingMonitor(NEW) - DebuggingIcon
** MediaRecording - RecordingIcon
** EmergencyCallbackManager - EmergencyCallbackIcon
** TetheringMonitor(NEW) - TetheringIcon
** UsbCore(NEW) - UsbIcon
** GeolocationCore(NEW) - GeolocationIcon
** Wifi - WifiIcon
* Lazy load all icons elements in Statusbar once a time
* Bluetooth has a simple test right now
* FtuLauncher is handling ftu step change (it should be renamed to FtuAgent later)
* ScreenManager provides suspend mechanism
* TelephonyMonitor will provide call info in system service

What's found during the refactoring and not fixed in this patch:
* There is not an easy way to keep the icon in fixed order but still render them asynchronously
* Operator & Date Icon is coupling to TimeIcon. We should find a way to split Operator/Date into two icons.
* MobileConnectionIcons are frequently restyle (even before the patch) because it wants to check the width once voicechange or datachange is triggered.
* DownloadManager has limited test
* cloneStatusbar will make us have two elements having the id
* Statusbar is setting appearance according to some UI events, but it's complex to know the detail of those UI. A better way is utilizing hierarchychanged and stackchanged event to know who is the top most window to change the style.

Note:
* The plan is to land this after v2.2 is branched.
* StatusBar in FTU is missing due to bug 1113482. But the icon does show according to ftu step.
* Most icons work for me, but there might be something missing so welcome to point out the problem if you know the domain more than me.

Comment 15

[Krzysztof Mioduszewski[:tauzen]]

Comment on attachment 8531994 [details]
V3, review round 1

Right now NFC icon is not visible, left one comment on github how to fix it. Unit tests will need to be updated once the fix will be applied. Thanks.

Comment 16

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

(In reply to Krzysztof Mioduszewski[:kmioduszewski][:tauzen] from comment #15)
> Comment on attachment 8531994 [details]
> V3, review round 1
> 
> Right now NFC icon is not visible, left one comment on github how to fix it.
> Unit tests will need to be updated once the fix will be applied. Thanks.

Fixed :)

Status update:
* Fix callforwarding, nfc, mute icon bugs.

Comment 17

[[e-mail]]

Comment on attachment 8531994 [details]
V3, review round 1

Wow such a huge PR! Overall looks good to me for BT/media recording/USB core parts. 

IMHO I think it will be more manageable if you could land the BaseIcon module first then migrate icons one by one.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8531994 [details]
V3, review round 1

I only look at the API and syntax without checking anything else. Overall this is a huge improvement over what's currently in the System app.

I am cancelling the review because we need to harden the way promise chain is constructed, and the naming of BaseIcon methods should be reconsidered -- such as BaseIcon#displayed should really be BaseIcon#shouldDisplay.

Comment 19

[Michael Henretty [:mikehenrty][:mhenretty]]

ni? Eli for a boot time perf run on this patch.

Comment 20

[:Eli Perelman]

Attachment: Performance comparison

From my testing, it appears this patch actually improves reboot time by a little over 1 second. With 30 reboot repetitions comparing master and the attached patch, time to osLogoEnd fell from 44.59ms to 43.40s. See attached image [1].

[1] https://cloudup.com/caVGRMfMc_z

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8531994 [details]
V3, review round 1

I reviewed the parts related to mobile connections and wifi. I found some minor bugs, please check them in github. 

While reading the code, I had a hard time on identifying which methods are inherited(overridden) from BaseIcon and which ones are designed to be called by its manager. It could be improved by adding documents in BaseIcon describing the role of each method and how they get called. This helps the reader understand the purpose of each method.

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8531994 [details]
V3, review round 1

Happy with the approach, but going to clear review flag until comments by Tim/Arthur have been addressed.

Also after doing some very light testing I noticed that the bluetooth icon did not appear when I enabled it. I'm pretty sure it's a regression against master, so we should be sure that we add a test in a follow-up if this is a new regression and gaia-try didn't catch it. Nice work here!

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8531994 [details]
V3, review round 1

Great work! And you are improving boot time!!!

I left some comments on github, but nothing I would call deal breakers. Still, I want to see the final product and play around with it a little more before giving it the final r+.

Comment 24

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

Update: fixing most comments, still needs to check
* Emergency callback works
* Wifi icon fails to hide data icon
* l10n issues if any

Comment 25

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

Comment on attachment 8531994 [details]
V3, review round 1

Hi Eric, could you help to test emergency call icon & CDMA behavior of this patch? Thanks in advance.

Comment 26

[Eric Chang [:ericcc] [:echang]]

2 itesm.

1. Emergency call icon -> what kind of icon on status bar? I don't remember I saw that before.
2. All icons on CDMA?

Comment 27

[Aus Lacroix [:aus]]

Comment on attachment 8531994 [details]
V3, review round 1

Things looking good with the changes to downloads. I didn't have anything extra to add to the BaseIcon review itself. Assuming all tests still pass, I'm giving this my approval. That being said, I'd love to play with it before it gets committed once everyone else has also reviewed it. Cheers! :)

Comment 28

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

(In reply to Eric Chang [:ericcc] [:echang] from comment #26)
> 2 itesm.
> 
> 1. Emergency call icon -> what kind of icon on status bar? I don't remember
> I saw that before.

Sorry, should be emergency callback notification, it will show an icon at statusbar as well.

> 2. All icons on CDMA?

Mobile connection data text should be hidden once using CDMA + in a call

Comment 29

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

Observed interesting bug (not regression)
* Enable debugging
* Open settings app and enable NFC
* Pull down utility tray

Debugging icon will overlap with nfc icon.

Going to file a bug.

Comment 30

[Eric Chang [:ericcc] [:echang]]

Emergency callback can be tested with simulator, will try to figure out how to do that.
About CDMA, I was not able to turn on mobile connection, let me check with RIL team.

Comment 31

[Eric Chang [:ericcc] [:echang]]

Check: 
CDMA test, Ev(mobile network) disappears when there is an active call/
http://youtu.be/y-QSZtemYZA

Comment 32

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

One another:
Nobody is setting 'tethering.wifi.connectedClients' anymore; there is a new event from WifiManager: stationinfoupdate from https://bugzilla.mozilla.org/show_bug.cgi?id=774582

We will need to update it in gaia side.

What's more, 'tethering.usb.connectedClients' is deprecated either. I still cannot find someone who is doing that..

File a followup.

Comment 33

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

Comment on attachment 8531994 [details]
V3, review round 1

round 2 & Happy new year!

I tried my best to test all icons I could, except emergency-callback-notification - it's waiting QA approval, but may still miss something - welcome to play with it. Also I found some issues during the tests; we have much dead code. Filed some followups to improve.

After thinking I remove the suspendWithScreen and switch to visibilitychange; it would be easy to understand/test/maintain.

I am not sure if mobile connection icons need to support RTL - it didn't before. But call forwarding icons did.

Note: after https://bugzilla.mozilla.org/show_bug.cgi?id=864274 we will need an extra icon for battery level.

Yura/Eitan: could one of you check if this patch is screen reader friendly?

Comment 34

[Eitan Isaacson [:eeejay]]

Comment on attachment 8531994 [details]
V3, review round 1

I tested, and this doesn't introduce any regressions on the screen reader front. Looks like we are having some issues with the status bar in master now in bug 1115829, but this patch doesn't seem to touch it.

Please let Yura get a look too, since most of the status bar screen reader work is his. Thanks!

Comment 35

[Etienne Segonzac (:etienne)]

Comment on attachment 8531994 [details]
V3, review round 1

Happy new year everyone!
Very impressed with the patch itself *and* all the review work that was already one.

Added my comments on github, mainly discussing what gets exposed through `Service`, tests, and unimportant nits :)

Comment 36

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8531994 [details]
V3, review round 1

I am giving an r+ here since my comments are all recommendations, not requirements. Great job on designing the BaseIcon and it's relation to the controlling module.

I however, could think of a few ways future icon implementations can break the pattern here (for example, if a giving controlling module did not call this.update() but change this.element.hidden directly). It's really depending on how defensive we want but let's see how it goes.

Comment 37

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8531994 [details]
V3, review round 1

Looks good (added a comment re l10n), though the statusbar accessibility is generally broken in master and thus here as well.

Comment 38

[Kevin Grandon :kgrandon]

Comment on attachment 8531994 [details]
V3, review round 1

I'm still quite happy with the code, but doing some light testing I found that the USB storage icon no longer appears, when it's working fine for me in master. I'm just leaving an R- to make sure that you see the issue ;-)

I'll continue to look at the code and test some additional statusbar features, and let you know if I fine anything else.

Comment 39

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

(In reply to Kevin Grandon :kgrandon from comment #38)
> Comment on attachment 8531994 [details]
> V3, review round 1
> 
> I'm still quite happy with the code, but doing some light testing I found
> that the USB storage icon no longer appears, when it's working fine for me
> in master. I'm just leaving an R- to make sure that you see the issue ;-)
> 
> I'll continue to look at the code and test some additional statusbar
> features, and let you know if I fine anything else.

My fault, fixed!

Comment 40

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

Patch updated, waiting tbpl and then some manual tests then we could start the 3rd review run.

Comment 41

[Guillaume Marty [:gmarty]]

Comment on attachment 8531994 [details]
V3, review round 1

The date on the upper left corner when the utility tray is opened doesn't seem to appear.
Also, there have been some bug fixing in the status bar lately, so we must make sure they are replicated here too.

The number of JS files under system/js has gone crazy and it makes it impossible to open a file if you don't know exactly its name or just want to browse the files. I suggest we move all the *_icon.js to a subfolder icons/ or statusbar/. Any thoughts?

Apart from that, that's a massive work! Thanks Alive for that.

Comment 42

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

(In reply to Guillaume Marty [:gmarty] from comment #41)
> Comment on attachment 8531994 [details]
> V3, review round 1
> 
> The date on the upper left corner when the utility tray is opened doesn't
> seem to appear.

Good catch! I fixed it and added tests.

> Also, there have been some bug fixing in the status bar lately, so we must
> make sure they are replicated here too.
> 
> The number of JS files under system/js has gone crazy and it makes it
> impossible to open a file if you don't know exactly its name or just want to
> browse the files. I suggest we move all the *_icon.js to a subfolder icons/
> or statusbar/. Any thoughts?

IMO I don't think moving files into certain folders could help much,
but this is something worthy to be considered/discussed after this patch.

> 
> Apart from that, that's a massive work! Thanks Alive for that.

Comment 43

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

Comment on attachment 8531994 [details]
V3, review round 1

[Review round 3]

Etienne:
Let me know if the new render() works for you.
Also I have difficulty to figure out how appInstall test works...it seems old-fashioned and needs to be refactored (I think Evan is on it) but I need to keep the restore() there. I'd be happy if you could point me out why removing that would fail the test.

Kevin:
I'd fixed all issues I found including operator icon and bluetooth icon, lemme know what you found still.

Thanks everyone!

Comment 44

[Etienne Segonzac (:etienne)]

Comment on attachment 8531994 [details]
V3, review round 1

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #43)
> Comment on attachment 8531994 [details]
> V3, review round 1
> 
> [Review round 3]
> 
> Etienne:
> Let me know if the new render() works for you.

It does!

> Also I have difficulty to figure out how appInstall test works...it seems
> old-fashioned and needs to be refactored (I think Evan is on it) but I need
> to keep the restore() there. I'd be happy if you could point me out why
> removing that would fail the test.

That was just a nit anyway :) No worries.

Congratulations tackling such a big project so smoothly!

Comment 45

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8531994 [details]
V3, review round 1

Hrm, is it just me, or does the rocketbar search input not show up on the collapsed statusbar with this patch?

Comment 46

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [screenshot] no rocketbar input

This is what I'm seeing.

Comment 47

[Kevin Grandon :kgrandon]

There will still be many uplifts after branching, and until FC I believe. Uplifts will be quite difficult if this lands. I would recommend waiting until at least FL or maybe even until FC to land this. The downside being - that this could bitrot significantly in the meantime.

Alive - what date were you hoping to land this, and is waiting until 2.2 FC an option?

Comment 48

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

(In reply to Kevin Grandon :kgrandon from comment #47)
> There will still be many uplifts after branching, and until FC I believe.
> Uplifts will be quite difficult if this lands. I would recommend waiting
> until at least FL or maybe even until FC to land this. The downside being -
> that this could bitrot significantly in the meantime.
> 
> Alive - what date were you hoping to land this, and is waiting until 2.2 FC
> an option?

My original plan is just after 1/12 (and patch stabilize), need to check when is 2.2 FC..
But I don't see what's different between 2.2 branching and FC - we should have few new features in statusbar, don't we?

Comment 49

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

(In reply to Michael Henretty [:mhenretty] from comment #46)
> Created attachment 8545353 [details]
> [screenshot] no rocketbar input
> 
> This is what I'm seeing.

I will check, thanks Michael!

Comment 50

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #49)
> (In reply to Michael Henretty [:mhenretty] from comment #46)
> > Created attachment 8545353 [details]
> > [screenshot] no rocketbar input
> > 
> > This is what I'm seeing.
> 
> I will check, thanks Michael!

Hmm... so how to trigger "collapsed mode"?

Comment 51

[Michael Henretty [:mikehenrty][:mhenretty]]

Collapsed is the default for most apps. So for instance, all you have to do is open the SMS application and you should see the problem.

FWIW, I have been dogfooding this patch since yesterday, and that is the only problem I noticed in the statusbar that isn't already existing.

Comment 52

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [screenshot] missing "open" on lockscreen

Spoke to soon. This patch also causes notifications to not be actionable from the lockscreen. See screenshot.

Comment 53

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

(In reply to Michael Henretty [:mhenretty] from comment #51)
> Collapsed is the default for most apps. So for instance, all you have to do
> is open the SMS application and you should see the problem.

Interesting, maybe something bad happens after rebasing.

Comment 54

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

The search bar is missing is because the term 'search-the-web' is not translated. But the l10n properties has it and I didn't change it. Still don't know what happens...

Hi stas and gandalf,
Sorry to bother you, but I want to know: When is l10n starting to translate things? Is it too late to create an element and change dataset.1l0nId later? Is there a custom way to trigger translation? Is it possible something is not translated but no any warning?

Comment 55

[Staś Małolepszy :stas]

The exact heuristics are a bit complex but in general there is a MutationObserver set up to observe changes to the data-l10n-id attribute.

In the beginning of the page load we wait for readystatechange to interactive and then we start downloading the localization resources.  Once this is complete, we translate the entire DOM (if it wasn't pretranslated on buildtime) and also all elements that were dynamically created during the startup process.

Later on, when you add or modify the data-l10n-id attribute on an existing DOM element or when you insert a new element into the DOM with this attribute, the MutationObserver makes sure the element is localized.  This is what navigator.mozL10n.setAttributes uses, btw.

If l10n.js tries to localize something and can't find the translations, you should see L10nErrors in the log or in the console.  You might need to change DEBUG to true in l10n.js to see more detailed warnings, too.

What is the exact problem that you're seeing?

Comment 56

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

Okay I found the root cause: l10n will throw if we setAttributes() on a DOM element with childs - the signal icon having a child node (data text) so all translation after that will fail. Good lesson. (Not sure why that works now)

Comment 57

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

Patch updated: use mozL10n.formatValue for signal icon as stas suggested.
Michael I think this will fix the strange issues you see, let me know if any still.

Comment 58

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

Patch changed per :stas, what we need to do is just having a ".ariaLabel" for statusbarRoaming/statusbarWifi* in l10n prop file.

Comment 59

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #57)
> Patch updated: use mozL10n.formatValue for signal icon as stas suggested.
> Michael I think this will fix the strange issues you see, let me know if any
> still.

Hrmm, I'm still seeing the issue of the missing statusbar. Are you not able to reproduce this?

I'm also still seeing the issue of the missing "Open" button for lockscreen notifications.

Comment 60

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #56)
> Okay I found the root cause: l10n will throw if we setAttributes() on a DOM
> element with childs - the signal icon having a child node (data text) so all
> translation after that will fail. Good lesson. (Not sure why that works now)

Only if you try to set the value of the node that has child nodes. If you only set attributes, it will work fine.

Comment 61

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

(In reply to Michael Henretty [:mhenretty] from comment #59)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #57)
> > Patch updated: use mozL10n.formatValue for signal icon as stas suggested.
> > Michael I think this will fix the strange issues you see, let me know if any
> > still.
> 
> Hrmm, I'm still seeing the issue of the missing statusbar. Are you not able
> to reproduce this?
> 
> I'm also still seeing the issue of the missing "Open" button for lockscreen
> notifications.

It works for me.

Pic: http://imgur.com/SpsCoX9

Could you tell me your commit hash?

Comment 62

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

Patch updated to address :gandalf's comment, thanks!

Comment 63

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #61)
> It works for me.
> 
> Pic: http://imgur.com/SpsCoX9
> 
> Could you tell me your commit hash?

commit 68080a767335477d45d3e197656c51a9e66d50ce
Author: Alive Kuo <[e-mail]>
Date:   Thu Nov 13 21:31:38 2014 +0800

    Bug 1098168 - BaseIcon

commit e2a0f7c311119d4a8e160bdfb9e28a0e61a180fc
Author: Kevin Grandon <[e-mail]>
Date:   Tue Jan 13 13:17:08 2015 -0800

    Bug 1117976 - Disable intermittent statusbar_test.js | First Time Use > statusbar icons should change r=kgrandon

I am indeed still seeing this issue, with your patch applied to the latest gecko/gaia.

Comment 64

[Michael Henretty [:mikehenrty][:mhenretty]]

I also noticed that the Wifi icon isn't behaving properly. When you turn on Wifi, the icon immediately displays like Wifi is connected even when it's not. It also doesn't do the wifi connecting animation, or display anything but full signal for that matter.

Comment 65

[Kevin Grandon :kgrandon]

Comment on attachment 8531994 [details]
V3, review round 1

I'm more than happy to take a last look once Mike's comments have been addressed.

Comment 66

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8531994 [details]
V3, review round 1

To sum up, the three issue I've noticed are:

1.) Missing rocketbar input (maybe this is only my issue?)
2.) Lockscreen notifications lack an "Open" button
3.) Wifi icon behaves strangely

Please reflag me when these are addressed. At that point we should also have someone from QA run through the smoke tests here to sanity check. I can help coordinate that.

Comment 67

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

(In reply to Michael Henretty [:mhenretty] from comment #66)
> Comment on attachment 8531994 [details]
> V3, review round 1
> 
> To sum up, the three issue I've noticed are:
> 
> 1.) Missing rocketbar input (maybe this is only my issue?)
> 2.) Lockscreen notifications lack an "Open" button

I believe 1.,2. was fixed. Your commit hash indicates it was old than the l10n fix.

> 3.) Wifi icon behaves strangely

Fixing, thanks!

> 
> Please reflag me when these are addressed. At that point we should also have
> someone from QA run through the smoke tests here to sanity check. I can help
> coordinate that.

Comment 68

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

Comment on attachment 8531994 [details]
V3, review round 1

Thanks for your hard testing! Again if still any issue found feel free to point out.
Michael if you able to reach QA to test the overall stuff please do. Thanks in advance.

Comment 69

[Michael Henretty [:mikehenrty][:mhenretty]]

ni? Parul to help coordinate a QA smoketest run on this patch.

Comment 70

[Michael Henretty [:mikehenrty][:mhenretty]]

Going back to what Kevin said in comment 47, I think we either need to get uplift to 2.2 approval before landing, or wait until FC to land this. Otherwise, any statusbar fixes we need for 2.2 will have to be fixed twice since the codebase will be so different between master and 2.2.

Comment 71

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

(In reply to Michael Henretty [:mhenretty] from comment #70)
> Going back to what Kevin said in comment 47, I think we either need to get
> uplift to 2.2 approval before landing, or wait until FC to land this.
> Otherwise, any statusbar fixes we need for 2.2 will have to be fixed twice
> since the codebase will be so different between master and 2.2.

If FC is 2015/04/06 I don't really think it is adoptable. I also don't think uplift this patch is reasonable.

Fix twice in master/product is something I had from long time ago - even up to support 3 branches at the same time. I don't think it does not work for other engineers.

What I could afford is I could help on anything statusbar master patch from now on to FC. I will put off any other work if someone have trouble to write the master patch. That said, I don't think write something twice is a big problem.

Comment 72

[Kevin Grandon :kgrandon]

Comment on attachment 8531994 [details]
V3, review round 1

This is looking good to me. Nice work!

Comment 73

[Kevin Grandon :kgrandon]

Alive - I screwed up a bit here doing some testing and reverting today. I seemed to have accidentally pushed your commit to master after testing =| I've since reverted so master is fine, but this has the unfortunate side-effect of closing your PR.

Anyway - if this is ready I think we can just cherry-pick your commit onto master (instead of doing a new PR). I think I'm fine with landing this early if you will help us support statusbar/icon fixes too.

Thanks, and sorry about the noise/mess here =/

Comment 74

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

Recreated pull request
https://github.com/mozilla-b2g/gaia/pull/27485

Comment 75

[GitHub Autolander Bot]

Attachment: [PullReq] alivedise:bugzilla/1098168-v3/statusbar-baseicon to mozilla-b2g:master

Comment 76

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8531994 [details]
V3, review round 1

All comments were addressed, r=me. Great work!

Comment 77

[Eric Chang [:ericcc] [:echang]]

Attachment: EmergencyCallbackEmulator.png

Emergency Callback on emulator, see attachment

Comment 78

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8551086 [details] [review]
[PullReq] alivedise:bugzilla/1098168-v3/statusbar-baseicon to mozilla-b2g:master

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #67)
> (In reply to Michael Henretty [:mhenretty] from comment #66)
> > Comment on attachment 8531994 [details]
> > V3, review round 1
> > 
> > To sum up, the three issue I've noticed are:
> > 
> > 1.) Missing rocketbar input (maybe this is only my issue?)
> > 2.) Lockscreen notifications lack an "Open" button
> 
> I believe 1.,2. was fixed. Your commit hash indicates it was old than the
> l10n fix.
> 
> > 3.) Wifi icon behaves strangely
> 
> Fixing, thanks!

Hrm, I'm still seeing issue 1 and 2, but since no one else is seeing this I'm not going to hold up the review. 3 works great now!

Parul will be running through the smoketests with this today. Flagging her for qa-approval to that effect.

Comment 79

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

Attachment: 1098168-smoketest-bugs-screenshots.jpg

Failing Smoke Tests
1. The lockscreen does not show up when the Power button is pressed or on Display Timeout. This causes the following test cases to fail:
#15243 [Lockscreen] Verify Lockscreen Display after timeout
#2460 [CAMERA] Verify the user can take a picture with camera from lockscreen.

2. Overlay dialogs are missing title, text and button labels. This causes the following test cases to fail:
#6117 [HomeScreen] Verify the user can delete a packaged app (The delete app confirmation dialog is missing all text.)
#6073 [Browser] Verify that videos can be streamed and display correctly on YouTube.com (The app fullscreen permission dialog is missing the title, some text and button labels.)

3. Notifications do not display correctly because (1) missing lockscreen (2) when the status bar is pulled down. This causes the following test cases to fail:
#10744 [EMAIL] Notification for E-mail will be displayed


Other Severe Issues
1. Rocketbar search input does not show up on the collapsed statusbar. The collapsed statusbar sometimes does not show the time and many other icons.
2. Notifications show zero count and missing text for updates when status bar is pulled down. 
3. Wifi icon displays full strength and no animation even while connecting to a network.
4. Time on the device shows 10:31 PM (New York Time) but setting the alarm for 10:33 PM says that the alarm will go off 3 hours from now. (Device SIM Card is talking to cellphone tower in Los Angeles timezone which is 3 hours behind from New York.) Setting the time to Los Angeles timezone triggers the alarm correctly.


(see omnibus screenshot attached: 1098168-smoketest-bugs-screenshots.jpg)

~*~*~*~*~*~*

Test Environment

Flame device was initially flashed as follows:
Gaia-Rev        a5c5ac093814a80b0627514c3bd5f9e96c096a4b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/c1c6840d9255
Build-ID        20150120010227
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150120.044652
FW-Date         Tue Jan 20 04:47:02 EST 2015
Bootloader      L1TC000118D0

Then pull request https://github.com/mozilla-b2g/gaia/pull/27648 was applied as follows:

$ git clone https://github.com/mozilla-b2g/gaia.git
$ git checkout a5c5ac093814a80b0627514c3bd5f9e96c096a4b
$ git pull https://github.com/mikehenrty/gaia.git alivedise-bugzilla/1098168-v3/statusbar-baseicon
$ DEVICE_DEBUG=1 GAIA_DEV_PIXELS_PER_PX=1.5 make reset-gaia

After this, device had
Gaia-Rev        71d5a6ec7f302c7557c0db11ae68b394a6f957a4

Comment 80

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

Comment on attachment 8551086 [details] [review]
[PullReq] alivedise:bugzilla/1098168-v3/statusbar-baseicon to mozilla-b2g:master

Hi Parul,

Thanks for your hard test. However the test result does look strange because most of them does not appear on my device. I guess it's rebasing issues, could you test with this pull request again? Thanks again.

Comment 81

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

(In reply to Parul Mathur [:pragmatic] from comment #79)
> 
> 2. Overlay dialogs are missing title, text and button labels. This causes
> the following test cases to fail:
> #6117 [HomeScreen] Verify the user can delete a packaged app (The delete app
> confirmation dialog is missing all text.)
> #6073 [Browser] Verify that videos can be streamed and display correctly on
> YouTube.com (The app fullscreen permission dialog is missing the title, some
> text and button labels.)

There is no title for fullscreen request dialog in master as well.

All of the issues do not occur in 2563ed57acb4175f68a3ba3ce61799fbf4ea7f1f,
snapshots:
https://www.flickr.com/photos/49905767@N02/sets/72157650067729787/

Hi Eric, sorry to bother you again, could you help to double confirm https://github.com/mozilla-b2g/gaia/pull/27485 does not have comment 79's issues?

Comment 82

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

I also tried this from comment 79:

$ git clone https://github.com/mozilla-b2g/gaia.git test-1098168
$ cd test-1098168
$ git checkout a5c5ac093814a80b0627514c3bd5f9e96c096a4b
$ git pull https://github.com/mikehenrty/gaia.git alivedise-bugzilla/1098168-v3/statusbar-baseicon
$ DEVICE_DEBUG=1 GAIA_DEV_PIXELS_PER_PX=1.5 make reset-gaia

But it still works for me.
BTW..the last commit with these steps are
48a2164e7c233d4e9865f7ca0e44d828d80337d7

Comment 83

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

Patch updated: 5ca32cbbb83d5eac4526a3188b53466b02b369fc
Found an issue that if strength level is changed in mobile connection or wifi in collapsed mode, the maximized statusbar is not cloned to replace the minimized one. Test added.

Comment 84

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

Alive, could you let us know your detailed test environment by running the check_versions.sh script?

Here's how I setup mine:

1. Take a Flame device and flash the v18D base build on it: 
https://drive.google.com/a/mozilla.com/file/d/0Bz6aW3cbHgB8V0llaWI0N215R0E/view?usp=sharing

2. Use the Taiwan QA flashing tool to full flash the Jan 20 nightly master build:
https://github.com/Mozilla-TWQA/B2G-flash-tool
$ ./flash_pvt -w
Flashing paramaters: flame-kk, mozilla-central, Engineer, images, Build ID 20150120010227 (not latest).

3. Run the check_versions.sh script to determine the test environment:
$ ./check_versions.sh

This is the output:
Gaia-Rev        a5c5ac093814a80b0627514c3bd5f9e96c096a4b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/c1c6840d9255
Build-ID        20150120010227
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150120.044652
FW-Date         Tue Jan 20 04:47:02 EST 2015
Bootloader      L1TC000118D0

The rest of the steps that I followed are outlined in comment 79.

It seems that your Gaia-Rev and mine do not match, which is why we see different things.

Comment 85

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

(In reply to Parul Mathur [:pragmatic] from comment #84)
> Alive, could you let us know your detailed test environment by running the
> check_versions.sh script?
> 
> Here's how I setup mine:
> 
> 1. Take a Flame device and flash the v18D base build on it: 
> https://drive.google.com/a/mozilla.com/file/d/0Bz6aW3cbHgB8V0llaWI0N215R0E/
> view?usp=sharing
> 
> 2. Use the Taiwan QA flashing tool to full flash the Jan 20 nightly master
> build:
> https://github.com/Mozilla-TWQA/B2G-flash-tool
> $ ./flash_pvt -w
> Flashing paramaters: flame-kk, mozilla-central, Engineer, images, Build ID
> 20150120010227 (not latest).
> 
> 3. Run the check_versions.sh script to determine the test environment:
> $ ./check_versions.sh
> 
> This is the output:
> Gaia-Rev        a5c5ac093814a80b0627514c3bd5f9e96c096a4b
> Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/c1c6840d9255
> Build-ID        20150120010227
> Version         38.0a1
> Device-Name     flame
> FW-Release      4.4.2
> FW-Incremental  eng.cltbld.20150120.044652
> FW-Date         Tue Jan 20 04:47:02 EST 2015
> Bootloader      L1TC000118D0
> 
> The rest of the steps that I followed are outlined in comment 79.
> 
> It seems that your Gaia-Rev and mine do not match, which is why we see
> different things.

I followed your step in comment 79:

Gaia-Rev        48a2164e7c233d4e9865f7ca0e44d828d80337d7
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/fa91879c8428
Build-ID        20150125160206
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  39
FW-Date         Thu Oct 16 18:19:14 CST 2014
Bootloader      L1TC00011880

BTW my gecko is yesterday latest kk+mc+engineer build.
Moreover, if you are using Michael's pull request, the last gaia commit is
https://github.com/mikehenrty/gaia/commit/48a2164e7c233d4e9865f7ca0e44d828d80337d7 it is same as my gaia-rev instead of yours: 71d5a6ec7f302c7557c0db11ae68b394a6f957a4

Comment 86

[Eric Chang [:ericcc] [:echang]]

Hi Alive, here is my quick check with https://github.com/mozilla-b2g/gaia/pull/27485
1. Lock screen is there with the patch.
2. Missing title when videos streaming in full screen as you said.
3. Notification is also back with the patch.


(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #81)
> (In reply to Parul Mathur [:pragmatic] from comment #79)
> > 
> > 2. Overlay dialogs are missing title, text and button labels. This causes
> > the following test cases to fail:
> > #6117 [HomeScreen] Verify the user can delete a packaged app (The delete app
> > confirmation dialog is missing all text.)
> > #6073 [Browser] Verify that videos can be streamed and display correctly on
> > YouTube.com (The app fullscreen permission dialog is missing the title, some
> > text and button labels.)
> 
> There is no title for fullscreen request dialog in master as well.
> 
> All of the issues do not occur in 2563ed57acb4175f68a3ba3ce61799fbf4ea7f1f,
> snapshots:
> https://www.flickr.com/photos/49905767@N02/sets/72157650067729787/
> 
> Hi Eric, sorry to bother you again, could you help to double confirm
> https://github.com/mozilla-b2g/gaia/pull/27485 does not have comment 79's
> issues?

Comment 87

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

(In reply to Eric Chang [:ericcc] [:echang] from comment #86)
> Hi Alive, here is my quick check with
> https://github.com/mozilla-b2g/gaia/pull/27485
> 1. Lock screen is there with the patch.
> 2. Missing title when videos streaming in full screen as you said.
> 3. Notification is also back with the patch.
> 

Thanks!

Comment 88

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

Tested patch https://github.com/mozilla-b2g/gaia/pull/27485 and found all the same bugs again as in comment 79 except for the alarm clock one.

Steps to Reproduce
1. Take a Flame device and flash the v18D base build on it: 
https://drive.google.com/a/mozilla.com/file/d/0Bz6aW3cbHgB8V0llaWI0N215R0E/view?usp=sharing

2. Use the Taiwan QA flashing tool to full flash the Jan 29 nightly master build:
https://github.com/Mozilla-TWQA/B2G-flash-tool
$ ./flash_pvt -w
Flashing paramaters: flame-kk, mozilla-central, Engineer, images, latest (Build ID 20150129010239).

3. Run the check_versions.sh script to determine the test environment:
$ ./check_versions.sh

This is the output:
Gaia-Rev        9d2378a9ef092ab1fc15c3a9f7fc4171aab59d57
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/6bfc0e1c4b29
Build-ID        20150129010239
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150129.043711
FW-Date         Thu Jan 29 04:37:21 EST 2015
Bootloader      L1TC000118D0

4. Then pull request https://github.com/mozilla-b2g/gaia/pull/27485 was applied as follows:

$ git clone https://github.com/mozilla-b2g/gaia.git
$ cd gaia
$ git checkout master
$ git remote add alive https://github.com/alivedise/gaia
$ git fetch alive
$ git checkout bugzilla/1098168-v3/statusbar-baseicon
$ DEVICE_DEBUG=1 GAIA_DEV_PIXELS_PER_PX=1.5 make reset-gaia

After this, device had
Gaia-Rev        08c602b4e7de95ce14fffe67f55bf3c49f9df164

Comment 89

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

Per offline discussion, here is the landing policy of this patch:
* Mark RTL blocker block this bug - Michael could you handle this?
* Wait until those RTL blockers land and we will seek for qa-approval on this patch again after my *daily* rebasing is done; and then we land it without waiting FC.

For smoke test, I propose that we added a *third* QA, better from Taipei office to provide the smoke test result so it could help me to debug - because I am sure there is something unsync between us :/

Michael correct me if I miss something.

Comment 90

[Al Tsai [:atsai]]

I am going to take over the local testing job for confirming whether we can reproduce the failures Parul encountered. Currently, I can reproduce most of the problem. Since we are using debug build and it will turn lockscreen off by default. I assume it's not a blocker. For missing labels, I can reproduce the issue and Alive is now taking a look into it.

Comment 91

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

Comment on attachment 8551086 [details] [review]
[PullReq] alivedise:bugzilla/1098168-v3/statusbar-baseicon to mozilla-b2g:master

Request l10n review since I changed some key.
I am not sure if we could tolerate there is two key for the same value?

The problem is I don't want to introduce an async call to assign the aria-label "NoSimCard" for the statusbar signal icon with dom element children; so I created a new key 'statusbarSignalNoSimCard' but it should have the same translation with 'NoSimCard'. Also I found that the aria label 'emergencyCallOnly' is not having a key in the property file so I add it. Other key change is due to javascript side name change.

Comment 92

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

(In reply to Al Tsai [:atsai] from comment #90)
> I am going to take over the local testing job for confirming whether we can
> reproduce the failures Parul encountered. Currently, I can reproduce most of
> the problem. Since we are using debug build and it will turn lockscreen off
> by default. I assume it's not a blocker. For missing labels, I can reproduce
> the issue and Alive is now taking a look into it.

I'd found what's the difference:  Al/Parul's phone has no SIM card but I am having two SIM cards :|
The root cause is l10n will throw when we specify 'NoSimCard' on the signal icon element who has DOM element child(data text.)

I'd fixed it and will request smoke test again to you guys later. Thanks QAs!

Comment 93

[Francesco Lodolo [:flod]]

Comment on attachment 8551086 [details] [review]
[PullReq] alivedise:bugzilla/1098168-v3/statusbar-baseicon to mozilla-b2g:master

Steeling flag. String-wise the patch looks good, it's not an issue to have two strings with different IDs but the same content (it's actually needed when they're used in different contexts).

Comment 94

[Axel Hecht [:Pike]]

Comment on attachment 8551086 [details] [review]
[PullReq] alivedise:bugzilla/1098168-v3/statusbar-baseicon to mozilla-b2g:master

... opening another feedback request on stas.

I see a bunch of 

- foo = something
+ foo.ariaLabel = something

That triggers memories of trouble, but I'm not sure if we fixed that?

Comment 95

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #94)
> That triggers memories of trouble, but I'm not sure if we fixed that?

That should be bug 1091113 (fixed).

Comment 96

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

Comment on attachment 8551086 [details] [review]
[PullReq] alivedise:bugzilla/1098168-v3/statusbar-baseicon to mozilla-b2g:master

Now since the root cause is fixed, let's try again :)

Comment 97

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #89)
> Per offline discussion, here is the landing policy of this patch:
> * Mark RTL blocker block this bug - Michael could you handle this?
> * Wait until those RTL blockers land and we will seek for qa-approval on
> this patch again after my *daily* rebasing is done; and then we land it
> without waiting FC.
> 
> Michael correct me if I miss something.

Sounds about right. It's really only two bugs we should wait for bug 1119371 and bug 1126613. Bug 1119371 is the obvious important one.

Comment 98

[Staś Małolepszy :stas]

Comment on attachment 8551086 [details] [review]
[PullReq] alivedise:bugzilla/1098168-v3/statusbar-baseicon to mozilla-b2g:master

(In reply to Axel Hecht [:Pike] from comment #94)

> I see a bunch of 
> 
> - foo = something
> + foo.ariaLabel = something
> 
> That triggers memories of trouble, but I'm not sure if we fixed that?

Yes, we fixed that in bug 1091113.  Thanks for checking!

Comment 99

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

Tested patch https://github.com/mozilla-b2g/gaia/pull/27485 and saw none of the bugs reported in comment 79. However, one new bug was found: drop down lists no longer work. Tapping on a drop down list does not bring up the options. (See in Settings > Date and Time and Clock > New Alarm)

Steps to Reproduce
1. Take a Flame device and flash the v18D base build on it: 
https://drive.google.com/a/mozilla.com/file/d/0Bz6aW3cbHgB8V0llaWI0N215R0E/view?usp=sharing

2. Use the Taiwan QA flashing tool to full flash the Feb 04 nightly master build:
https://github.com/Mozilla-TWQA/B2G-flash-tool
$ ./flash_pvt -w
Flashing paramaters: flame-kk, mozilla-central, Engineer, images, latest (Build ID 20150204010225).

3. Run the check_versions.sh script to determine the test environment:
$ ./check_versions.sh

This is the output:
Gaia-Rev        dfebaaa8aab43470f482d09d71137bab840c3ae9
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/0c2f7434c325
Build-ID        20150204010225
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150204.043856
FW-Date         Wed Feb  4 04:39:07 EST 2015
Bootloader      L1TC000118D0

$ git clone https://github.com/mozilla-b2g/gaia.git
$ cd gaia
$ git checkout master
$ git remote add alive https://github.com/alivedise/gaia
$ git fetch alive
$ git checkout bugzilla/1098168-v3/statusbar-baseicon
$ GAIA_DEV_PIXELS_PER_PX=1.5 make reset-gaia

After this, device had
Gaia-Rev        7ba20543c479770e7e2db328280ba4a72a2ff187

Comment 100

[Al Tsai [:atsai]]

clear ni? per comment 99

Comment 101

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

Hmmm, I suspect value selector issue is https://bugzilla.mozilla.org/show_bug.cgi?id=1129329

Comment 102

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

Comment on attachment 8551086 [details] [review]
[PullReq] alivedise:bugzilla/1098168-v3/statusbar-baseicon to mozilla-b2g:master

We plan to land this week since RTL bugs are fixed and it's rebased now; the value selector issue should already been fixed already. Let's get QA test result to see what's blocking us to land.

Comment 103

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

Tested patch https://github.com/mozilla-b2g/gaia/pull/27485 and saw the following bugs:
1. When the airplane mode is toggled on in the Settings app, the cellular signal icon keeps looping in animation mode. (On the nightly master build, the icon disappears.)
2. During FTU, the Wifi icon does not display while connecting to a network. (This also reproduces on the nightly master build, so it is not a bug introduced by this patch.) 

Steps to Reproduce
1. Take a Flame device and flash the v18D base build on it: 
https://drive.google.com/a/mozilla.com/file/d/0Bz6aW3cbHgB8V0llaWI0N215R0E/view?usp=sharing

2. Use the Taiwan QA flashing tool to full flash the Feb 04 nightly master build:
https://github.com/Mozilla-TWQA/B2G-flash-tool
$ ./flash_pvt -w
Flashing paramaters: flame-kk, mozilla-central, Engineer, images, latest (Build ID 20150210160206).

3. Run the check_versions.sh script to determine the test environment:
$ ./check_versions.sh

This is the output:
Gaia-Rev        8c7865486a1b11076b849bbf8f7fccbaffbfafe7
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/ee093ca70666
Build-ID        20150210160206
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150210.193016
FW-Date         Tue Feb 10 19:30:27 EST 2015
Bootloader      L1TC000118D0

$ git clone https://github.com/mozilla-b2g/gaia.git
$ cd gaia
$ git remote add alive https://github.com/alivedise/gaia
$ git fetch alive
$ git checkout bugzilla/1098168-v3/statusbar-baseicon
$ GAIA_DEV_PIXELS_PER_PX=1.5 make reset-gaia

After this, device had
Gaia-Rev        e39c608517d17dafff795e08b3943a51ef7e482b

Comment 104

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

(In reply to Parul Mathur [:pragmatic] from comment #103)
> Tested patch https://github.com/mozilla-b2g/gaia/pull/27485 and saw the
> following bugs:
> 1. When the airplane mode is toggled on in the Settings app, the cellular
> signal icon keeps looping in animation mode. (On the nightly master build,
> the icon disappears.)

The root cause is this patch uses real ril state instead of settings db state to reflect the signal change;
I'd talked to Michael and Rob that this is not a blocker to land the patch. We could file followup to improve it (Because rob has concern about the speed, but eventually we will deprecate the settings db of ril but use real enable/disable API in the settings and the system app so I think even we switch to the settings db way in a moment, we still need to change it "back" later.)

> 2. During FTU, the Wifi icon does not display while connecting to a network.
> (This also reproduces on the nightly master build, so it is not a bug
> introduced by this patch.) 
> 

I think this is by design - the wifi icon will only show after you complete the wifi step of FTU. BUT I agree this is a problem. Let's file followup to improve.

Comment 105

[Al Tsai [:atsai]]

clear ni? per comment 103

Comment 106

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

https://github.com/mozilla-b2g/gaia/commit/60422eafb36528ff154075f98d15f3e26f6aa010

Comment 107

[Ryan VanderMeulen [:RyanVM]]

Something in this Bumper Bot push made Gaia unit tests near permafail.
https://hg.mozilla.org/integration/b2g-inbound/rev/4c7c738437d9

I already tried reverting bug 1132391, but the failures persisted. Bug 1120877 had a fully-green Gaia Try run, so I'm reverting this next.
Master: https://github.com/mozilla-b2g/gaia/commit/71defd569502274e877b56094e3a44d776033120

The failures we're hitting:
https://treeherder.mozilla.org/logviewer.html#?job_id=1344625&repo=b2g-inbound

Comment 108

[Ryan VanderMeulen [:RyanVM]]

Multiple retriggers on the revert appear to confirm that this was indeed reponsible for the failures in comment 107.

Comment 109

[Kevin Grandon :kgrandon]

Thank you so much for tracking that down Ryan. This is a really weird set of errors. I wonder if we could have some new race condition causing us to fail to fully startup?

Comment 110

[GitHub Autolander Bot]

Attachment: [gaia] alivedise:bugzilla/1098168-backout/base-icon > mozilla-b2g:master

Comment 111

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

Looking at the failures at comment 107, many of them are very unlikely to be triggered by the patch; I saw some of them even before my patch. What's the difference between Gaia-Try we are using and comment 107?

Comment 112

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

Investigation so far:
* Some failures are related to changed mockLazyLoader, trying to use a specific version for base icon.
* Some failures are related to document.hidden which are changed in TimeCore and NetworkActivity tests; but I don't understand why change document.hidden there will affect some unit tests :/ Also it's restored at suiteTeardown anyway. Really strange.

Comment 113

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

(In reply to Alive Kuo@Paris~2/17 [:alive][NEEDINFO!] from comment #112)
> Investigation so far:
> * Some failures are related to changed mockLazyLoader, trying to use a
> specific version for base icon.
> * Some failures are related to document.hidden which are changed in TimeCore
> and NetworkActivity tests; but I don't understand why change document.hidden
> there will affect some unit tests :/ Also it's restored at suiteTeardown
> anyway. Really strange.

Update: Removing document.hidden hijacking does not fix the problem.
Next: divide all the unit tests into several branches and run them at the same time to see which parts are causing the failure.

Comment 114

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

(In reply to Alive Kuo@Paris~2/17 [:alive][NEEDINFO!] from comment #113)
> (In reply to Alive Kuo@Paris~2/17 [:alive][NEEDINFO!] from comment #112)
> > Investigation so far:
> > * Some failures are related to changed mockLazyLoader, trying to use a
> > specific version for base icon.
> > * Some failures are related to document.hidden which are changed in TimeCore
> > and NetworkActivity tests; but I don't understand why change document.hidden
> > there will affect some unit tests :/ Also it's restored at suiteTeardown
> > anyway. Really strange.
> 
> Update: Removing document.hidden hijacking does not fix the problem.
> Next: divide all the unit tests into several branches and run them at the
> same time to see which parts are causing the failure.

Update: Even removing ALL unit tests in system app and restore shared/test/unit/mocks does not fix it...
How could this happen? How could change in system app affects lots unit test among several apps?

Comment 115

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

It seems when the failure happens 'document.hidden' is true due to unknown reason.
For example, AppChrome timeout: it's because in teardown it's waiting for window.requestAnimationFrame to finish the teardown; however, according to MDN, in a background tab, RAF will be slowed down so the timeout happens.

Comment 116

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

About tagVisibilityMonitor timeout:
I don't understand why we are using setTimeout() in unit test..:(
It makes sense the function is not invoked because it's at background while running(Although we don't know why it's at background and I don't think simply defineProperty for document.hidden will affect the timer schedule in gecko.)

https://github.com/mozilla-b2g/gaia/blob/master/apps/sharedtest/test/unit/tag_visibility_monitor_test.js#L322
Kevin, thought?

Comment 117

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

I forced document.hidden to be false in all failed unit test setup:
https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=39f1e9ddb1bb

Some of them are fixed, but those who timeouts still exist.

Comment 118

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #117)
> I forced document.hidden to be false in all failed unit test setup:
> https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=39f1e9ddb1bb
> 
> Some of them are fixed, but those who timeouts still exist.

So far I believe this is a test framework issue. i.e. any change in b2g code base should not affect individual unit tests; override document.hidden does not work, i.e., something causes the test environment to go to background and every 'requestAnimationFrame', 'setTimeout' ... stuff then timeouts.

I don't know what I could do now, honestly.

Comment 119

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

Julien, James,

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=26fc0a08d724

I found that my patch will cause the iframe sandbox in test-agent to run at background (or it would be run at background in the past but something in my patch made that situation more repro-able.); hence all setTimeout and requestAnimationFrame will timeout because gecko will slow down a background tab.

Do you guys have any clue to fix it? I tend to think this is a test-agent problem because I am modifying system app and this should not change any behavior of any unit test.

I tried to increase SCREEN_TIMEOUT to 0 but it does not work.

Please help before my last resort - to disable those tests :(

Comment 120

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

Here is my analysis:
Test agent is running all tests in individual iframes; we are using desktop b2g to run it.
something happens during test agent is busy running the 18xxx unit tests and causes it go to background.
(I originally guess it's screen time out but set screen.timeout to 0 does not fix it.)

All the iframes of test agent will go background as their parent(test agent) does.

Some of them(TagVisibilityMonitor) are using setTimeout(this is really bad) and wait for the setTimeout to finish the test hence timeouts.
Some of them(AppChrome test) are using requestAnimationFrame to finish the test but gecko will slow down the  callback speed of RAF in a background page.

I didn't go through all the remaining timeout (will go if in the long run we need to disable all the tests),
but so far everything is related to the background state of the test agent and the sandbox iframe.

If the tests/scripts are checking document.hidden, I could mock it in setup to avoid that;
but I cannot change the behavior of setTimeout/requestAnimationFrame of gecko...

Comment 121

[Kevin Grandon :kgrandon]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #113)
> Update: Removing document.hidden hijacking does not fix the problem.
> Next: divide all the unit tests into several branches and run them at the
> same time to see which parts are causing the failure.

Hmm, that was my biggest guess as to what caused this. If tests still fail after removing the changes to system unit tests and mocks, then we must be triggering some change in behavior by the new system code.

The next thing I'd look at is the visibilitychange listener in network_monitor. Another option, though very painful, would be to figure out a way to split this up into several different patches, enabling and landing icons one-by-one.

Leaving ni?, I will continue to investigate in the next few days.

Comment 122

[Kevin Grandon :kgrandon]

Oops - I don't have anything immediately actionable for you yet. Clearing ni.

Comment 123

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

Update: I loaned a machine from buildduty to run mozharness on the 'slow' ubuntu instance to see if I could find out what happens during the tests. Will update soon.

Comment 124

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #123)
> Update: I loaned a machine from buildduty to run mozharness on the 'slow'
> ubuntu instance to see if I could find out what happens during the tests.
> Will update soon.

Have some trouble running mozharness on the loaned machine.
=====


I still suspect the reason is very simple:
1. Maybe we have too many tests to run to over 10 minutes device timeout
2. Maybe we never send the active app to background before

When device timeouts, system app goes to background and then all of the apps will go to background including test-agent. Then all the iframes go to background so all the timeout failure happens. This is most possible reason of all the bad things..

What I don't understand is
1. why make SCREEN_TIMEOUT=0 does not fix this. 
2. In theory my idea works but I don't know how to approve it.

Comment 125

[Julien Wajsberg [:julienw]]

So, there are several thoughts here:

* switch to Firefox (or Mulet) to run Gu tests (bug 1068809); there is near zero value to run the unit tests in B2G Desktop.
* switch to a newer mocha (bug 874510) which is a lot faster. Some tests were still failing last time I checked (some months ago).
* maybe we uncovered a real bug in your patch? But because it's so huge, it's difficult to point exactly where the issue is :/


You can try to run in B2G Desktop locally using "bin/gaia-test -d" and then "make test-agent-test" in another terminal; I don't think you need mozharness to reproduce this -- unless the environment difference plays a role here, but I doubt it.

Comment 126

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

Update:
I successfully ran mozharness in the loaned machine.
* With the original patch: the device time outs and failure reproduced.
* With the patch + hardcode screen.timeout settings to 0 in common settings: the device never time outs,
but, I am seeing the homescreen after running some tests.

Here is my guess:
My patch makes system app inits quickly than before; and we are using a "non-official" way to launch the test-agent app from command line(via --runapp?). When test agent is launched programtically, the homescreen is launched so system app decides to display the homescreen and send test agent to background.

If my guess is right, I don't think this is a bug of system app; system app is not designed to satisfy this kind of "human-impossible" behavior - to launch an app before homescreen is launched.

Comment 127

[Kevin Grandon :kgrandon]

Nice finding Alive, seems like that could definitely be the cause. We might need to allow the system app to handle this instead of gecko. Clearing ni, let me know if there's anything specific you want me to do here.

For reference, here is where we launch the app from gecko: http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/runapp.js#141

Comment 128

[Kevin Grandon :kgrandon]

Although we did try to buffer the runapp launch command in bug 999574 to try to prevent something like this: http://hg.mozilla.org/mozilla-central/rev/9fec1eb9dfeb

Comment 129

[James Lal [:lightsofapollo]]

Looks like most of the context has been added here... The only thing I can think to add is maybe we can tweak the settings we use in the profile to keep the screen alive ?

Comment 130

[Julien Wajsberg [:julienw]]

Yeah, that's something alive has tried and worked. I think he's working on resolving the race with --runapp.

Another alternative to --runapp could be to use the test-agent URL in the setting "homescreen.manifestURL".

Comment 131

[Kevin Grandon :kgrandon]

(In reply to Julien Wajsberg [:julienw] from comment #130)
> Yeah, that's something alive has tried and worked. I think he's working on
> resolving the race with --runapp.

Again, we should be guarding --runapp with this: http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/runapp.js#130

This worked in the past, but maybe something has changed to make this racey again.

Comment 132

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

I'd fixed the problem by a workaround in appWindowManager - but yes we need to fix runapp stuff in the long term.
Try is green https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=94e8e1c5ff7f
I'd re-triggered the unit test several times to confirm nothing intermittent.

The real problem is --runapp who triggers mozApps.launch is caught by AppWindowFactory before HomescreenWindow /FtuLauncher is ready. When homescreen is ready and ftu is skipped, AppWindowManager will open homescreenWindow right away - it does not know that there will be a third app (except homescreen and ftu) being launched before homescreen starts.

After thinking, I decided not to cache the launchapp request in AppWindowFactory (because it's somewhat dangerous to me) but only workaround in AppWindowManager. We need someone to fix the runapp stuff and figure out what's the best timing to call app launch. I could offer some hints I think.

BTW, what's strange is, screen timeout does not take test agent to background as I thought - this should be another issue which might cause potential intermittent failure in the future.

Comment 133

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #132)
> I'd fixed the problem by a workaround in appWindowManager - but yes we need
> to fix runapp stuff in the long term.
> Try is green
> https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=94e8e1c5ff7f
> I'd re-triggered the unit test several times to confirm nothing intermittent.
> 
> The real problem is --runapp who triggers mozApps.launch is caught by
> AppWindowFactory before HomescreenWindow /FtuLauncher is ready. When
> homescreen is ready and ftu is skipped, AppWindowManager will open
> homescreenWindow right away - it does not know that there will be a third
> app (except homescreen and ftu) being launched before homescreen starts.
> 
> After thinking, I decided not to cache the launchapp request in
> AppWindowFactory (because it's somewhat dangerous to me) but only workaround
> in AppWindowManager. We need someone to fix the runapp stuff and figure out
> what's the best timing to call app launch. I could offer some hints I think.
> 
> BTW, what's strange is, screen timeout does not take test agent to
> background as I thought - this should be another issue which might cause
> potential intermittent failure in the future.

Therefore, I'd recommend to open another bug to turn SCREEN_TIMEOUT=0 for DEBUG=1.

Comment 134

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

All re-runs are green. I am doing some final check and then merge again. Thanks.

Comment 135

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

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

Comment 136

[Julien Wajsberg [:julienw]]

Alive, I think you shouldn't have renamed the locale key in [1] because it's used in [2] and I don't see any l10n key "statusbarOperator" anywhere.

[1] https://github.com/mozilla-b2g/gaia/commit/d9399ecd386aaf37214c4bffcaceca335c143413#diff-81fe86f3f20787d354c9275a837572b9L37

[2] https://github.com/mozilla-b2g/gaia/commit/d9399ecd386aaf37214c4bffcaceca335c143413#diff-a2db6a37ed9d7ff55761f283bd363473R65

Comment 137

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

(In reply to Julien Wajsberg [:julienw] from comment #136)
> Alive, I think you shouldn't have renamed the locale key in [1] because it's
> used in [2] and I don't see any l10n key "statusbarOperator" anywhere.
> 
> [1]
> https://github.com/mozilla-b2g/gaia/commit/
> d9399ecd386aaf37214c4bffcaceca335c143413#diff-
> 81fe86f3f20787d354c9275a837572b9L37
> 
> [2]
> https://github.com/mozilla-b2g/gaia/commit/
> d9399ecd386aaf37214c4bffcaceca335c143413#diff-
> a2db6a37ed9d7ff55761f283bd363473R65

:/ Let's open another issue

Comment 138

[Zibi Braniecki [:zbraniecki][:gandalf]]

This patch removed statusbarLabel string from system.properties resource, but added a call to it: https://github.com/mozilla-b2g/gaia/commit/c53d38fbc8510862defc08a568e355f8d198c757#diff-a2db6a37ed9d7ff55761f283bd363473R65

This results in:

W/GeckoConsole(  195): Content JS WARN: L10nError: "statusbarLabel" not found in en-US in app://system.gaiamobile.org/index.html 
W/GeckoConsole(  195):     at reportMissingEntity (app://system.gaiamobile.org/gaia_build_index.js:245:261)
W/GeckoConsole(  195): Content JS WARN: L10nError: "statusbarLabel" not found in en-US in app://system.gaiamobile.org/index.html 
W/GeckoConsole(  195):     at reportMissingEntity (app://system.gaiamobile.org/gaia_build_index.js:245:261)
I/Gecko   (  195): SharedSurface_Gralloc::Create -------
I/Gecko   (  195): SharedSurface_Gralloc::Create: success -- surface 0xa84deaf0, GraphicBuffer 0xab922580.
W/GeckoConsole(  195): Content JS WARN: L10nError: "statusbarLabel" not found in en-US in app://system.gaiamobile.org/index.html 
W/GeckoConsole(  195):     at reportMissingEntity (app://system.gaiamobile.org/gaia_build_index.js:245:261)

(I'm not sure why it's called three times, I suspect aggressive calls to OperatorIcon.update which fires mozL10n.setAttributes which triggers MutationObserver.

Alive, do you want to file a follow up or fix it here?

Comment 139

[Julien Wajsberg [:julienw]]

Gandalf, is it bug 1149908 ?

Comment 140

[Zibi Braniecki [:zbraniecki][:gandalf]]

yes! Sorry :)