1115311 - [LockScreen] (State-Component) Land Clock widget in the current codebase

Status: RESOLVED FIXED

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

Description

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

As one step of Bug 1101418, we can land some isolated components with the basic facilities. I now have a WIP patch can replace the old 'Clock' on LockScreen, with the new State-Component.

Comment 1

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

Attachment: WIP Patch

Tim: this is an almost completed clock widget with our new facilities (stream & process). "Some" changes after our last discussion:

1. Add "Source" constructor for Steam, to avoid Component need to manipulate how to adapt native API into event-style. It now can convert the mozSettings and timer changes into "event", so that Stream and Component needn't to know that.

2. After initialization, the parent component can't hold the real state the child would transfer to (since states only know its next state). That is, parent can't wait children's asynchronous operations done anymore. So I add a 'getActiveState' in LockScreenBasicComponent to let parent to access the current active state. The use case in this patch is in lockscreen.js#unlock function, which needs to halt the clock.

3. For demo I add some log in LockScreenClockWidgetTick, and make the interval faster than 1 minute. You can try to press the powerbutton or to unlock the screen to see the log changes.

4. I added 'elementsAs' and the similar functions to extends the states which the current one want to pass to the next State. This is different from the constructor, since with constructor arguments would overwrite the new State's default properties with the arguments.

We may need another discussions for the details. So, if you have any opinions please let me know. Thanks.

Comment 2

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

Oh, and I need to context-switch to blockers... (again)

Comment 3

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

Comment on attachment 8541160 [details] [review]
WIP Patch

OK I've updated the PR. The details is in the commit message.

Comment 4

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

(log) I've found one nasty thing in the architecture: since 'transferTo' of State must not be queued or we couldn't interrupt the process to do the transferring, if parent component commanded its child hurriedly transfers to another state before the child is started, we get an intermittent error. One way to solve this is that parent component need to check if the child is ready, but I think to force the parent to put its commands after child's ready queue is better. We also could specialize this queue as a parent command queue, so that every component would actually have two queues: one is for the events it's interested in, another is for parent commands.

Comment 5

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

(log) However, I've found that I couldn't solve the issue since currently, only 'event' can be handled at arbitrary moments and concated to the Stream. Other methods, can only allow user access the queue once, unless the user store the returned queue, which is a bad behavior. So I think maybe to follow the existing 'event' style to let child queue the commands from parent, and let parent command the child via an method-calling interface would be better.

Comment 6

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

Comment on attachment 8541160 [details] [review]
WIP Patch

Discussed offline extensively, we are getting close!

Comment 7

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

Attachment: Patch

Tim: this looks well on my device, and I modified it as we discussed. The patch is now waiting Try result to make sure it didn't break any automation test.

Changes:
    * Clock now is a widget.
    * Component is the common resource keeper, while State is the resource
      controller.
    * No more time-varying if...else. Compose states & components instead.
    * Process sequentialize the async & sync operations, while Stream add
      the ability to handle events & interrupts on it.
    * New abstraction layer (Source) for a uniform way to collect events
      from timer, clock, mozSettings and DOM events.

Comment 8

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

Comment on attachment 8556368 [details] [review]
Patch

Getting close. Let's finish the review F2F on Monday.

Comment 9

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

Since the try now is green, I set the review?

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=0e54c783af7b

Comment 11

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

Comment on attachment 8556368 [details] [review]
Patch

r+! Thanks for the patch! Please set seek QA for help on verifying the patch before landing.

Comment 12

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

The try is green again: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=427af4d7e8d8

Comment 13

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

I've fixed the nits and the try is green, so I would land it...and let's see if there is any problem.

Also ask Howie for QA resource as we discussed offline.

Comment 14

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

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

Comment 15

[howie [:howie]]

Hi Gerry, can you help to verify this? Thanks.

Comment 16

[Gerry Chang [:gchang]]

I've tested on below build without big issues.
1. 12/24 time format.
2. Change to different timezone/date/time/language.
3. Change time/timezone in FTU.

Gaia-Rev        c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/0b3c520002ad
Build-ID        20150302160230
Version         39.0a1
Device-Name     flame
FW-Release      4.4.2

Comment 17

[howie [:howie]]

Thank you Gerry :)