1395460 - Remove about:accounts

Status: VERIFIED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: Existing error page.

The FxA and Sync teams would like to see about:accounts die - the sync team because it's a maintenance burden and has strange e10s behaviours, the FxA team because the lose control over the flows.

The big downside of this is that about:accounts has a "pretty" error page for when a connection to accounts.firefox.com can't be established - removing it would mean you see the standard Firefox connection error page. Most of us don't think this is a bug deal, and it's not consistent anyway - eg, clicking "manage account" will show the standard error page in that situation.

This is really a UX call - so Ryan, can you please bless this?

Assuming we go ahead, we should *not* do this for 57 - let's wait for 58.

Attaching the current error page and what the error page would look like if we removed it.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: What the error page would look like without about:accounts

Comment 2

[Ryan Feeley [:rfeeley]]

Would this move kill the opportunity to present FxA in privileged UI?

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ryan Feeley [:rfeeley] from comment #2)
> Would this move kill the opportunity to present FxA in privileged UI?

I remember us discussing this, and on reflection, I don't believe it would have any impact on that.

Comment 4

[Ryan Feeley [:rfeeley]]

Then I'm all for it!

Comment 5

[Lina Butler [:lina]]

This might be a good bug to work on next...

Comment 6

[Edouard Oger [:eoger]]

Attachment: https://github.com/mozilla/activity-stream/pull/3759

Comment 7

[Edouard Oger [:eoger]]

Attachment: Bug 1395460 p1 - Remove usages of about:accounts.

Review commit: https://reviewboard.mozilla.org/r/192646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192646/

Comment 8

[Edouard Oger [:eoger]]

Attachment: Bug 1395460 p2 - Remove about:accounts.

Review commit: https://reviewboard.mozilla.org/r/192648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192648/

Comment 9

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8921654 [details]
Bug 1395460 p1 - Remove usages of about:accounts.

https://reviewboard.mozilla.org/r/192646/#review198250

This looks okay aside from one issue (which is kind of big). But please manually test this while pointed at a non-default url (e.g. stage).

::: services/fxaccounts/FxAccountsConfig.jsm:162
(Diff revision 1)
>        Services.prefs.setCharPref("identity.fxaccounts.remote.webchannel.uri", rootURL);
>        Services.prefs.setCharPref("identity.fxaccounts.settings.uri", rootURL + "/settings?service=sync&context=" + contextParam);
>        Services.prefs.setCharPref("identity.fxaccounts.settings.devices.uri", rootURL + "/settings/clients?service=sync&context=" + contextParam);
>        Services.prefs.setCharPref("identity.fxaccounts.remote.signup.uri", rootURL + "/signup?service=sync&context=" + contextParam);
>        Services.prefs.setCharPref("identity.fxaccounts.remote.signin.uri", rootURL + "/signin?service=sync&context=" + contextParam);
>        Services.prefs.setCharPref("identity.fxaccounts.remote.force_auth.uri", rootURL + "/force_auth?service=sync&context=" + contextParam);

I think you probably need to handle the new URI pref here. You might also need to add it to resetConfigURLs.

These allow you to only need to change 1 preference to point at stage/dev/local.

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8921655 [details]
Bug 1395460 p2 - Remove about:accounts.

https://reviewboard.mozilla.org/r/192648/#review198252

This looks okay, assuming you do the manual testing I mentioned before. There are a few try failures (as I mentioned in IRC), which you need to fix also.

Comment 11

[Edouard Oger [:eoger]]

Thanks Thom, this will need rebasing after bug 1411714 lands.

Comment 12

[Edouard Oger [:eoger]]

Comment on attachment 8921654 [details]
Bug 1395460 p1 - Remove usages of about:accounts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192646/diff/1-2/

Comment 13

[Edouard Oger [:eoger]]

Comment on attachment 8921655 [details]
Bug 1395460 p2 - Remove about:accounts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192648/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c028123001d
p1 - Remove usages of about:accounts. r=tcsc
https://hg.mozilla.org/integration/autoland/rev/0bc6d186d609
p2 - Remove about:accounts. r=tcsc

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failure:

https://hg.mozilla.org/integration/autoland/rev/4235b95666453a7d53afa8cc88c3d27765f82933

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139996257&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/uitour/UITour.jsm:556:21 | Expected to return a value at the end of arrow function. (consistent-return)

Comment 16

[Edouard Oger [:eoger]]

Comment on attachment 8921654 [details]
Bug 1395460 p1 - Remove usages of about:accounts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192646/diff/2-3/

Comment 17

[Edouard Oger [:eoger]]

Comment on attachment 8921655 [details]
Bug 1395460 p2 - Remove about:accounts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192648/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5caf5ea90a0
p1 - Remove usages of about:accounts. r=tcsc
https://hg.mozilla.org/integration/autoland/rev/b403d6dcdecc
p2 - Remove about:accounts. r=tcsc

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e5caf5ea90a0
https://hg.mozilla.org/mozilla-central/rev/b403d6dcdecc

Comment 20

[Julie McCracken (:julie)]

Edouard, what testing from Softvision is required?

Comment 21

[Edouard Oger [:eoger]]

I'm thinking they should test:
- Registration flow
- Sign-in flow
- Re-sign-in flow (password changed).
- Dev-edition link in about:preferences.
- Onboarding tour: Sync category.
- Activity Stream Sync Snippet (I don't think it's testable though).

Comment 25

[Kanchan Kumari QA]

I have performed regression testing for all the scenarios mentioned in comment 21(excluding Activity Stream Sync Snippet). No issues related to this code change were observed. Registration, sign-in and re-sign in flows through hamburger menus, first run page, dev-edition link, and onboarding tour functionalities are all working fine. I didn't encounter/discover any new issues
with existing FxA functionality due to this code change. Also, accounts.firefox.com error page looks as expected.

I discovered an issue, bug 1422172, while testing “Onboarding tour: Sync category”, which is not related to this bug fix. 

Tested Firefox Version: 58 beta 

Tested platforms: mac OS, Linux and Windows 10. 

Let me know if you need any additional testing here. Thanks!