Closed Bug 1168160 Opened 5 years ago Closed 4 years ago

Sync state machine

Categories

(Firefox OS Graveyard :: Sync, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.5+)

RESOLVED FIXED
FxOS-S7 (18Sep)
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
Blocks: fxos-sync
Component: General → Sync
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: 5 years ago
Resolution: --- → WONTFIX
We reopen this bug for implementing Sync State Machine in system app.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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: nobody → ferjmoreno
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)
Attachment #8654740 - Flags: feedback?(selee)
Great! Beautiful code as well, nice to learn from. :) I left some comments on github.
Attachment #8654740 - Flags: feedback?(mbdejong) → feedback+
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)
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?
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
Attachment #8654740 - Flags: review?(etienne)
Attachment #8654740 - Flags: review?(etienne)
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: awu → ferjmoreno
(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.
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 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+
https://github.com/mozilla-b2g/gaia/commit/94c1318a6b24a00c9f0a1df4643327b5d55c4589
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.