Closed Bug 1210697 Opened 4 years ago Closed 4 years ago

[smart-system] Add SyncManager and SyncStateMachine

Categories

(Firefox OS Graveyard :: Gaia::TV::System, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(feature-b2g:2.5+)

RESOLVED FIXED
FxOS-S9 (16Oct)
feature-b2g 2.5+

People

(Reporter: yifan, Assigned: ferjm)

References

Details

(Whiteboard: [ft:conndevices][partner-blocker][partner-cherry-picked<2015/11/10>])

User Story

As Mozilla, I want to back port Data sync mechanism from smart phone to TV

Attachments

(2 files)

Add SyncManager and SyncStateMachine from apps/system to tv_apps/smart-system.
No longer blocks: fxos-sync
No longer depends on: 1202382, 1207521, 1191773
Comment on attachment 8668831 [details] [review]
[gaia] begeeben:1210697_add_syncmanager > mozilla-b2g:master

Hi Sean & Fernando, could you please help to review this patch? Thanks!
Attachment #8668831 - Flags: review?(selee)
Attachment #8668831 - Flags: review?(ferjmoreno)
Assignee: nobody → yliao
Status: NEW → ASSIGNED
Blocks: 1207521
Target Milestone: --- → FxOS-S9 (16Oct)
feature-b2g: --- → 2.5+
Comment on attachment 8668831 [details] [review]
[gaia] begeeben:1210697_add_syncmanager > mozilla-b2g:master

Thank you YiFan. Looks good, however we need bug 1182071 and bug 1202342 to make your patch work properly. I'll file a bug for that. I left some comments on the PR I'd like you to address or answer before r+ing the patch.

Thank you for all the work you are doing with FxA/Sync!
Attachment #8668831 - Flags: review?(ferjmoreno) → feedback+
Comment on attachment 8668831 [details] [review]
[gaia] begeeben:1210697_add_syncmanager > mozilla-b2g:master

Hi Yifan,

LGTM :)
Attachment #8668831 - Flags: review?(selee) → feedback+
Depends on: 1211367
BTW, we also need to make use of the preprocessor here to enable/disable Sync. Check bug 1191770 and bug 1202383.
Depends on: 1211376
Component: Gaia::TV → Gaia::TV::System
Depends on: 1211767
Blocks: fxos-sync
See Also: → 1211833
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> BTW, we also need to make use of the preprocessor here to enable/disable
> Sync. Check bug 1191770 and bug 1202383.

Thank you! Opened Bug 1211833 - Merge preprocessing functionality to TV to enable/disable sync to follow up.
Comment on attachment 8668831 [details] [review]
[gaia] begeeben:1210697_add_syncmanager > mozilla-b2g:master

The patch and comments were updated. Please help to review it, thank you!
Attachment #8668831 - Flags: review?(ferjmoreno)
Comment on attachment 8668831 [details] [review]
[gaia] begeeben:1210697_add_syncmanager > mozilla-b2g:master

Thank you YiFan. I am afraid that this still doesn't work. If you want to test the patch, you can do it by adding the test_sync app from [1] to your build.

I suspect that what I am seeing is a BaseModule issue. 

What I see is that the state never changes in the Sync state machine. When we query the state via Service.query('StateMachine.state') we always get 'disabled'.

And I see this on the logcat:

ObjectActor.prototype.grip previewer function threw an exception: [Exception... "Factory not registered"  nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame :: app://smart-system.gaiamobile.org/js/service.js :: exports.Service.query :: line 195"  data: no]
Stack: exports.Service.query@app://smart-system.gaiamobile.org/js/service.js:195:9
.updateState@app://smart-system.gaiamobile.org/js/sync_manager.js:410:29
._handle_onsyncenabling@app://smart-system.gaiamobile.org/js/sync_manager.js:261:7
EventMixin.handleEvent@app://smart-system.gaiamobile.org/js/base_module.js:344:9
BaseModule.prototype.publish@app://smart-system.gaiamobile.org/js/base_module.js:597:7
._createTransition/<@app://smart-system.gaiamobile.org/js/sync_state_machine.js:140:1
exports.Service.request/<@app://smart-system.gaiamobile.org/js/service.js:58:21
exports.Service.request@app://smart-system.gaiamobile.org/js/service.js:57:18
["_handle_iac-gaia-sync-management"]@app://smart-system.gaiamobile.org/js/sync_manager.js:207:13
EventMixin.handleEvent@app://smart-system.gaiamobile.org/js/base_module.js:344:9
onReceivedMessage@app://smart-system.gaiamobile.org/shared/js/iac_handler.js:39:11

Could you take a look please?

Maybe it is easier to move away from BaseModule instead of fixing BaseModule itself.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8661685
Attachment #8668831 - Flags: review?(ferjmoreno) → review-
Thank you! Tried and ended up with another error message with BaseModule in service.js, maybe I did something wrong running the test apps. 

Anyway after offline discussion with Evelyn and Luke, we agree that we probably should move away from BaseModule in this case. Are you OK to provide a patch? It will be very appreciated!
Flags: needinfo?(ferjmoreno)
Sure. Moving away from BaseModule sounds good. Thanks.
Assignee: yliao → ferjmoreno
Flags: needinfo?(ferjmoreno)
Attachment #8672760 - Flags: review?(yliao)
Attachment #8672760 - Flags: review?(selee)
Comment on attachment 8672760 [details] [review]
[gaia] ferjm:bug1210697.syncsystemtv > mozilla-b2g:master

Thank you very much Fernando! That is quite some changes. LGTM!
Attachment #8672760 - Flags: review?(yliao) → review+
Comment on attachment 8672760 [details] [review]
[gaia] ferjm:bug1210697.syncsystemtv > mozilla-b2g:master

LGTM! and I would like to give f+ here :)
Attachment #8672760 - Flags: review?(selee) → feedback+
https://github.com/mozilla-b2g/gaia/commit/cef6f553ec8440a31d0497ea3a690b27860b3259
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [ft:conndevices][partner-blocker][partner-cherry-pick] → [ft:conndevices][partner-blocker][partner-cherry-picked<2015/11/10>]
You need to log in before you can comment on or make changes to this bug.