960901 - (lockscreen-instance) [LockScreen] Make LockScreen as an instantiable function

Status: VERIFIED FIXED

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

Description

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

Current the LockScreen is a global object, which require many tricks in the unit test. We should uniform components like this to be instanced with the 'new' or other native supported methods, and this can help us to solve some resource reallocating issues.

Expected:

    lockScreen = new LockScreen(perquisites)

Actual:

    // perquisites prepared here..
    LockScreen.init()

Comment 1

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

I've found CallScreen's tricky side-effectful code..

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/call_screen.js#L158

It new the LockScreenSlide, but not use it. It works totally because the elements in the window would be handled by the slide only.
I don't think new a thing and not to use it, but interact it with events and DOMs is a good pattern. If we really no need to hold it, there should be a component like Factory to do this.

Comment 2

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

Attachment: Patch

This patch:

1. moved the singleton LockScreen into a instantiable function
2. fixed jshint errors
3. fixed related tests
4. fixed CallScreen for incoming call
5. use event-based mechanism to communicate the unlocker and the lockscreen(s)

This patch still got suffered from a failed test which performs different result on the Travis and localhost. However, because there're lots code got changed, I still set the review flag to let Tim comments and maybe I can fix them with the test at the same time.

Comment 3

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

OK, the patch finally passed all tests.

https://travis-ci.org/mozilla-b2g/gaia/builds/18257755

Comment 4

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

I've updated the patch again so it need to pass the tests again (waiting).

Comment 5

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

Travis green again:

https://travis-ci.org/mozilla-b2g/gaia/builds/18321404

Comment 6

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

Comment on attachment 8370528 [details] [review]
Patch

I would like to discuss with you some of the questions raised in Github offline.

Also, this is a rather bug change to the lock screen, and I am afraid our tests does not cover all of the interaction, so we should manually test the patch thoroughly before merge it to reduce as much risk as possible. Thanks!

Comment 7

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

Tim and I discussed this: we need QA to test this patch more detailed, and if there are no regressions we would land this on time.

Comment 8

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

Comment on attachment 8370528 [details] [review]
Patch

Fixed according to comments on GitHub. Set review again.

Comment 9

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

Travis was green: https://travis-ci.org/mozilla-b2g/gaia/builds/18568100

Comment 10

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

Comment on attachment 8370528 [details] [review]
Patch

Fixed failed tests on Travis.

I left commits to let reviewer to see the changes, even though some changes have been squashed once.

Comment 11

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

Travis was green!

https://travis-ci.org/mozilla-b2g/gaia/builds/18788476

I'm waiting and shivered with the fear of those tests after I merge these two commits.

Comment 12

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

Travis is green again (after squash)

https://travis-ci.org/mozilla-b2g/gaia/builds/18855352

master: https://github.com/mozilla-b2g/gaia/commit/efba9babeedbb0c9af9b7cb91a5630d7be2f5d8d

Let's to see if we would have any problem after landed.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Reverted for Gaia unit test failures on TBPL.
https://github.com/mozilla-b2g/gaia/commit/652883373f2c67c23dd2a4b742f5a1ad76031698

https://tbpl.mozilla.org/php/getParsedLog.php?id=34664556&tree=B2g-Inbound

Comment 14

[Kevin Grandon :kgrandon]

Seems like this might be another case of we don't know what order to run the tests in? Adding Julien.

Comment 15

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

I've added some conditions in several files to make

    lockScreen.locked

become

    window.lockScreen && window.lockScreen.locked

to prevent the logged issue happen again.

But I don't know why this won't happen on Travis. And now if problem likes happens frequently, the only way I can know to merge code and wait it been backout by the TBPL again...

Comment 16

[Kevin Grandon :kgrandon]

I believe you should use the new gaia try: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_a_custom_Gaia

Also I'm concerned about that method of the object detection. It seems you should just ensure that lockScreen is included instead of failing if it's not.

Comment 17

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

If the object is actually not included before these singleton objects got loaded, all of the tests should be failed (not the one it reported), and none of my local tests would be passed. So that's the point bothers me, and I still need to figure out why.

Comment 18

[Kevin Grandon :kgrandon]

