948847 - [Gaia] add AirplaneModeHelper to make sure settings app and system app will not do the same thing

Status: RESOLVED FIXED

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

Description

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

This is a follow-up enhancement that we can make a AirplaneModeHelper to extract airplane mode logic existed in settings app and systems app.

Comment 1

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

This is a follow-up enhancement but from bug 945140.

Comment 2

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: WIP

It is currently a temp WIP.

Comment 3

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8349924 [details] [review]
WIP

hi Arthur,

can you give me some feedbacks about the implementations ? 

Just want to make sure the direction is right ! Thanks :)

Comment 4

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

Comment on attachment 8349924 [details] [review]
WIP

Looks good to me. Please check my github comments, thanks!

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

@Arthur, 

currently it works now and I think I still need to do some code refactoring. 

If possible, can you give it a try on your device ? Thanks :)

Comment 6

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8349924 [details] [review]
WIP

Hi Alive & Arthur, 

I just totally (well just a little bit ??) rewrote the airplaneMode in settings & system apps.

After this redesign, airplane_mode.js in settings app only has 5x lines now wow~ 

I just tested all airplane mode related test cases on MozTrap and they all work well, so I think its functionality is ok and just have to refactor some code and it will be fine to be used !

Can you guys give me some feedbacks ? Thanks :)

Comment 7

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

@alive, do you think it is needed to extract updateStatus/restore/suspend into a separate file (?

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8349924 [details] [review]
WIP

Hi Arthur & Alive,

can you help me review code base :) ?  Thanks !

Comment 9

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

Comment on attachment 8349924 [details] [review]
WIP

Good job, EJ! Please check my comments in github. And please also update related unit tests.

Comment 10

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

Blocks blocking bugs.

Please do flag the bug if you plan to tackle the blocking bugs this way, thanks!

Comment 11

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

Comment on attachment 8349924 [details] [review]
WIP

I believe Arthur's review is enough.

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #11)
> Comment on attachment 8349924 [details] [review]
> WIP
> 
> I believe Arthur's review is enough.

Thanks Alive.

Comment 13

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8349924 [details] [review]
WIP

Hi Arthur, 

I added unit test for this patch and I think they are good to go. 

BTW, I also rebased the code base to make it able to be merged.

Thanks !!

Comment 14

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

Comment on attachment 8349924 [details] [review]
WIP

Cancel the review per the offline discussion. We are almost there!

Comment 15

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8349924 [details] [review]
WIP

Hi Arthur, 

I need your review again !! Thanks :)

Comment 16

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

Comment on attachment 8349924 [details] [review]
WIP

r=me. Thank you for the effort! Please squash all commits before merging.

Comment 17

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Special thanks to Jose, Arthur and Alive for your great help. 

Patches were landed on : 

Gaia/master : 1bd937c8c110788fcaec7deefe5f3367def18a42
Gaia/v1.3 : 27f6ac2b5591dace72951baebde2345422b6f1e5

Comment 18

[Julien Wajsberg [:julienw]]

I'm gonna back out this from 1.3, this broke integration tests, see https://travis-ci.org/mozilla-b2g/gaia/jobs/16922527:

 1) manipulate support settings check support with SIM custom "before each" hook:

JavaScriptError: (17) TypeError: mobileConnections is undefined

Remote Stack:

@app://system.gaiamobile.org/js/airplane_mode.js, line 282

at Error.MarionetteError (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/error.js:67:13)

2) lockscreen disabled test launch test app:

JavaScriptError: (17) TypeError: mobileConnections is undefined

Remote Stack:

@app://system.gaiamobile.org/js/airplane_mode.js, line 282

at Error.MarionetteError (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/error.js:67:13)


reverting from v1.3: 5a9cae9babced2482c4cb8d560a64b99b2007f44

Comment 19

[Julien Wajsberg [:julienw]]

Hey EJ, the integration tests issue make me think the patch needs to be adapted for v1.3, or a dependent patch is missing.

Needinfo you so that you can properly uplift this.

Comment 20

[Jason Smith [:jsmith]]

Actually, let's back this out on all branches. This also completely broke one of the Gaia UI Tests as well in bug 959691.

Comment 21

[Jason Smith [:jsmith]]

needinfo on jhford to backout the patch on master

Comment 22

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

@Jason, 

just reverted from master : 5ac56d4474e94ba49356d0ee2c071a84a2c6e7fe

It's kinda interesting, I noticed that we would skip some Gaia UI tests on Travis and that's why this case is not covered in this patch. I have a question that why don't we activate it on Travis (?)

@Julien, 

it means that we have no mozMobileConnections on v1.3 when running marionette test (?) Kinda weird, I will check what's wrong about these marionette tests on v1.3. 

Thanks.

Comment 23

[Julien Wajsberg [:julienw]]

(In reply to EJ Chen [:eragonj] from comment #22)
> @Jason, 
> 
> just reverted from master : 5ac56d4474e94ba49356d0ee2c071a84a2c6e7fe
> 
> It's kinda interesting, I noticed that we would skip some Gaia UI tests on
> Travis and that's why this case is not covered in this patch. I have a
> question that why don't we activate it on Travis (?)
> 
> @Julien, 
> 
> it means that we have no mozMobileConnections on v1.3 when running
> marionette test (?) Kinda weird, I will check what's wrong about these
> marionette tests on v1.3. 
> 
> Thanks.

I think mozMobileConnections is not available in B2G Desktop, but I'll needinfo Vicamo to be sure.

I also think that Zac disabled in Travis all the tests that run only on devices. Zac, am I right here?

Comment 24

[Zac C (:zac)]

Julienw, yes it is disabled because the Travis VM lacks the hardware the test needs. (If we ever mock it for desktopb2g then we could enable it).

Comment 25

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi all, we noticed that two failed tests which Julien talked about were already commented out on master at Bug 952611 - Disable broken gaia integration tests. We would do the same thing on v1.3 to disable these integration tests.

About mozMobileConntions is undefined problem, it is a known issue here (bug 951888). After discussion with Tim & Arthur, because the related error is hidden on TBPL, we think `undefined` issue is better to be handled independently on that bug instead of this one together. 

Thanks :)

Comment 26

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on v1.3

Comment 27

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on master

Comment 28

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

Comment on attachment 8360312 [details] [review]
patch on v1.3

Looks good to me.

Comment 29

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

Comment on attachment 8360313 [details] [review]
patch on master

Looks good to me.

Comment 30

[Florin Strugariu [:Bebe]]

Comment on attachment 8360313 [details] [review]
patch on master

the UI tests seem to work as expected when setting the AirPlane Mode

Comment 31

[Florin Strugariu [:Bebe]]

Comment on attachment 8360312 [details] [review]
patch on v1.3

tested this on 1.3 and it works as expected

Comment 32

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks Bebe !! :)

Comment 33

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all, just merged the code in 

Gaia/master : 8a562338f6ea381f6f1cdac2a544970c1a5b2093
Gaia/v1.3 : ffc82b03405b4878cee0b626a1160a8e884c9c05

:)

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Julien Wajsberg [:julienw] from comment #23)
> (In reply to EJ Chen [:eragonj] from comment #22)
> > @Julien, 
> > 
> > it means that we have no mozMobileConnections on v1.3 when running
> > marionette test (?) Kinda weird, I will check what's wrong about these
> > marionette tests on v1.3. 
> > 
> > Thanks.
> 
> I think mozMobileConnections is not available in B2G Desktop, but I'll
> needinfo Vicamo to be sure.

Kind of late now :( Based on bug 859554, we have all RIL APIs (but MobileMessage) undefined on platforms unsupported since bug 915604 and bug 920551.