Incorporate a FxA Manager and FxA Client into the System app

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jedp, Assigned: ferjm)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa+])

Attachments

(3 attachments, 18 obsolete attachments)

3.04 KB, text/plain
Details
46 bytes, text/x-github-pull-request
arcturus
: review+
ferjm
: review+
alive
: feedback+
ferjm
: feedback+
Details | Review
75.73 KB, 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
Posted patch WIP (obsolete) — Splinter Review
Whiteboard: [qa?]
Posted patch WIP (obsolete) — 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
Posted patch WIP (obsolete) — Splinter Review
Now with web workers
Attachment #820987 - Attachment is obsolete: true
Blocks: 930074
No longer blocks: 930074
Depends on: 930074
Posted file IAC API version 2 (obsolete) —
Attachment #820428 - Attachment is obsolete: true
Posted patch WIP (obsolete) — 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
Posted patch WIP (obsolete) — Splinter Review
Attachment #821654 - Attachment is obsolete: true
Posted file IAC and Client APIs (obsolete) —
Attachment #821557 - Attachment is obsolete: true
Posted patch Gecko dummy responder (obsolete) — Splinter Review
Posted patch fxamanager.patch (obsolete) — Splinter Review
Attachment #823269 - Attachment is obsolete: true
Posted patch WIP (obsolete) — Splinter Review
Attachment #823920 - Attachment is obsolete: true
Posted patch Gecko dummy responder (obsolete) — Splinter Review
Attachment #823302 - Attachment is obsolete: true
Posted file IAC and Client APIs (obsolete) —
Attachment #823279 - Attachment is obsolete: true
Posted file IAC and Client APIs (obsolete) —
Attachment #824021 - Attachment is obsolete: true
Posted patch WIP (obsolete) — Splinter Review
Attachment #824012 - Attachment is obsolete: true
Posted patch fxamanager.patch (obsolete) — Splinter Review
Now the good one...
Attachment #824041 - Attachment is obsolete: true
Whiteboard: [qa?] → [qa+]
No longer depends on: 911384
Posted patch WIP (obsolete) — Splinter Review
Attachment #824046 - Attachment is obsolete: true
Depends on: 929386
Posted file IAC and Client APIs
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
Posted patch WIP (obsolete) — Splinter Review
Attachment #825960 - Attachment is obsolete: true
Attachment #824016 - Attachment is obsolete: true
Attachment #830203 - Attachment is obsolete: true
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!! 谢谢! :)
Attachment #8339148 - Flags: feedback?(alive)
Thanks Alive.

This is the higher level picture of the entire FxA system.
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?
Attachment #8339148 - Flags: feedback?(alive) → feedback+
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! 谢谢!
Attachment #8339148 - Flags: review?(alive)
Attachment #8339148 - Flags: review?(ferjmoreno)
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!
Attachment #8339148 - Flags: review?(ferjmoreno) → feedback+
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!
Attachment #8339148 - Flags: review?(alive)
Comment on attachment 8339148 [details] [review]
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] [review]
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] [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!
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: 6 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.