929388 - Incorporate a FxA Manager and FxA Client into the System app

Status: RESOLVED FIXED

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

Comment 1

[Sam Penrose]

Attachment: Initial sketch of the IAC API between FTU and System

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Now with web workers

Comment 5

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: IAC API version 2

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Please, ignore the huge bundle.js file for now. It is there this way only for testing purposes.

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: IAC and Client APIs

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko dummy responder

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: fxamanager.patch

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko dummy responder

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Attachment: IAC and Client APIs

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: IAC and Client APIs

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Attachment: fxamanager.patch

Now the good one...

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Attachment: IAC and Client APIs

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

I am pushing the changes to https://github.com/ferjm/gaia/tree/bug929388.fxamanager

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia Pull Request

Comment 22

[Borja Salguero [:borjasalguero]]

Comment on attachment 8339148 [details] [review]
Gaia Pull Request

Hi Alive! This is the WIP patch that I showed you yesterday. Could you take a look? Thanks a lot!! 谢谢! :)

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

Haven't finished reading everything yet.
Observed architecture to me:
https://docs.google.com/drawings/d/1oztuLKf6ReiiFk97VmztuGWTp_hHe8NCAHq-5h-dmuE/edit?usp=sharing

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Attachment: High level architecture

Thanks Alive.

This is the higher level picture of the entire FxA system.

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8339148 [details] [review]
Gaia Pull Request

Overall it's fine but lots of coding style nits and unit tests seem unhappy.

I would suggest to decoupling module dependency by event publish/subscribe instead of calling directly. Lots of FxAccountXXXXX.yyyyyy make it hard to read. But it's up to you.

About the manifestURLs, I won't insist on checking the caller in this way but we have better to ask on the maillist: how to use IAC to identify the one who is connecting? Is this a proper way to go?

Comment 26

[Borja Salguero [:borjasalguero]]

Comment on attachment 8339148 [details] [review]
Gaia Pull Request

Hi Alive! All your comments addressed, and now patches needed in Gecko are landed, So this feature is ready to be reviewed! r? Thanks! 谢谢!

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8339148 [details] [review]
Gaia Pull Request

This looks great! :)

I added only some minor comments. I'd like to see a final version though. 

Thanks!

Comment 28

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8339148 [details] [review]
Gaia Pull Request

I have no bandwidth to review it this week, but I think :ferjm's review is enough.
If there's a final version before I am OOO I'd try to give feedback. Thanks!

Comment 29

[Borja Salguero [:borjasalguero]]

Comment on attachment 8339148 [details] [review]
Gaia Pull Request

Francisco! Could you take a look? Thanks!

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8339148 [details] [review]
Gaia Pull Request

r=me with the comments addressed, please.

Also squash the commits and add r=ferjm, not r=fjimenez :P

Comment 31

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8339148 [details] [review]
Gaia Pull Request

r+ once fixed some minor comments regarding documentation and nits.

Also please apply the changes that Fernando requested, not all of them are applied despite that there are comments saying so, perhaps some files were not updated?

Amazing job from everyone involved. Incredible.

Thanks folks!

Comment 32

[Francisco Jordano [:arcturus] [:francisco]]

Hi Fernando,

I'm flagging you as needinfo cause of my comment here:
https://github.com/mozilla-b2g/gaia/pull/14101/files#diff-b32b94ec058b67e99347f7a66a394c87R107

I'm a bit concern about possible security leaks, if we leave in the system app access to a test application that won't be installed on the phone.

IMHO, we should remove it.
F.

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

This is actually true! However, it is a shame that we can't have a test app or UI tests because of this. I would prefer to add an extra rule to also limit the connection to certified apps only instead of getting rid of the test app. What do you think?

Comment 34

[Francisco Jordano [:arcturus] [:francisco]]

Right, that sounds to me like the perfect solution.

Thanks Fernando!

Comment 35

[Fernando Jiménez Moreno [:ferjm]]

Great, thanks! I've just sent a PR with that change and a fix for the unit tests.

Comment 36

[Borja Salguero [:borjasalguero]]

Travis Green, Feedback&R+, Merge this!