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.
Created attachment 8541160 [details] [review] 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.
Attachment #8541160 - Flags: feedback?(timdream)
Oh, and I need to context-switch to blockers... (again)
Comment on attachment 8541160 [details] [review] WIP Patch OK I've updated the PR. The details is in the commit message.
(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.
(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 on attachment 8541160 [details] [review] WIP Patch Discussed offline extensively, we are getting close!
Attachment #8541160 - Flags: feedback?(timdream) → feedback+
Created attachment 8556368 [details] [review] 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 on attachment 8556368 [details] [review] Patch Getting close. Let's finish the review F2F on Monday.
Attachment #8556368 - Flags: review?(timdream) → feedback+
Since the try now is green, I set the review? https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=0e54c783af7b
Comment on attachment 8556368 [details] [review] Patch r+! Thanks for the patch! Please set seek QA for help on verifying the patch before landing.
Attachment #8556368 - Flags: review?(timdream) → review+
The try is green again: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=427af4d7e8d8
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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Hi Gerry, can you help to verify this? Thanks.
Flags: needinfo?(hochang) → needinfo?(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
Thank you Gerry :)
You need to log in before you can comment on or make changes to this bug.