Closed Bug 960901 (lockscreen-instance) Opened 6 years ago Closed 6 years ago

[LockScreen] Make LockScreen as an instantiable function


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

Gonk (Firefox OS)
Not set


(b2g-v1.4 verified)

Tracking Status
b2g-v1.4 --- verified


(Reporter: gweng, Assigned: gweng)



(Whiteboard: [c= p= s= u=] [ft:system-platform])


(2 files)

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.


    lockScreen = new LockScreen(perquisites)


    // perquisites prepared here..
Assignee: nobody → gweng
Alias: lockscreen-instance
I've found CallScreen's tricky side-effectful code..

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.
Attached file 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.
Attachment #8370528 - Flags: review?(timdream)
OK, the patch finally passed all tests.
I've updated the patch again so it need to pass the tests again (waiting).
Comment on attachment 8370528 [details] [review]

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!
Attachment #8370528 - Flags: review?(timdream) → feedback+
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 on attachment 8370528 [details] [review]

Fixed according to comments on GitHub. Set review again.
Attachment #8370528 - Flags: feedback+ → review?(timdream)
Attachment #8370528 - Flags: review?(timdream) → review+
Comment on attachment 8370528 [details] [review]

Fixed failed tests on Travis.

I left commits to let reviewer to see the changes, even though some changes have been squashed once.
Attachment #8370528 - Flags: review+ → review?(timdream)
Travis was green!

I'm waiting and shivered with the fear of those tests after I merge these two commits.
Attachment #8370528 - Flags: review?(timdream) → review+
Travis is green again (after squash)


Let's to see if we would have any problem after landed.
Closed: 6 years ago
Resolution: --- → FIXED
Seems like this might be another case of we don't know what order to run the tests in? Adding Julien.
I've added some conditions in several files to make



    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...
I believe you should use the new gaia try:

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.
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.
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).
(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.
Fixed bugs, and tests was green again.

Closed: 6 years ago6 years ago
Resolution: --- → FIXED
It seems that I still missed one case, and fired a Bug 973501 to fix it.
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.
Depends on: 973501
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?
Flags: needinfo?(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.
Flags: needinfo?(gweng)
Depends on: 973768
Whiteboard: [c= p= s= u=] [ft:system-platform]
This change has a typo that causes errors as reported by :gaye (on dev-gaia):

if (noMoreWorkCalback)  <<<<
Can someone do a quick fix or backout so that the tree can reopen?
(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:
Quite clearly it caused the "noMoreWorkCalback" failure in marionette JS :) Thanks Kevin!
(In reply to Marcus Cavanaugh [:mcav] <> from comment #25)
> This change has a typo that causes errors as reported by :gaye (on dev-gaia):
> if (noMoreWorkCalback)  <<<<
> 56ddd220f786916d596ea60ee170af0678d19838#diff-
> 04cef62435c1e1c73bf49157c6868956R123

Ahhh, I'm very sorry for that.
Comment on attachment 8377928 [details] [review]
Pull Request - follow up to fix test

Review from :johu in bug 972651 carries over.
Attachment #8377928 - Flags: review+
Landed a follow-up instead of backing this out:

Just hope we don't have to back this out.
Blocks: 956662
Did you run |make test-perf| ?
Flags: needinfo?(gweng)
forget my question. I was afraid it would break the test-perf. It is work OK.
Flags: needinfo?(gweng)
Thanks Greg!

No regression bug after 2 full run test.

* V1.4 Buri fullrun-1: 
* V1.4 Buri fullrun-2: 
You need to log in before you can comment on or make changes to this bug.