1185900 - [LockScreen] Connection Info Manager is slow to draw the info after reboot

Status: RESOLVED FIXED

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

Description

[Greg Weng [:snowmantw][:gweng][:λ]]

On master, it takes almost 8 seconds to draw the first info on the LockScreen. I've bisected it's caused by Bug 1147237. Since it's for test and have too much comments, I open this bug to clarify the regression, with a video link.

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

Here is the video (sorry, I'm listening radio, please mute it freely):

https://www.youtube.com/watch?v=MNfGlAwZzCs

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

NI Gabriele to see if he can fix this with following patches, or we need to back it out (hope not).

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

I think this is because we're not listening to mozL10n.ready() anymore. Before we would immediately update the status when mozL10n.ready() was called while now we're waiting for something else to happen. I hadn't thought about this because I don't think this was intended. I'll prepare a patch to fix the issue, it should be a one-liner.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] gabrielesvelto:bug-1185900-speedup-contact-info-display > mozilla-b2g:master

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8636608 [details] [review]
[gaia] gabrielesvelto:bug-1185900-speedup-contact-info-display > mozilla-b2g:master

Two changes here:

- Immediately update the connection info when initializing the object, this ensures that we display something right away (though it's just the default values, those will be updated as soon as the observers start triggering)

- Do not use the settings lock manually for reading the values of 'ril.radio.disabled' and 'ril.telephony.defaultServiceId', use SettingsListener.observe instead. This is more concise and faster too as the two requests go out in parallel instead of one after the other

I've adjusted the unit-tests to comply with these changes and tested them on my device with two SIMs.

Comment 6

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8636608 [details] [review]
[gaia] gabrielesvelto:bug-1185900-speedup-contact-info-display > mozilla-b2g:master

Basically I think the changes of code is good. But from Tim I've heard SettingsListener now should be deprecated because of some API design issues, so I set feedback for Tim to see if this is a use case that SettingsListener could still match it well. My recognition is that the API issues is for the cases  only need to read the value once but not monitor it. If so, since this patch needs to monitor the values while we shall read the initial values, I think it's still good to use it here. Otherwise we may need to find other similar but better solutions.

Comment 7

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

Comment on attachment 8636608 [details] [review]
[gaia] gabrielesvelto:bug-1185900-speedup-contact-info-display > mozilla-b2g:master

I agree we should immediately update the UI if the label is indeed something vague like "(Unknown)", and Greg is right -- I don't think reverting direct mozSettings calls and use SettingsListener is the necessary change nor the right change here.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

OK, I'll upload another patch without the SettingsListener bits. The reason why I put them in the first place was that we were getting the settings lock from the SettingsListener already so just using the SettingsListener across the board made the code leaner and cleaner IMHO.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8636608 [details] [review]
[gaia] gabrielesvelto:bug-1185900-speedup-contact-info-display > mozilla-b2g:master

Updated patch without the SettingsListener bits.

Comment 10

[Greg Weng [:snowmantw][:gweng][:λ]]

Thanks Gabriele. However, to avoid violate the principle of landing code with unit tests, could you also write an unit test for that? I think it would be an easy one: you just need to mock things in the initialization method, and then to check if the `updateConnStates` will be invoked after call the init method.

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

I've updated the patch with an additional unit-test covering this case specifically.

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8636608 [details] [review]
[gaia] gabrielesvelto:bug-1185900-speedup-contact-info-display > mozilla-b2g:master

Looks good to me! Thanks!

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

Thanks, merged to gaia/master 41823c5c6fe58968bb0e37385f7386d172b66e74

https://github.com/mozilla-b2g/gaia/commit/41823c5c6fe58968bb0e37385f7386d172b66e74