1043892 - [LockScreen] Solve the visual regression caused by Bug 1043821, with panel switching refactoring

Status: RESOLVED FIXED

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

Description

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

In the progress of resolving Bug 1043821, I've found the switchPanel and it's use case is too complicated, which blocks me to resolve the visual regression caused by the patch. So after discussed with Tim, I decide to solve the regression in this follow-up bug, and maybe with some refactoring work.

The visual regression is if user unlock the screen with passcode entered, and then lock it again, the passcode pad would play the slide out animation again. I've found this is relevant with the switchPanel, but in the current code it scatters about the whole lockscreen.js, which make it's hard to debug.

Comment 1

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

I almost resolved this bug. One discover is if we don't hide the slide while unlocking with passcode, it looks more smooth. The video is attached:

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

NI UX to see if this minor change is OK.

Comment 2

[Stephany Wilkes]

Flagging Peter to check the UX change.

Comment 3

[Peter La]

Hi Greg,

Can you provide a video of the actual regression from bug 1043821?  It's a bit hard to visualize.

Thanks,
Peter

Comment 4

[Peter La]

Hi Greg,

Nevermind, I think I see the regression now on my device.  As for your minor change - I think I like your new version better because there is less of a delay (the first option shown in your video).

Peter.

Comment 5

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

Attachment: Patch

Major changes:

1. Pull out panel switching code to individual state components
2. Manage these states with specific rules in LockScreenStateManager
3. Modify the existing components as less as possible to reduce risks

TBPL only failed at building stage, which failed at others' sessions as well. So I set the review before I re-run it.

Comment 6

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

Comment on attachment 8468164 [details] [review]
Patch

LFTM as discussed offline.

This patch implements a state machine to manage lock screen panel states. The state machine is also the centralize place do decide the next state should go to. I am not very opinionated on what state machine should look like and I think the amount of creativity and diversity is good.

There is also a queue module that handles the events with setInterval. I never saw a queue being implemented as such but I failed to think of any down side of it. So let's get this checked-in and see how this goes.

Nice work!

Comment 7

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

The test only failed at two intermittent error:

https://tbpl.mozilla.org/?rev=3b20fe7c78c53d5fb6e7ea33b73384ffb090815f&tree=Gaia-Try

So I would land this patch. Thanks Tim for the reviewing and feedbacks.

Comment 8

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

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

Comment 9

[Ed Morley [:emorley]]

Reverted on suspicion of causing debug B2G ICS emulator xpcshell failures like:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45731843&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45732695&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45733764&tree=B2g-Inbound

https://github.com/mozilla-b2g/gaia/commit/894586458443d1f235dc3a315f4dacc282864abb

Comment 10

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

Attachment: Patch rev2

Removed the event queue to make sure there is no potential risk to land this patch. While I think it's very strange to fail only one test in Gonk on emulator with one Gaia patch, which looks should be innocent except it's in the System app.

Comment 11

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

The new patch failed with two test cases only on Gaia Try: they passed on my local console perfectly.
So I submit a bug to require a machine to debug with: Bug 1053688

Comment 12

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

Brief update: test with |NOFTU=0 make| would make it fail on both Mac and Ubuntu locally. |NOFTU=1 make| would make them pass, while on remote server with only |make| always failed.

Comment 13

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

Yes. On the remote server, |NOFTU=1 make| would lead to fail, too.

Comment 14

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

Oh no... now |NOFTU=0 make| on Mac would lead to the success result again.

Comment 15

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

Okay, it failed only with intermittent errors:

https://tbpl.mozilla.org/?rev=3c10b2a9da487d2ef7f202918bc912396eabf80e&tree=Gaia-Try

So I would land it again.

Comment 16

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

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

To see if the RIL and other errors occur again

Comment 17

[Ed Morley [:emorley]]

Reverted for the test failures that were also present in the gaia-try run:
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=c0dc6b070008&jobname=gaia-ui-test

https://github.com/mozilla-b2g/gaia/commit/0074e17d02e488049502a28789135afed3d4e3d1

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #15)
> Okay, it failed only with intermittent errors:

Those were not intermittent failures - they didn't match the bug suggestions.

Comment 18

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

Attachment: Patch rev3

...start to feel frustrated to fix tests neither written nor maintain by me.

Comment 19

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

Maybe I should disable the test and NI the author who wrote and maintain the test to update it next time.

Comment 20

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

While I'm fixing the rest integration test failure, I've found the previous Gonk RIL test failed even without my patch, which caused the first back out. Although the patch indeed should be fixed with other errors in Gaia UI tests.

NI for notifying Ed the errors:

Gaia change with the RIL failure #1
https://hg.mozilla.org/integration/b2g-inbound/rev/c6bedead82d8

which was with the RIL error:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46354439&tree=B2g-Inbound

Gaia change with the RIL failure #2
https://hg.mozilla.org/integration/b2g-inbound/rev/15b30f6faca9

which was with the RIL error:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46352648&tree=B2g-Inbound

These samples show that the RIL error on ICS emulator is irrelevant with my patch. Because these failures were all occurs before my patch got landed.

Comment 21

[Ed Morley [:emorley]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #18)
> ...start to feel frustrated to fix tests neither written nor maintain by me.

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #19)
> Maybe I should disable the test and NI the author who wrote and maintain the
> test to update it next time.

That's unfortunately not how development works at Mozilla - if a landing breaks expectations of tests, then it needs to adjust the tests at the same time.

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #20)
> While I'm fixing the rest integration test failure, I've found the previous
> Gonk RIL test failed even without my patch, which caused the first back out.
> Although the patch indeed should be fixed with other errors in Gaia UI tests.
> 
> NI for notifying Ed the errors:
> 
> Gaia change with the RIL failure #1
> https://hg.mozilla.org/integration/b2g-inbound/rev/c6bedead82d8
> 
> which was with the RIL error:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=46354439&tree=B2g-Inbound
> 
> Gaia change with the RIL failure #2
> https://hg.mozilla.org/integration/b2g-inbound/rev/15b30f6faca9
> 
> which was with the RIL error:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=46352648&tree=B2g-Inbound
> 
> These samples show that the RIL error on ICS emulator is irrelevant with my
> patch. Because these failures were all occurs before my patch got landed.

I don't quite understand what you are saying? The xpcshell failures listed in comment 20 were due to bug 1052503.

Comment 22

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

Okay, the patch now pass all tests on Gaia-Try:

https://tbpl.mozilla.org/?rev=ac025d7709ef815cab792c12c1f932a641c6125a&tree=Gaia-Try

Hope everything goes fine on Inbound as well.

Comment 23

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

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

Comment 24

[KTucker [:KTucker][Inactive 3/4/2016]]

Not closing this as verified fixed since this issue is still occurring when locking the device in the settings app.

bug 1064630