1168160 - Sync state machine

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 2

[Sean Lee [:seanlee][:weilonge]]

We reopen this bug for implementing Sync State Machine in system app.

Comment 3

[Michiel de Jong [:michielbdejong]]

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?).

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] ferjm:bug1168160.statemachine > mozilla-b2g:master

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 6

[Michiel de Jong [:michielbdejong]]

Great! Beautiful code as well, nice to learn from. :) I left some comments on github.

Comment 7

[Etienne Segonzac (:etienne)]

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

Comment 8

[Richard Newman [:rnewman]]

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?

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 10

[Sean Lee [:seanlee][:weilonge]]

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 )

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

(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 12

[Fernando Jiménez Moreno [:ferjm]]

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 :(

Comment 13

[Etienne Segonzac (: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 :)

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/94c1318a6b24a00c9f0a1df4643327b5d55c4589