Closed Bug 929388 Opened 11 years ago Closed 11 years ago

Incorporate a FxA Manager and FxA Client into the System app

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::System
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jedp, Assigned: ferjm)

References

Details

(Whiteboard: [qa+])

Attachments

(3 files, 18 obsolete files)

IAC and Client APIs
(deleted), text/plain
Details
Gaia Pull Request
(deleted), text/x-github-pull-request
arcturus
: review+
ferjm
: review+
alive
: feedback+
ferjm
: feedback+
Details
High level architecture
(deleted), image/png
Details
No description provided.
Depends on: 929389
Depends on: 929395
Depends on: 929407
Depends on: 929411
Blocks: fxos-accounts
No longer blocks: 905637
Assignee: nobody → ferjmoreno
Attached file Initial sketch of the IAC API between FTU and System (obsolete) (deleted) —
Attached patch WIP (obsolete) (deleted) — — Splinter Review
Whiteboard: [qa?]
Attached patch WIP (obsolete) (deleted) — — Splinter Review
Attachment #820475 - Attachment is obsolete: true
Summary: Incorporate a FXA Manager into the System app → Incorporate a FxA Manager and FxA Client into the System app
Attached patch WIP (obsolete) (deleted) — — Splinter Review
Now with web workers
Attachment #820987 - Attachment is obsolete: true
Blocks: 930074
No longer blocks: 930074
Depends on: 930074
Attached file IAC API version 2 (obsolete) (deleted) —
Attachment #820428 - Attachment is obsolete: true
Attached patch WIP (obsolete) (deleted) — — Splinter Review
Please, ignore the huge bundle.js file for now. It is there this way only for testing purposes.
Attachment #821041 - Attachment is obsolete: true
Depends on: 930598
No longer depends on: 793911
Attached patch WIP (obsolete) (deleted) — — Splinter Review
Attachment #821654 - Attachment is obsolete: true
Attached file IAC and Client APIs (obsolete) (deleted) —
Attachment #821557 - Attachment is obsolete: true
Attached patch Gecko dummy responder (obsolete) (deleted) — — Splinter Review
Attached patch fxamanager.patch (obsolete) (deleted) — — Splinter Review
Attachment #823269 - Attachment is obsolete: true
Attached patch WIP (obsolete) (deleted) — — Splinter Review
Attachment #823920 - Attachment is obsolete: true
Attached patch Gecko dummy responder (obsolete) (deleted) — — Splinter Review
Attachment #823302 - Attachment is obsolete: true
Attached file IAC and Client APIs (obsolete) (deleted) —
Attachment #823279 - Attachment is obsolete: true
Attached file IAC and Client APIs (obsolete) (deleted) —
Attachment #824021 - Attachment is obsolete: true
Attached patch WIP (obsolete) (deleted) — — Splinter Review
Attachment #824012 - Attachment is obsolete: true
Attached patch fxamanager.patch (obsolete) (deleted) — — Splinter Review
Now the good one...
Attachment #824041 - Attachment is obsolete: true
Whiteboard: [qa?] → [qa+]
No longer depends on: 911384
Attached patch WIP (obsolete) (deleted) — — Splinter Review
Attachment #824046 - Attachment is obsolete: true
Depends on: 929386
Attached file IAC and Client APIs (deleted) —
Attachment #824022 - Attachment is obsolete: true
No longer depends on: 930598
Depends on: 935232
Depends on: 935234
No longer depends on: 929386
No longer depends on: 930792
No longer depends on: 935232
No longer depends on: 935242
No longer depends on: 935234
Attached patch WIP (obsolete) (deleted) — — Splinter Review
Attachment #825960 - Attachment is obsolete: true
Attached file Gaia Pull Request (deleted) —
Attachment #824016 - Attachment is obsolete: true
Attachment #830203 - Attachment is obsolete: true
Comment on attachment 8339148 [details] Gaia Pull Request Hi Alive! This is the WIP patch that I showed you yesterday. Could you take a look? Thanks a lot!! 谢谢! :)
Attachment #8339148 - Flags: feedback?(alive)
Attached image High level architecture (deleted) —
Thanks Alive. This is the higher level picture of the entire FxA system.
Comment on attachment 8339148 [details] 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?
Attachment #8339148 - Flags: feedback?(alive) → feedback+
Comment on attachment 8339148 [details] 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! 谢谢!
Attachment #8339148 - Flags: review?(alive)
Attachment #8339148 - Flags: review?(ferjmoreno)
Comment on attachment 8339148 [details] Gaia Pull Request This looks great! :) I added only some minor comments. I'd like to see a final version though. Thanks!
Attachment #8339148 - Flags: review?(ferjmoreno) → feedback+
Comment on attachment 8339148 [details] 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!
Attachment #8339148 - Flags: review?(alive)
Comment on attachment 8339148 [details] Gaia Pull Request Francisco! Could you take a look? Thanks!
Attachment #8339148 - Flags: review?(francisco.jordano)
Attachment #8339148 - Flags: review?(ferjmoreno)
Comment on attachment 8339148 [details] Gaia Pull Request r=me with the comments addressed, please. Also squash the commits and add r=ferjm, not r=fjimenez :P
Attachment #8339148 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8339148 [details] 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!
Attachment #8339148 - Flags: review?(francisco.jordano) → review+
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.
Flags: needinfo?(ferjmoreno)
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?
Flags: needinfo?(ferjmoreno) → needinfo?(francisco.jordano)
Right, that sounds to me like the perfect solution. Thanks Fernando!
Flags: needinfo?(francisco.jordano)
Great, thanks! I've just sent a PR with that change and a fix for the unit tests.
Travis Green, Feedback&R+, Merge this!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 959227
Blocks: 960130
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: