Closed
Bug 1168160
Opened 10 years ago
Closed 9 years ago
Sync state machine
Categories
(Firefox OS Graveyard :: Sync, defect)
Tracking
(blocking-b2g:2.5+)
People
(Reporter: ferjm, Assigned: ferjm)
References
Details
Attachments
(1 file)
We need to create a module that handles all the sync states logic.
The iOS implementation seems like a good start [1]
[1] https://github.com/mozilla/firefox-ios/blob/master/Sync/SyncStateMachine.swift
Assignee | ||
Updated•10 years ago
|
Component: General → Sync
Assignee | ||
Comment 1•9 years ago
|
||
Instead of directly implementing a Sync client for FxOS, we are going to use a Kinto based service [1] to store FxOS related data and this service will handle the synchronization with the Firefox Sync servers. So we won't don't need this piece for now.
[1] https://wiki.mozilla.org/Firefox_OS/Syncto
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Comment 2•9 years ago
|
||
We reopen this bug for implementing Sync State Machine in system app.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Comment 3•9 years ago
|
||
Note from today's meeting: let's see who has time for this during sprint 6 (@seanlee? @ferjm? maybe split the system-app modifications into smaller parts?).
Target Milestone: --- → FxOS-S6 (04Sep)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ferjmoreno
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8654740 [details] [review]
[gaia] ferjm:bug1168160.statemachine > mozilla-b2g:master
This is the first piece of the Sync module that will be part of the System app. It basically handles the possible states where Sync can be. On this patch I implemented a very basic finite state machine that transitions between states as response to external events and triggers onSTATE events on the SyncStateMachine object when transitioned to a state named STATE. It can be extended to trigger additional events, but for now I think this is enough for our needs.
SyncStateMachine will be used by the SyncManager module that I'll be adding in bug 1196096.
Thanks in advance for any feedback.
Attachment #8654740 -
Flags: review?(etienne)
Attachment #8654740 -
Flags: feedback?(mbdejong)
Assignee | ||
Updated•9 years ago
|
Attachment #8654740 -
Flags: feedback?(selee)
Comment 6•9 years ago
|
||
Great! Beautiful code as well, nice to learn from. :) I left some comments on github.
Updated•9 years ago
|
Attachment #8654740 -
Flags: feedback?(mbdejong) → feedback+
Comment 7•9 years ago
|
||
Comment on attachment 8654740 [details] [review]
[gaia] ferjm:bug1168160.statemachine > mozilla-b2g:master
* Super happy to see sync stuff landing :)
* Thank you for dividing the work into manageable size patches!
* This is a new system module, so it needs to follow the new module guidelines [1].
It's not a tiny change (:/) but consistency is key in an app the size of the system app. And I know everybody's not a big fan of the BaseModule design but it's really starting to pay off (and could enable us to do much more cool stuffs in the future)!
I'd say the minimum would be to create from BaseModule (so no more singleton), implement a start/stop and expose "current" through the base module STATES [2].
But ideally enable/disable/sync should be SERVICES [3] and the state transition events would be |.publish()|'ed [4].
I'm on IRC to talk about it if needed, cheers!
[1] https://wiki.mozilla.org/Gaia/System/NewModule
[2] https://github.com/mozilla-b2g/gaia/blob/805025801a68f9ddbba6ffd2ed3926c97fa7fcc8/apps/system/js/base_module.js#L137
[3] https://github.com/mozilla-b2g/gaia/blob/805025801a68f9ddbba6ffd2ed3926c97fa7fcc8/apps/system/js/base_module.js#L116
[4] https://github.com/mozilla-b2g/gaia/blob/805025801a68f9ddbba6ffd2ed3926c97fa7fcc8/apps/system/js/base_module.js#L587
Attachment #8654740 -
Flags: review?(etienne)
Comment 8•9 years ago
|
||
ferjm: this doesn't look like it covers most of the states I'd expect a Sync client to be in. Is this just a stub?
Assignee | ||
Comment 9•9 years ago
|
||
Thanks for the feedback folks :) I'll work on your suggestions and update the PR shortly.
(In reply to Richard Newman [:rnewman] from comment #8)
> ferjm: this doesn't look like it covers most of the states I'd expect a Sync
> client to be in. Is this just a stub?
These are the high level states for the Sync feature on FxOS (i.e. they represent the states that can be presented to the user via the Settings app). The specifics of the Sync client are being implemented at Bug 1196239. Also, note that this won't be a regular Sync client, but a Syncto [1][2] one. For FxOS the high level architecture is like: kinto.js (FxOS) <-> Syncto (remote) <-> Sync (remote).
[1] https://wiki.mozilla.org/Firefox_OS/Syncto
[2] https://github.com/mozilla-services/syncto
Assignee | ||
Updated•9 years ago
|
Attachment #8654740 -
Flags: review?(etienne)
Assignee | ||
Updated•9 years ago
|
Attachment #8654740 -
Flags: review?(etienne)
Comment 10•9 years ago
|
||
Comment on attachment 8654740 [details] [review]
[gaia] ferjm:bug1168160.statemachine > mozilla-b2g:master
LGTM!
Sync State Machine is concise and explicit! I am very glad to give a feedback here. :)
IMO, this state machine design is very general. Could we extract any Sync related codes out of sync_state_machine.js?
The first idea comes to my mind is to implement a StateMachineFactory for creating a general state machine with Sync related arguments.
https://github.com/weilonge/gaia/commit/b357d74505fb18026a63b8d756d5fdb71a15fb59
(The indent of BaseModule.create block is incorrect. that's for readability of the patch. :P )
Attachment #8654740 -
Flags: feedback?(selee) → feedback+
Assignee: ferjmoreno → awu
blocking-b2g: --- → 2.5+
Target Milestone: FxOS-S6 (04Sep) → FxOS-S7 (18Sep)
Assignee | ||
Updated•9 years ago
|
Assignee: awu → ferjmoreno
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Sean Lee [:seanlee] from comment #10)
> Comment on attachment 8654740 [details] [review]
> [gaia] ferjm:bug1168160.statemachine > mozilla-b2g:master
>
> LGTM!
>
> Sync State Machine is concise and explicit! I am very glad to give a
> feedback here. :)
>
> IMO, this state machine design is very general. Could we extract any Sync
> related codes out of sync_state_machine.js?
>
> The first idea comes to my mind is to implement a StateMachineFactory for
> creating a general state machine with Sync related arguments.
>
> https://github.com/weilonge/gaia/commit/
> b357d74505fb18026a63b8d756d5fdb71a15fb59
>
> (The indent of BaseModule.create block is incorrect. that's for readability
> of the patch. :P )
Thank you for your suggestion Sean. I've been thinking about it and that's a good idea. But it's only useful if there's more than one module requiring a state machine as simple as this one, which is currently not the case. For now, since the only one needing this is Sync, I'll prefer to keep it as simple as it.
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8654740 [details] [review]
[gaia] ferjm:bug1168160.statemachine > mozilla-b2g:master
Ugh, I thought I set the review flag on Etienne a few days ago :(
Attachment #8654740 -
Flags: review?(etienne)
Comment 13•9 years ago
|
||
Comment on attachment 8654740 [details] [review]
[gaia] ferjm:bug1168160.statemachine > mozilla-b2g:master
Awesome! Thanks for being a good system app citizen!
You have a unit test that is failing, but assuming it is a small fix r=me :)
Attachment #8654740 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 14•9 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•