The problem is test ordering is not the same on TBPL and travis/local. We are trying to solve this, but for now you need to ensure every test does proper startup/teardown. Unfortunately we don't yet do that either =(

I bet you can reproduce this by running each test for each file which references lockScreen individually locally (not the entire suite).

Comment 19

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

(In reply to Kevin Grandon :kgrandon from comment #18)
> The problem is test ordering is not the same on TBPL and travis/local. We
> are trying to solve this, but for now you need to ensure every test does
> proper startup/teardown. Unfortunately we don't yet do that either =(

Or maybe a sandbox like using iframe to run each test to gain greater isolation can solve this problem, too (for unit test, which now owns tests aren't isolated like UI test). I recently discussed this with some people offline, and there is already a related Bug 864178.

Comment 20

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

Fixed bugs, and tests was green again.

master: https://github.com/mozilla-b2g/gaia/commit/56ddd220f786916d596ea60ee170af0678d19838

Comment 21

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

It seems that I still missed one case, and fired a Bug 973501 to fix it.

Comment 22

[Julien Wajsberg [:julienw]]

TBPL is using B2G-Desktop, that's probably the reason for the failure here, and not the sandboxing thing.

Still we need both ;) Bug 864178 is the next item on my test-agent todo list.

Comment 23

[Julien Wajsberg [:julienw]]

After this change, running (for example) "APP=sms RUNS=1 make test-perf" does not disable lockscreen. This still work and give results though...

Are you sure you still obey the lockscreen.enabled and lockscreen.locked settings?

Greg, can you file a new bug to investigate this?

Comment 24

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

OK, I'll check this as soon as possible.
I didn't change any code about listening the settings, so I have no idea what happened yet.

Comment 25

[Marcus Cavanaugh [:mcav]]

This change has a typo that causes errors as reported by :gaye (on dev-gaia):

if (noMoreWorkCalback)  <<<<

https://github.com/mozilla-b2g/gaia/commit/56ddd220f786916d596ea60ee170af0678d19838#diff-04cef62435c1e1c73bf49157c6868956R123

Comment 26

[Julien Wajsberg [:julienw]]

Can someone do a quick fix or backout so that the tree can reopen?

Comment 27

[Kevin Grandon :kgrandon]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> Can someone do a quick fix or backout so that the tree can reopen?

I am not convinced that this is causing all of our problems today - though it may be causing a few. I've opened a pull request here and will see what travis does with it: https://github.com/mozilla-b2g/gaia/pull/16425

Comment 28

[Julien Wajsberg [:julienw]]

Quite clearly it caused the "noMoreWorkCalback" failure in marionette JS :) Thanks Kevin!

Comment 29

[Kevin Grandon :kgrandon]

Attachment: Pull Request - follow up to fix test

Comment 30

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

(In reply to Marcus Cavanaugh [:mcav] <[e-mail]> from comment #25)
> This change has a typo that causes errors as reported by :gaye (on dev-gaia):
> 
> if (noMoreWorkCalback)  <<<<
> 
> https://github.com/mozilla-b2g/gaia/commit/
> 56ddd220f786916d596ea60ee170af0678d19838#diff-
> 04cef62435c1e1c73bf49157c6868956R123

Ahhh, I'm very sorry for that.

Comment 31

[Kevin Grandon :kgrandon]

Comment on attachment 8377928 [details] [review]
Pull Request - follow up to fix test

Review from :johu in bug 972651 carries over.

Comment 32

[Kevin Grandon :kgrandon]

Landed a follow-up instead of backing this out: https://github.com/mozilla-b2g/gaia/commit/70a96fb38ae28fab1810cac4da17c20e23c86448

Just hope we don't have to back this out.

Comment 33

[Hubert Figuiere [:hub]]

Did you run |make test-perf| ?

Comment 34

[Hubert Figuiere [:hub]]

forget my question. I was afraid it would break the test-perf. It is work OK.

Comment 35

[William Hsu [:whsu]]

Thanks Greg!

No regression bug after 2 full run test.

* V1.4 Buri fullrun-1: 
  - http://goo.gl/XAuAEs
* V1.4 Buri fullrun-2: 
  - http://goo.gl/jhih0h